Create Algorithm Registry API by tetsuo-cpp · Pull Request #1601 · sigstore/sigstore

tetsuo-cpp · 2024-01-18T11:00:34Z

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

tetsuo-cpp · 2024-01-18T11:00:56Z

I'm working on some unit tests now.

tetsuo-cpp · 2024-01-18T12:57:37Z

pkg/signature/algorithm_registry.go

+	return a.hashType == hashType
+}
+
+var algorithmDetails = []algorithmDetailsImpl{


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.

tetsuo-cpp · 2024-01-18T12:59:06Z

pkg/signature/algorithm_registry.go

+	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


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

tetsuo-cpp · 2024-01-18T13:04:22Z

CC: @ret2libc @woodruffw @haydentherapper

pkg/signature/algorithm_registry.go

ret2libc · 2024-01-19T16:55:12Z

@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!

woodruffw · 2024-01-19T20:12:02Z

(Force-pushed to fix the signoffs.)

pkg/signature/algorithm_registry.go

Hayden-IO · 2024-01-19T22:26:55Z

pkg/signature/algorithm_registry.go

+}
+
+// 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 {


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)

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?

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

go.mod

Hayden-IO · 2024-01-24T00:11:30Z

pkg/signature/algorithm_registry.go

+}
+
+// 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 {


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.

bobcallaway · 2024-01-24T18:40:16Z

pkg/signature/algorithm_registry.go

+// GetAlgorithmDetails retrieves a set of details for a given v1.KnownSignatureAlgorithm flag that allows users to
+// introspect the public key algorithm, hash algorithm and more.
+func GetAlgorithmDetails(knownSignatureAlgorithm v1.KnownSignatureAlgorithm) (AlgorithmDetails, error) {
+	for _, detail := range algorithmDetails {


maybe turn this into a map[v1.KnownSignatureAlgorithm]AlgorithmDetails ?

I was thinking about this, but my reasons not to were:

If I do that, I have to match the v1.KnownSignatureAlgorithm in the AlgorithmDetails with the one in the map key, which is another opportunity for subtle typos that can introduce bugs. I can't remove it from AlgorithmDetails since we need it for things like AlgorithmRegistryConfig.ListPermittedAlgorithms.

The algorithmDetails array only includes an entry for each signature algorithm in the registry which by design, is a small, constrained set of algorithms. I haven't benchmarked and to be honest, I don't know much about Go's performance characteristics, but I wouldn't expect the map to be faster than a simple linear search for this number of elements.

This array isn't part of the public API so if my assumptions are wrong in the long term, we can easily walk this decision back if we need to without introducing breaking changes.

Happy to change it over to a map, if you'd prefer though.

Hayden-IO

Overall LGTM, please take a look at the lint failure

Hayden-IO · 2024-01-29T21:50:02Z

pkg/signature/algorithm_registry_test.go

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


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.

pkg/signature/algorithm_registry.go

Hayden-IO · 2024-01-30T00:17:47Z

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

kommendorkapten · 2024-01-30T12:25:50Z

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 · 2024-01-30T13:57:47Z

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 · 2024-01-30T14:06:25Z

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 · 2024-01-30T14:14:29Z

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.

ret2libc · 2024-02-19T08:41:40Z

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)

pkg/signature/algorithm_registry.go

Signed-off-by: Riccardo Schirone <[email protected]> Signed-off-by: Alex Cameron <[email protected]> Signed-off-by: William Woodruff <[email protected]> Co-authored-by: Alex Cameron <[email protected]> Co-authored-by: Riccardo Schirone <[email protected]>

ret2libc

I have squashed the whole history into 1 commit because there were very old iterations that did not make sense anymore.

The PR was tested with fulcio/rekor/cosign.

With regards to the API and its uses: to be honest a few functions are not used anywhere, so we may make the API even smaller and hide more things, if we want. So if/when it grows/changes it will be easier to do so.

In particular, these functions are not used in any of the other components:

HasAlgorithmDetails
ListPermittedAlgorithms
registry.GetAlgorithmDetails
GetAlgorithmDetails

ret2libc · 2025-01-21T18:07:33Z

ping @haydentherapper

There has been a lot of back and forth on this PR and the others. I have updated all of them, so I'd appreciate a new review on this.

Hayden-IO · 2025-01-27T21:42:27Z

Thanks @ret2libc for the updates! I did a review over the PR and reviewed my old comments to remember where we had ended - I think the PR is looking good!

For those functions, if they're unused, I'd agree that we can omit then. I'd rather keep the API surface smaller and expand it later as needed. Do you recall why we had proposed those? It seems like those are mostly focused on exposing the configuration mapping, which I assume would only be relevant to developers who could inspect the source code.

ret2libc · 2025-01-28T14:48:48Z

Thanks @ret2libc for the updates! I did a review over the PR and reviewed my old comments to remember where we had ended - I think the PR is looking good!

For those functions, if they're unused, I'd agree that we can omit then. I'd rather keep the API surface smaller and expand it later as needed. Do you recall why we had proposed those? It seems like those are mostly focused on exposing the configuration mapping, which I assume would only be relevant to developers who could inspect the source code.

I think they were just exposed to have a "nice API" and maybe also because we did not know how we would have ended up using the API when we started with this.

I'm going to clean the API!

…ry-api

Signed-off-by: Riccardo Schirone <[email protected]>

Hayden-IO

This looks great! Any other changes or is this good to merge?

go.mod

Signed-off-by: Riccardo Schirone <[email protected]>

Hayden-IO · 2025-02-04T19:40:08Z

Going to merge now, thanks all!

tetsuo-cpp changed the title ~~Create Algorithm Registry Config API~~ Create Algorithm Registry API Jan 18, 2024

tetsuo-cpp commented Jan 18, 2024

View reviewed changes

ret2libc reviewed Jan 18, 2024

View reviewed changes

woodruffw force-pushed the alex/algorithm-registry-api branch from 9eec947 to 1be7204 Compare January 18, 2024 17:54

woodruffw force-pushed the alex/algorithm-registry-api branch 2 times, most recently from 5b727de to dfb835b Compare January 19, 2024 20:11

Hayden-IO reviewed Jan 19, 2024

View reviewed changes

tetsuo-cpp requested a review from Hayden-IO January 23, 2024 13:30

ret2libc suggested changes Jan 23, 2024

View reviewed changes

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

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

ret2libc mentioned this pull request Jan 23, 2024

Added --client-signing-algorithms flag sigstore/rekor#1974

Merged

Hayden-IO previously approved these changes Jan 24, 2024

View reviewed changes

Hayden-IO requested a review from bobcallaway January 24, 2024 00:13

tetsuo-cpp dismissed Hayden-IO’s stale review via c14bfce January 24, 2024 04:50

Hayden-IO previously approved these changes Jan 24, 2024

View reviewed changes

ret2libc approved these changes Jan 24, 2024

View reviewed changes

bobcallaway reviewed Jan 24, 2024

View reviewed changes

tetsuo-cpp requested a review from bobcallaway January 25, 2024 04:21

tetsuo-cpp dismissed Hayden-IO’s stale review via d89eade January 29, 2024 05:59

Hayden-IO reviewed Jan 29, 2024

View reviewed changes

tetsuo-cpp requested a review from Hayden-IO January 29, 2024 22:43

Hayden-IO reviewed Jan 30, 2024

View reviewed changes

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

ret2libc dismissed Hayden-IO’s stale review via d548633 February 19, 2024 09:04

ret2libc force-pushed the alex/algorithm-registry-api branch from d548633 to e6f153b Compare February 19, 2024 09:06

kommendorkapten reviewed Feb 19, 2024

View reviewed changes

pkg/signature/algorithm_registry.go Show resolved Hide resolved

ret2libc force-pushed the alex/algorithm-registry-api branch 2 times, most recently from 4b47bc2 to 51e2bd0 Compare January 21, 2025 09:31

Create Algorithm Registry API

9b265f4

Signed-off-by: Riccardo Schirone <[email protected]> Signed-off-by: Alex Cameron <[email protected]> Signed-off-by: William Woodruff <[email protected]> Co-authored-by: Alex Cameron <[email protected]> Co-authored-by: Riccardo Schirone <[email protected]>

ret2libc force-pushed the alex/algorithm-registry-api branch from 51e2bd0 to 9b265f4 Compare January 21, 2025 09:33

ret2libc reviewed Jan 21, 2025

View reviewed changes

woodruffw mentioned this pull request Jan 21, 2025

[META] Cryptographic agility for Sigstore clients and services sigstore/sig-clients#16

Open

10 tasks

ret2libc added 6 commits January 28, 2025 17:41

Merge remote-tracking branch 'origin/main' into alex/algorithm-regist…

4cf4cd8

…ry-api

pkg/signature/kms/cliplugin: update go.mod/sum

846f49a

Signed-off-by: Riccardo Schirone <[email protected]>

pkg/signature/algorithm_registry: minimize API

f821063

Signed-off-by: Riccardo Schirone <[email protected]>

test/cliplugin: update go.mod/sum

fb16a4b

Signed-off-by: Riccardo Schirone <[email protected]>

pkg/signature: avoid AlgorithmDetails public interface

1fb2951

Signed-off-by: Riccardo Schirone <[email protected]>

pkg/signature: avoid simple getters in algorithmDetails

d51867d

Signed-off-by: Riccardo Schirone <[email protected]>

Hayden-IO previously approved these changes Feb 3, 2025

View reviewed changes

bobcallaway reviewed Feb 3, 2025

View reviewed changes

go.mod Outdated Show resolved Hide resolved

Update protobuf-specs to v0.4.0

f627228

Signed-off-by: Riccardo Schirone <[email protected]>

ret2libc dismissed Hayden-IO’s stale review via f627228 February 4, 2025 14:12

bobcallaway previously approved these changes Feb 4, 2025

View reviewed changes

Update protobuf-specs to v0.4.0 everywhere

11b0d5a

Signed-off-by: Riccardo Schirone <[email protected]>

ret2libc dismissed bobcallaway’s stale review via 11b0d5a February 4, 2025 14:39

test/cliplugin/localkms: update protobuf-specs to v0.4.0

642bc87

Signed-off-by: Riccardo Schirone <[email protected]>

Hayden-IO approved these changes Feb 4, 2025

View reviewed changes

Hayden-IO merged commit 63abeeb into sigstore:main Feb 4, 2025

Conversation

tetsuo-cpp commented Jan 18, 2024

Summary

Release Note

Documentation

Uh oh!

tetsuo-cpp commented Jan 18, 2024

Uh oh!

tetsuo-cpp Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tetsuo-cpp Jan 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tetsuo-cpp commented Jan 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ret2libc commented Jan 19, 2024

Uh oh!

woodruffw commented Jan 19, 2024

Uh oh!

Uh oh!

Uh oh!

Hayden-IO Jan 19, 2024

Choose a reason for hiding this comment

Uh oh!

tetsuo-cpp Jan 20, 2024

Choose a reason for hiding this comment

Uh oh!

Hayden-IO Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Hayden-IO Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

bobcallaway Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tetsuo-cpp Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hayden-IO left a comment

Choose a reason for hiding this comment

Uh oh!

Hayden-IO Jan 29, 2024

Choose a reason for hiding this comment

Uh oh!

tetsuo-cpp Jan 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Hayden-IO commented Jan 30, 2024

Uh oh!

kommendorkapten commented Jan 30, 2024

Uh oh!

tetsuo-cpp commented Jan 30, 2024

Uh oh!

kommendorkapten commented Jan 30, 2024

Uh oh!

tetsuo-cpp commented Jan 30, 2024

Uh oh!

ret2libc commented Feb 19, 2024

Uh oh!

Uh oh!

ret2libc left a comment

tetsuo-cpp Jan 18, 2024 •

edited

Loading

bobcallaway Jan 24, 2024 •

edited

Loading

tetsuo-cpp Jan 25, 2024 •

edited

Loading