[RFC] RSA lib integration #29

alt3r-3go · 2022-07-17T12:37:34Z

This is an "request for comments"-type PR to invite a discussion and iron out the details of the RSA library integration.

Context

We've discussed this with the Nitrokey team a while ago (around the end of 2021) and I took a stab at it, then it got paused for several months after this discussion we additionally had with @nickray. Recently @daringer suggested we move ahead anyway, probably in a form of an NK-specific temporary fork, while those TIPs are getting written/implemented. AFAIU this is driven by the desire to have an RSA implementation for NK3, so at least some working solution is valued higher than the ideal one we discussed in that Matrix chat.

Implemented now

RSA2K-PKCS_v1.5 signing/verification and the whole necessary set of helper traits (DeriveKey, etc) + tests for them, that work on a PC. I separately was able to run Trussed and the RSA lib's full set of tests on my nRF52832, but haven't yet tried both of them at once.

Caveats

The PR is still well WIP and has items to discuss (marked as TODO:alt3r-3go in the code). I've based the general approach on the miracl one (#12) and that proved to be quite useful, so kudos to @nickray for such a nice example. Due to one of the TODOs related to the test infra, tests need to be run one by one for now, at the bottom of this description I'm showing a simple script that can be used for that. I haven't included it into the code, but let me know if it's helpful until I close the TODO.

All comments, big and small, as well as suggestions on the TODOs are more than welcome.

Testing script

#!/bin/bash

set -e
set -x

clear && RUST_BACKTRACE=1 cargo test --test rsa2kpkcs rsa2kpkcs_sign_verify -- --nocapture --show-output
RUST_BACKTRACE=1 cargo test --test rsa2kpkcs rsa2kpkcs_generate -- --nocapture --show-output
RUST_BACKTRACE=1 cargo test --test rsa2kpkcs rsa2kpkcs_derive -- --nocapture --show-output
RUST_BACKTRACE=1 cargo test --test rsa2kpkcs rsa2kpkcs_exists -- --nocapture --show-output
RUST_BACKTRACE=1 cargo test --test rsa2kpkcs rsa2kpkcs_serialize -- --nocapture --show-output
RUST_BACKTRACE=1 cargo test --test rsa2kpkcs rsa2kpkcs_deserialize -- --nocapture --show-output
RUST_BACKTRACE=1 cargo test --test rsa2kpkcs rsa2kpkcs_sign_verify -- --nocapture --show-output

robin-nitrokey · 2022-07-20T12:50:57Z

Thank you for the PR! I did not have a closer look yet but I just want to give you a heads-up that we are already working on a solution for the test issue.

szszszsz · 2022-07-20T13:19:04Z

@alt3r-3go Hey! I plan to take a look at it this week

robin-nitrokey · 2022-07-21T10:52:25Z

@alt3r-3go #31 refactors the client used for the tests and should allow you to execute them normally with cargo test --features virt.

daringer · 2022-07-29T12:06:52Z

Hey @alt3r-3go,

great work, I will answer (hopefully all) of your TODOs here to have it in one place.

let's first stick (implementation-wise) to rsa2k only
generally cfg-feature granularity: I would suggest to have exactly 3 features for RSA: rsa2k, rsa3k and rsa4k
don't worry about the magic numbers for now, config.rs anyways (as this comment implies) lacks a proper magic-number-management-mechanism
using this configured numbers for KeyStore is perfectly fine for now - later this would need a compile-time constant (see magic-number-management-mechanism)
essentially the same argument for signature length, funny though that p256::Verify does not check signature length at all and ed255 does it based on a constant from salty - under the line: this is fine for now
serialization is perfectly fine with Raw only for now, keep it simple, as long as nothing forces us towards der or asn1der we stay with raw, which anyways implies pkcs#8-der (right?)
skip sign_blinded for now, to keep it simple (from here)
rsa takes a hash instead of raw data, this makes it a "prehased" algortihm, thus we should stick to how p256prehashed does it: request.message directly contains the hash.
sign & verify i.e. the PSS vs. PKCS padding story: p256 also implements two different variants for sign, so RSA should do the same. This means we will have something like rsa2k as feature and then something like:

pub struct rsa2k {}
pub struct rsa2kpss {}
mod rsa2k;

// and then

impl Sign for super::rsa2k {}
impl Sign for super::rsa2kpss {}

impl Verify for super::rsa2k {}
impl Verify for super::rsa2kpss {}

But again: keep it simple and stick to rsa2k (pkcs) first...

Sooo, hope this answers the questions.

Rebasing on top of main also allowed me to run all the tests at once: (thx @robin-nitrokey )

RUST_BACKTRACE=1 cargo test --test rsa2kpkcs --features virt -- --nocapture --show-output

This is really nice! Feels like nearly all of the heavy lifting stuff is done, despite the smaller changes described above (ok ignoring 3k, 4k - but that's totally fine for now). We anyways won't be able to upstream this until RSACrypto / RSA will have proper no_std support (which I cannot fully oversee what is missing at this point)

Signed-off-by: alt3r 3go <[email protected]>

alt3r-3go · 2022-07-31T14:33:13Z

Thanks @daringer, that makes a lot of sense and is certainly helpful. I've rebased to the latest main (tests indeed now work with virt, yay! 🙂) and added a commit that updates the code in accordance with those comments. In a separate commit for readability, will of course be squashed when we're done with the discussion.

[...] we stay with raw, which anyways implies pkcs#8-der (right?)

Other existing ones seem to store pure bytes when using Raw, however as those appropriate enum values are commented out anyway (and the rest simply doesn't fit, like e.g. Cose) we can probably get by for now by treating Raw as "whatever the serialize/deserialize implementation is assuming" and inside those functions I have comments as to the specific format for any future reworks.

[...] until RSACrypto / RSA will have proper no_std support (which I cannot fully oversee what is missing at this point)

As of ~6 months ago, when I actively tested this, it actually was running fine with no_std - I created a small test program for my nRF82532, which essentially included library's PKCS1 and 8 decoding tests, wrapped into proper incantations to run on nRF, and it worked just fine. It did require alloc (is removal of that was what you meant by "proper" here?) and an appropriate allocator implementation though, and alloc-cortex-m worked fine. I'm out of time today to update my test program to try the newer 0.6.1 library version, but I think that (i.e moving to the microcontroller target from PC) would anyway be the next step for this integration, so we'll know soon enough.

src/mechanisms.rs

In a separate commit for better visibility. Removed addressed TODOs, reworded those that should stay for later (magic numbers), rebased to the latest "main". Signed-off-by: alt3r 3go <[email protected]>

sosthene-nitrokey · 2022-09-12T09:20:39Z

I've rebased it on top of 8e347ab in sosthene-nitrokey:rsa-lib-integration-fmt

szszszsz · 2022-09-13T17:48:19Z

Hey @alt3r-3go !
Is there any chance you would have time to look into this PR again this week?

alt3r-3go · 2022-09-14T18:13:51Z

@szszszsz I probably could, but what exactly do you have in mind in terms of actions required? It seems to me it's ready to be merged into whatever place we'd designate for this, as per our past discussions, maybe based on the rebased branch that @sosthene-nitrokey mentioned. Are you maybe talking about namely that cargo-fmt commit and rebasing this PR specifically onto it?

sosthene-nitrokey · 2022-10-07T08:05:33Z

I mean't this PR should be reset to my https://github.com/sosthene-nitrokey/trussed/tree/rsa-lib-integration-fmt branch to resolve merge conflicts.

Sorry it took me so much time to respond, I missed the notification

sosthene-nitrokey · 2022-10-07T09:22:05Z

I'm not sure what is the best way to serialize public keys. For example the OpenPGP smartcard specification uses it's own serialization format, and to build it we need both e and n separately. I think we should try to avoid having to de-serialize it in the client app.

Maybe we could have two additional serialization formats RsaN and RsaE to get each individualy ? This seems ugly.

szszszsz · 2022-10-07T13:08:46Z

Hey @sosthene-nitrokey @alt3r-3go !
Just FYI, I've added public key serialization and decryption support below (based on @sosthene-nitrokey 's fork). How to proceed to integrate? I can make a PR to this PR in a few days (feel free to do it earlier if you need it):

6f4b547...Nitrokey:trussed:webcrypt-devel-rsa

Edit: @alt3r-3go : Sorry for the delay. Had hot month. I just thought something is still pending to do. Good to hear it's more or less finished! It works for my use case in Nitrokey WebCrypt as well.

sosthene-nitrokey · 2022-10-07T13:14:32Z

I'm aiso working on a modified branch on top of #51: here

sosthene-nitrokey · 2022-10-07T08:12:42Z

src/mechanisms/rsa2k.rs

+}
+
+#[cfg(feature = "rsa2k")]
+impl DeserializeKey for super::Rsa2kPkcs


DeserializeKey and SerializeKey should only work with public keys.

Private key import is done through UnsafeInjectKey, and private key export should never be an option.

I also think the serialization format should be Pkcs8, not Raw

Yeah, good catch. Looking at the code now, I'm not sure why I used the Secret one here. I was following the p256 and ed255 implementations, but apparently they don't do this either. Probably an artifact of working on this on and off for a longer time. I'll correct this. There's no Pkcs8 type though (that's why we settled on Raw with @daringer in his earlier comment). Do you mean we should add it?

sosthene-nitrokey · 2022-10-07T15:22:46Z

src/mechanisms/rsa2k.rs

+        // TODO: Consider using .sign_blinded(), which is supposed to protect the private key from timing side channels
+        use rsa::padding::PaddingScheme;
+        use rsa::hash::Hash;
+        let native_signature = priv_key


This can panic il the length of the data is not 32, it should return an error instead.

Also we may want to use another hash than SHA-256. I'd be in favor of using None for the hash. We need that for OpenPGP anyway, as all hashes can be used. it depends on the host.

It would require added documentation that the hash prefix needs to be added by the client though.

Right, this was an outcome of our earlier discussion with @daringer, where we decided to just treat this as prehashed, by contract. As for the None specifically, this is a bit error-prone, do we really have to provide that choice?

I agree this is somewhat uncomfortable, but for OpenPGP: in § 7.2.10, it mentions the digest prefix added for each hash that come as part of the input (DigestInfo) "The card may not check the correctness of the DigestInfo.". So we need to accept any input and not attempt to parse it or add any prefix other than the Pkcs#1v15 padding.

alt3r-3go · 2022-10-08T17:09:46Z

I'll look into and address specific comments later, but overall it looks like this needs to be merged somewhere, as more and more further changes start to pile up on top. I will rebase this on top of @sosthene-nitrokey's cargo-fmt branch, but let's also define where we want to merge this. One option we discussed before was an NK-owned Trussed fork, at least until mechanisms are in place to address @nickray's concerns cleanly. @daringer (or whoever is the right person to decide?), do you maybe want to create one? If not, how do you folks think we should move forward?

szszszsz · 2022-10-10T07:56:27Z

@sosthene-nitrokey @daringer @robin-nitrokey We should discuss this tomorrow

sosthene-nitrokey · 2022-10-10T08:51:40Z

src/mechanisms/rsa2k.rs

+
+        let key_id = request.key;
+
+        let priv_key_der = keystore.load_key(key::Secrecy::Secret, Some(key::Kind::Rsa2k), &key_id)


Signature verification should probably load a public key.

It does derive it, a little below. Though it indeed could load it directly, instead of deriving. This is probably the same thing as in Serialize/Deserialize, it looks like for whatever reason I assumed that keystore only holds private keys, or some such.

The keystore can hold both public and private keys. It's a bit of a shame that it needs to be told what key is being loaded rather than returning an enum allowing to match against the key type, which would be more flexible.

It does derive it, a little below.

Yes, but the current implementation means you can't verify a signature unless you also have the private key.

sosthene-nitrokey · 2022-10-11T16:05:58Z

Ok, so we talked about where we are going to go with this PR.

We're going have a version off this PR on the Nitrokey repo here. I already added some fixes that I needed for the OpenPGP card implementation.

With regard to serialization:

Since both OpenPGP and the SE050 secure element use a similar BER-TLV format ¹, we are going to use it for (de)serialization of RSA Key (with an added KeySerialization variant).

In case their is a need for Pkcs (de)serialization, we should keep it with an added KeySerialization variant.

For OpenPGP see page 75 and 38, for the SE050, see 4.7.1.2 WriteRSAKey and table 105 (for reading public keys it appears that the SE050 does it similarly with to what I do with 2 commands for reading N and E. ↩

alt3r-3go · 2022-10-12T05:02:10Z

Good! I see you've already fixed in that new PR the comments posted in this PR. Let me know if you need any further action from me. I also think we can close this PR and continue in the NK repo one - let me know if there are any objections.

alt3r-3go · 2022-10-23T12:43:34Z

Now that we've got the right place for this PR, let me close it. We can continue in that NK repo if/when needed, please just @-mention me, so that I don't miss the notification.

alt3r-3go changed the title ~~[RFC] RSA lib integration~~ [WIP] RSA lib integration Jul 17, 2022

alt3r-3go marked this pull request as draft July 17, 2022 12:46

alt3r-3go changed the title ~~[WIP] RSA lib integration~~ [RFC] RSA lib integration Jul 17, 2022

alt3r-3go and others added 9 commits July 31, 2022 13:56

WIP: RSA lib integration - part 1 (very first steps, incomplete)

ff24073

Signed-off-by: alt3r 3go <[email protected]>

WIP: RSA lib integration - part 2 (more plumbing, still incomplete)

20df3c7

Signed-off-by: alt3r 3go <[email protected]>

WIP: RSA lib integration - part 3 (RSA2K GenerateKey+test)

b01039e

Signed-off-by: alt3r 3go <[email protected]>

WIP: RSA lib integration - part 4 (DeriveKey+test)

edce7ab

Signed-off-by: alt3r 3go <[email protected]>

WIP: RSA lib integration - part 5 (Exists/{De}SerializeKey+tests)

0bdeb28

Signed-off-by: alt3r 3go <[email protected]>

WIP: RSA lib integration - part 6 (Sign/Verify+tests)

497bf9d

Signed-off-by: alt3r 3go <[email protected]>

WIP: remove unneeded code, mark my TODOs for simpler discovery

b72b80e

Signed-off-by: alt3r 3go <[email protected]>

WIP: Rebase to current trussed/main, cleanup

7bc9a69

Signed-off-by: alt3r 3go <[email protected]>

WIP: update RSA traits after updating to 0.6.0

c360dce

Signed-off-by: alt3r 3go <[email protected]>

alt3r-3go force-pushed the rsa-lib-integration branch from ecc399e to 16003b6 Compare July 31, 2022 14:03

sosthene-nitrokey reviewed Aug 17, 2022

View reviewed changes

src/mechanisms.rs Show resolved Hide resolved

sosthene-nitrokey mentioned this pull request Aug 17, 2022

Run cargo fmt #37

Closed

WIP: Address review comments - 1

c7cca84

In a separate commit for better visibility. Removed addressed TODOs, reworded those that should stay for later (magic numbers), rebased to the latest "main". Signed-off-by: alt3r 3go <[email protected]>

alt3r-3go force-pushed the rsa-lib-integration branch from 16003b6 to c7cca84 Compare August 20, 2022 15:05

sosthene-nitrokey reviewed Oct 7, 2022

View reviewed changes

sosthene-nitrokey reviewed Oct 10, 2022

View reviewed changes

alt3r-3go closed this Oct 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] RSA lib integration #29

[RFC] RSA lib integration #29

alt3r-3go commented Jul 17, 2022

robin-nitrokey commented Jul 20, 2022

szszszsz commented Jul 20, 2022

robin-nitrokey commented Jul 21, 2022

daringer commented Jul 29, 2022 •

edited

Loading

alt3r-3go commented Jul 31, 2022 •

edited

Loading

sosthene-nitrokey commented Sep 12, 2022

szszszsz commented Sep 13, 2022

alt3r-3go commented Sep 14, 2022

sosthene-nitrokey commented Oct 7, 2022 •

edited

Loading

sosthene-nitrokey commented Oct 7, 2022 •

edited

Loading

szszszsz commented Oct 7, 2022 •

edited

Loading

sosthene-nitrokey commented Oct 7, 2022

sosthene-nitrokey Oct 7, 2022

alt3r-3go Oct 11, 2022 •

edited

Loading

sosthene-nitrokey Oct 7, 2022

sosthene-nitrokey Oct 10, 2022

alt3r-3go Oct 11, 2022

sosthene-nitrokey Oct 11, 2022

alt3r-3go commented Oct 8, 2022

szszszsz commented Oct 10, 2022

sosthene-nitrokey Oct 10, 2022

alt3r-3go Oct 11, 2022

sosthene-nitrokey Oct 11, 2022

sosthene-nitrokey Oct 11, 2022

sosthene-nitrokey commented Oct 11, 2022

alt3r-3go commented Oct 12, 2022

alt3r-3go commented Oct 23, 2022


		let key_id = request.key;

		let priv_key_der = keystore.load_key(key::Secrecy::Secret, Some(key::Kind::Rsa2k), &key_id)

[RFC] RSA lib integration #29

[RFC] RSA lib integration #29

Conversation

alt3r-3go commented Jul 17, 2022

Context

Implemented now

Caveats

Testing script

robin-nitrokey commented Jul 20, 2022

szszszsz commented Jul 20, 2022

robin-nitrokey commented Jul 21, 2022

daringer commented Jul 29, 2022 • edited Loading

alt3r-3go commented Jul 31, 2022 • edited Loading

sosthene-nitrokey commented Sep 12, 2022

szszszsz commented Sep 13, 2022

alt3r-3go commented Sep 14, 2022

sosthene-nitrokey commented Oct 7, 2022 • edited Loading

sosthene-nitrokey commented Oct 7, 2022 • edited Loading

szszszsz commented Oct 7, 2022 • edited Loading

sosthene-nitrokey commented Oct 7, 2022

Choose a reason for hiding this comment

alt3r-3go Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alt3r-3go commented Oct 8, 2022

szszszsz commented Oct 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sosthene-nitrokey commented Oct 11, 2022

Footnotes

alt3r-3go commented Oct 12, 2022

alt3r-3go commented Oct 23, 2022

daringer commented Jul 29, 2022 •

edited

Loading

alt3r-3go commented Jul 31, 2022 •

edited

Loading

sosthene-nitrokey commented Oct 7, 2022 •

edited

Loading

sosthene-nitrokey commented Oct 7, 2022 •

edited

Loading

szszszsz commented Oct 7, 2022 •

edited

Loading

alt3r-3go Oct 11, 2022 •

edited

Loading