-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add RSA variants, experimental LMS and LM-OTS to algorithm registry #199
Conversation
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Will take a longer look next week! A few quick thoughts:
Another thought, should we put PQ algs into a separate experimental enum which has no guarantees with respect to breaking changes? That may also signal these are a work in progress. |
That's my understanding of the use case, but I'm just the lowly implementer 🙂. @tpletcher-hpe might be able to offer some additional context (here or on Slack; I can set that chat up).
This makes sense to me -- the clear, safe case for LM-OTS is keyless signing, which should be relatively misuse-resistant so long as the Sigstore client always generates a new keypair per signing operation.
RFC 8554 has some parameter recommendations, including "top-level" recommendations for HSS (constructed on LMS) based on different tree depths and signing volume/longevity assumptions. All of those recommendations have reasonable security margins, but unfortunately aren't easy to generalize (since some clients may only need a key-tree that's valid for a few minutes, while another might need one that's valid for decades).
That sounds reasonable to me. @tetsuo-cpp and @ret2libc: do you think having these in a separate enum would pose an integration challenge? |
I think it would be ideal to treat them as the same type, otherwise integration might become harder. If we can keep them separate while still treating them the same type on the Go-side (and other languages as well, actually), then I'm ok. |
Hmm, this might be hard without a layer of wrapper types in between. Given that enum variants are "cheap" and that nothing else uses Thoughts @haydentherapper? |
I'd prefer that idea also. If they're captured in separate enums, we'll have to wrap the enums behind an |
That sounds good with me. |
Yes, |
But I still want to raise the question of how The Wouldn't it make sense to only use that enum to capture the use-case here too (i.e add new experimental algorithms)? From a clients side perspective I'm kind of scratching my head a little, what's the intent of Or is the idea for this to be a list of valid signature algorithms for the clients, i.e what is allowed to use for a client in their signing cert, or when signing a rekord to be pushed to Rekor? With a list here, we could reference from Fulcio or Rekor what algorithms they support? |
Signed-off-by: William Woodruff <[email protected]>
protos/sigstore_common.proto
Outdated
EXPERIMENTAL_LMOTS_SHA256 = 15; | ||
|
||
// Reserved for future additions of public key/signature algorithm types. | ||
reserved 16 to 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: I've added a reserved range here, similar to some of the other messages we have. I figure this will make things a little more flexible on the wire format side, although I'm not sure if it matters in the JSON serialization case.
Rewritten to extend |
Signed-off-by: William Woodruff <[email protected]>
protos/sigstore_common.proto
Outdated
RSA_PSS_2048_SHA256 = 16; | ||
RSA_PSS_3072_SHA256 = 17; | ||
RSA_PSS_4096_SHA256 = 18; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From #212 (comment): maybe we want to explicitly prefix these with PKIX_
, emphasizing that they're encoded according to RFC 4055? Thoughts?
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates!
Can you update the PR title to note the RSA scheme additions?
protos/sigstore_common.proto
Outdated
// RSA public key in PKIX format, PKCS#1v1.5 signature | ||
PKIX_RSA_PKCS1V15_2048_SHA256 = 9; | ||
PKIX_RSA_PKCS1V15_3072_SHA256 = 10; | ||
PKIX_RSA_PKCS1V15_4096_SHA256 = 11; | ||
// RSA public key in PKIX format, RSASSA-PSS signature | ||
PKIX_RSA_PSS_2048_SHA256 = 16; // See RFC4055 | ||
PKIX_RSA_PSS_3072_SHA256 = 17; | ||
PKIX_RSA_PSS_4096_SHA256 = 18; | ||
// RSA public key in PKCS#1 format, PKCS#1v1.5 signature | ||
PKCS1_RSA_PKCS1V15_2048_SHA256 = 19; | ||
PKCS1_RSA_PKCS1V15_3072_SHA256 = 20; | ||
PKCS1_RSA_PKCS1V15_4096_SHA256 = 21; | ||
// RSA public key in PKCS#1 format, RSASSA-PSS signature | ||
PKCS1_RSA_PSS_2048_SHA256 = 22; // See RFC4055 | ||
PKCS1_RSA_PSS_3072_SHA256 = 23; | ||
PKCS1_RSA_PSS_4096_SHA256 = 24; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the full linearization of supported RSA key format + signature format options.
As a random thought: given that the PKIX encoding is just an encapsulation of the PKCS1 encoding for RSA public keys, maybe it makes sense to encourage the ecosystem as a whole to uniformly use the PKIX encoding? In other words: rather than explicitly enumerating the PKCS1_RSA_*
variants, maybe we could say that they're subsumed under the deprecated PKCS1_RSA_PKCS1V5
variant and that implementations should transform their public keys into PKIX instead?
(I don't know if this is a good idea or not, it just came to mind as an option if we want to reduce the enum size here 🙂)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the public instance, we will only use PKIX. PKCS1 would cover private deployments. Do you know how much support there is for PKCS1 encodings in other languages? As in, if the only sigstore client that supports pkcs1 encoded rsa keys is cosign, maybe we drop these for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know how much support there is for PKCS1 encodings in other languages? As in, if the only sigstore client that supports pkcs1 encoded rsa keys is cosign, maybe we drop these for now?
Not sure about JS or Java, but we don't support PKCS1-encoded keys in sigstore-python
. CC @bdehamer @loosebazooka @kommendorkapten for additional datapoints here 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would to ignore PKCS1 encoded keys (that's why I didn't add them, only kept the old definitions as deprecated
). I'll look into JS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sigstore-js only supports PKIX keys now: https://github.com/sigstore/sigstore-js/blob/ed19022f2817c8444cc343984609102d0324fe4f/packages/core/src/crypto.ts#L25
Side note is that nodejs documentation also recommends using PKIX for persisting public keys for long-term storage.
That said it would be possible to enable usage of PKCS1 encoded keys in sigstore-js, but as mentioned earlier, I rather not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! Given that, I think we can entirely drop the PKCS1 variants here (and re-add later, if needed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on dropping. FWIW, golang has made support for other key types and formats so straightforward that we forget the complexity of adding the support in other languages. I don’t know of any use cases for pkcs1 besides BYO-key, which we can address if that comes up or just keep using the legacy enum.
Done! |
Thanks, good to see this. My preference is to remove the PCS1 encoding for RSA, then we are ready to merge. |
Signed-off-by: William Woodruff <[email protected]>
Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Opening this up to get a discussion started! I suspect this will require a decent amount of discussion and review before it can be merged.
Some particular points worth highlighting:
LMOTS_SHA256_N32_W8
almost always), but LMS is harder to make general purpose recommendations for (since the different performance/size tradeoffs will vary for different signer setups).CC @tetsuo-cpp @ret2libc for visibility