Skip to content
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

Create Algorithm Registry API #1601

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

tetsuo-cpp
Copy link

Summary

As part of our work to support configurable crypto algorithms across the Sigstore stack, we've added a KnownSignatureAlgorithm enumeration to protobuf-specs to reflect what signature algorithms are part of the official Sigstore algorithm registry. This PR adds some helpers and APIs to make use of this registry in Sigstore services.

Release Note

  • Added GetAlgorithmDetails helper to access the key algorithm, hash algorithm and other algorithm-specific parameters for a given signature algorithm.
  • Added AlgorithmRegistryConfig API to check a public key and hash algorithm combination against a set of accepted signature algorithms.
  • Added FormatSignatureAlgorithmFlag and ParseSignatureAlgorithmFlag to convert the signature algorithm type to and from a string representation that can be used as a CLI parameter.

Documentation

Signed-off-by: Alex Cameron <asc@tetsuo.sh>
Signed-off-by: Alex Cameron <asc@tetsuo.sh>
@tetsuo-cpp
Copy link
Author

I'm working on some unit tests now.

@tetsuo-cpp tetsuo-cpp changed the title Create Algorithm Registry Config API Create Algorithm Registry API Jan 18, 2024
Signed-off-by: Alex Cameron <asc@tetsuo.sh>
return a.hashType == hashType
}

var algorithmDetails = []algorithmDetailsImpl{
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved away from that design where I had an interface and an implementation for each signature algorithm as it was really doing anything for us. Each AlgorithmRegistryConfig now holds pointers to algorithms in this list.

return fmt.Errorf("signing algorithm not permitted: %T, %s", key, hash)
}

// FormatSignatureAlgorithmFlag formats a v1.KnownSignatureAlgorithm to a string that conforms to the conventions of CLI
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CLI stuff isn't terribly efficient but I don't expect it to be on the hot path since this is just initialisation code.

pkg/signature/algorithm_registry.go Show resolved Hide resolved
pkg/signature/algorithm_registry.go Show resolved Hide resolved
pkg/signature/algorithm_registry.go Outdated Show resolved Hide resolved
pkg/signature/algorithm_registry.go Outdated Show resolved Hide resolved
@tetsuo-cpp
Copy link
Author

pkg/signature/algorithm_registry.go Outdated Show resolved Hide resolved
pkg/signature/algorithm_registry.go Outdated Show resolved Hide resolved
pkg/signature/algorithm_registry.go Outdated Show resolved Hide resolved
pkg/signature/algorithm_registry.go Show resolved Hide resolved
pkg/signature/algorithm_registry.go Show resolved Hide resolved
woodruffw and others added 2 commits January 18, 2024 10:26
Co-authored-by: Riccardo Schirone <562321+ret2libc@users.noreply.github.com>
Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@trailofbits.com>
@ret2libc
Copy link
Contributor

@tetsuo-cpp you can see https://github.com/trail-of-forks/cosign/compare/support-ed25519ph...trail-of-forks:cosign:signing-algorithm-flag?expand=1 on how I might be using the registry in cosign. Hope it helps us define a good API!

This reverts commit 9eec947.

Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
Copy link
Member

(Force-pushed to fix the signoffs.)

pkg/signature/algorithm_registry.go Outdated Show resolved Hide resolved
pkg/signature/algorithm_registry.go Show resolved Hide resolved
}

// IsAlgorithmPermitted checks whether a given public key/hash algorithm combination is permitted by a registry config.
func (registryConfig AlgorithmRegistryConfig) IsAlgorithmPermitted(key crypto.PublicKey, hash crypto.Hash) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you foresee this being used with KMS keys? Should the caller be required to inspect the KMS key for its key and hash (we have key, we're missing hash, but it's being implemented in #1426). Should each KMS provider has its own registry?(as a service, this would be a bit annoying to use, since you'd have to consult a registry of regstries)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes most sense for the calling code to inspect the KMS key and hash, and check against the registry to see whether it should use that key or not.

I don't think it makes sense for each KMS provider to own its own registry, but perhaps the kms.SignerVerifier API could take an optional AlgorithmRegistryConfig and do the checking there? But I'd lean towards not tangling the APIs together like that and just let the calling code compose these calls as required.

What are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me. Long-term I'd lean towards the proposal to take in a config, just to avoid each client having to reimplement this, but I don't think it's something that's blocking regardless, just thinking through how we'll handle users who provide their own KMS keys.

pkg/signature/algorithm_registry.go Outdated Show resolved Hide resolved
Signed-off-by: Alex Cameron <asc@tetsuo.sh>
Signed-off-by: Alex Cameron <asc@tetsuo.sh>
Signed-off-by: Alex Cameron <asc@tetsuo.sh>
Signed-off-by: Alex Cameron <asc@tetsuo.sh>
pkg/signature/algorithm_registry.go Outdated Show resolved Hide resolved
pkg/signature/algorithm_registry.go Outdated Show resolved Hide resolved
Signed-off-by: Alex Cameron <asc@tetsuo.sh>
go.mod Outdated Show resolved Hide resolved
}

// IsAlgorithmPermitted checks whether a given public key/hash algorithm combination is permitted by a registry config.
func (registryConfig AlgorithmRegistryConfig) IsAlgorithmPermitted(key crypto.PublicKey, hash crypto.Hash) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me. Long-term I'd lean towards the proposal to take in a config, just to avoid each client having to reimplement this, but I don't think it's something that's blocking regardless, just thinking through how we'll handle users who provide their own KMS keys.

Signed-off-by: Alex Cameron <asc@tetsuo.sh>
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, please take a look at the lint failure

if err != nil {
t.Errorf("unexpected error creating algorithm registry config: %v", err)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add a test for RSA keys being permitted?

It might be overkill, but ideally we'd have a test for every supported key type, to have maximum code coverage.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed-off-by: Alex Cameron <asc@tetsuo.sh>
failure cases

Signed-off-by: Alex Cameron <asc@tetsuo.sh>
@haydentherapper
Copy link
Contributor

@kommendorkapten Do you want to take a look at this PR? There's an open question of how this should interact with PublicKeyDetails (thread)

Co-authored-by: Hayden B <hblauzvern@google.com>
Signed-off-by: Alex Cameron <asc@tetsuo.sh>
@kommendorkapten
Copy link
Member

This is great 🚀
This is quite related to the existing PublicKeyDetails, which today is used when distributing the trust root, to indicate how to interpret and use public key material, e.g. the public key for Rekor.

I believe there is an overlap between KnownSignatureAlgorithm and PublicKeyDetails, and I fear this may become a bit complicated for clients to work with. How do we feel about merging them into PublicKeyDetails?

PublicKeyDetails captures the signature algorithm used (albeit a bit underspecified for RSA keys, but that can easily be fixed), which is exactly what KnownSignatureAlgorithm does.

The only differences I see is the naming, and that PublicKeyDetails also indicates the encoding of the public key material, but I would think that it wouldn't really matter, I would say that it's only RSA keys that in practice may have different encodings (PKCS vs PKIX).

One can argue that the two types might solve slightly different problems, but as the overlap is so big, I would feel better if they could be combined, otherwise the clients would need to translate between the two formats, which I feel is sub-optimal.

Thoughts?

@tetsuo-cpp
Copy link
Author

The only differences I see is the naming, and that PublicKeyDetails also indicates the encoding of the public key material, but I would think that it wouldn't really matter, I would say that it's only RSA keys that in practice may have different encodings (PKCS vs PKIX).

Aren't the RSA values also missing the hash algorithm, or is that implied somehow? That was the main reason I didn't add to PublicKeyDetails (as well as the key encoding). It seemed to me that the KnownSignatureAlgorithm enum would be capturing a slightly different set of information than what's in PublicKeyDetails.

If it's fine to overwrite the values in PublicKeyDetails with what's in KnownSignatureAlgorithm (that is, if we can avoid distinguishing how the key is encoded as well as add hash functions and key sizes for each of the RSA options), then I don't see anything stopping us from merging the two.

@kommendorkapten
Copy link
Member

Aren't the RSA values also missing the hash algorithm, or is that implied somehow?

I don't remember the reason we left the RSA values underspecified 🤔 , but adding new values with them in there would make no harm.

Yeah, adding the new values missing in PublicKeyDetails is preferable IMHO. The old RSA values can be marked as [deprecated = true]. I believe RSA are the only ones that needs updates, the EC types matches what's in KownSignatureAlgorithms (but only P256 is listed). I would also be tempted to mark deterministic ECDSA as deprecated, I don't think anyone is using that?

@tetsuo-cpp
Copy link
Author

Yeah, adding the new values missing in PublicKeyDetails is preferable IMHO. The old RSA values can be marked as [deprecated = true]. I believe RSA are the only ones that needs updates, the EC types matches what's in KownSignatureAlgorithms (but only P256 is listed). I would also be tempted to mark deterministic ECDSA as deprecated, I don't think anyone is using that?

That seems reasonable.

I had another idea though: what do you think about marking the entire PublicKeyDetails enum as deprecated and then move any code using this enum to use KnownSignatureAlgorithms? That way, we can maintain backwards compatibility as you're suggesting without diverging from the algorithm registry markdown and having lots of deprecated options mixed in with legitimate ones in the short term.

@kommendorkapten
Copy link
Member

I had another idea though: what do you think about marking the entire PublicKeyDetails enum as deprecated

My gut feeling is that this will be quite disruptive for the community, as this field is being used in quite a few places. And it would also require us to have mechanism of indicating the encoding of the key (especially for RSA keys). There are quite a few implementations out there already (Go, Java, JavaScript, Python, and some tooling for managing the trust root which depends on the pubplic key type).

without diverging from the algorithm registry markdown

Hm, why would the public key type diverge from that? Because of the added encoding info?

@tetsuo-cpp
Copy link
Author

My gut feeling is that this will be quite disruptive for the community, as this field is being used in quite a few places. And it would also require us to have mechanism of indicating the encoding of the key (especially for RSA keys). There are quite a few implementations out there already (Go, Java, JavaScript, Python, and some tooling for managing the trust root which depends on the pubplic key type).

I think I understand. My reasoning was that if we're deprecating most of the existing values, it'll already be disruptive and will require modification to any code that reads/writes those enums. But there's probably some code where the enum gets passed around without operating on it which would recompile fine if we stick with PublicKeyDetails, whereas removing that enum will break that code. Is that what you mean?

Hm, why would the public key type diverge from that? Because of the added encoding info?

Yeah, I expect the number of values for RSA will double since we'll be duplicating them to include the encoding info.

@kommendorkapten
Copy link
Member

recompile fine if we stick with PublicKeyDetails, whereas removing that enum will break that code. Is that what you mean?

Yeah, I would envision something like this:

enum PublicKeyDetails {
        PUBLIC_KEY_DETAILS_UNSPECIFIED = 0;
        // RSA
        PKCS1_RSA_PKCS1V5 = 1 [deprecated = true]; // See RFC8017
        PKCS1_RSA_PSS = 2 [deprecated = true]; // See RFC8017
        PKIX_RSA_PKCS1V5 = 3 [deprecated = true];
        PKIX_RSA_PSS = 4 [deprecated = true];
        PKIX_RSA_PKCS1_2048_SHA256 = 9;
        PKIX_RSA_PKCS1_3072_SHA256 = 10;
        PKIX_RSA_PKCS1_4096_SHA256 = 11;
       
        // ECDSA
        PKIX_ECDSA_P256_SHA_256 = 5; // See NIST FIPS 186-4
        PKIX_ECDSA_P256_SHA_384 = 12;
        PKIX_ECDSA_P256_SHA_521 = 13;        
        PKIX_ECDSA_P256_HMAC_SHA_256 = 6 [deprecated = true]; // See RFC6979
       
        // Ed 25519
        PKIX_ED25519 = 7; // See RFC8032
        PKIX_ED25519_PH = 8;
}

So there would be only additions for the new curves and more specified RSA ones. And as (at least to my knowledge) the currently most commonly used key-type is NIST P256, this wouldn't be disruptive, and we can let the deprecated RSA keys be there for a few releases to let any users of that update.

I think we can be prescriptive here about encoding and signature scheme for RSA, but curious to hear from e.g. @haydentherapper to minimize the duplication.

@woodruffw
Copy link
Member

(Sorry, catching up here!)

@kommendorkapten's proposal looks reasonable to me -- it'd be nice to deduplicate here and reuse PublicKeyDetails as the registry enumeration, provided that doesn't cause issues for clients (I don't see why it would).

Two thoughts:

  1. Specifying the full suite (i.e. PK type + hash) shouldn't be an issue from an encoding perspective -- there are standard X.509 AlgorithmIdentifiers for RSA+PSS and RSA+PKCS1v1.5 that encode hash algorithm choices. That being said, those identifiers don't encode public modulus length, so in practice there won't be any X.509 encoding difference between PKIX_RSA_PKCS1_2048_SHA256 and PKIX_RSA_PKCS1_3072_SHA256, etc. This may or may not matter; we may want to keep them as separate enum variants regardless 🙂

  2. The PublicKeyDetails name is slightly misleading, since the enum now (fully) captures both the public key type and its signature parameter set. This is fine IMO (especially since some variants already do this, but I figure it's worth calling it).

@kommendorkapten
Copy link
Member

That being said, those identifiers don't encode public modulus length, so in practice there won't be any X.509 encoding difference between PKIX_RSA_PKCS1_2048_SHA256 and PKIX_RSA_PKCS1_3072_SHA256, etc. This may or may not matter; we may want to keep them as separate enum variants regardless 🙂

I don't have any strong preferences against any of the options here I think. As the public key is part of the function to verify a key and hash, we could instead of matching against a set list only verify that the public modulus length is larger or equal than say 2048 (or compare against a set of lengths).

The PublicKeyDetails name is slightly misleading, since the enum now (fully) captures both the public key type and its signature parameter set.

Yes, the name may not be optimal, but I don't think it's that bad 😄

@kommendorkapten
Copy link
Member

See sigstore/protobuf-specs#212 for PR on merge of PublicKeyDetails and KnownSignatureAlgorithms.

@tetsuo-cpp
Copy link
Author

See sigstore/protobuf-specs#212 for PR on merge of PublicKeyDetails and KnownSignatureAlgorithms.

Nice work, thanks for that. Once a release is cut, I'll get this PR up to date.

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. As a meta comment, my only reservation is it's hard to know if we've gotten the API right here without seeing this in use. I'm not really concerned though since I don't expect this API to be used outside of the agility work initially.

go.mod Outdated
@@ -13,6 +13,7 @@ require (
github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8
github.com/secure-systems-lab/go-securesystemslib v0.8.0
github.com/segmentio/ksuid v1.0.4
github.com/sigstore/protobuf-specs v0.3.0-beta.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can bump up now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I also had to use PublicKeyDetails as KnownSignatureAlgorithm was removed with recent changes.

@ret2libc
Copy link
Contributor

Overall LGTM. As a meta comment, my only reservation is it's hard to know if we've gotten the API right here without seeing this in use. I'm not really concerned though since I don't expect this API to be used outside of the agility work initially.

@haydentherapper you can see its use in other pending PRs like sigstore/rekor#1974 , sigstore/cosign#3497 , sigstore/fulcio#1517 (this last one might be outdated)

Signed-off-by: Riccardo Schirone <riccardo.schirone@trailofbits.com>
var algorithmDetails = []algorithmDetailsImpl{
{v1.PublicKeyDetails_PKIX_RSA_PKCS1V15_2048_SHA256, RSA, crypto.SHA256, RSAKeySize(2048), "rsa-sign-pkcs1-2048-sha256"},
{v1.PublicKeyDetails_PKIX_RSA_PKCS1V15_3072_SHA256, RSA, crypto.SHA256, RSAKeySize(3072), "rsa-sign-pkcs1-3072-sha256"},
{v1.PublicKeyDetails_PKIX_RSA_PKCS1V15_4096_SHA256, RSA, crypto.SHA256, RSAKeySize(4096), "rsa-sign-pkcs1-4096-sha256"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add the RSA/PSS options here too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also document we aren't handling the deprecated options, namely a PKCS1v1.5 encoded RSA key, preferring PKIX encoding.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants