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

Added a new ecdsa key that uses the new type ecdsa. #755

Merged
merged 2 commits into from Mar 22, 2024

Conversation

kommendorkapten
Copy link
Contributor

In the TUF spec v1.0.32 (released 2023-03-02) the key type was updated for ecdsa keys from ecdsa-sha2-nistp256.
This PR introduces the new type and keeps support for the older key type.

Issue #, if available: #754

Description of changes:

In the TUF spec v1.0.32 (released 2023-03-02) the key type was updated for ecdsa keys from ecdsa-sha2-nistp256.
This PR introduces the new type and keeps support for the older key type.

I first tried with #[serde(alias = "ecdsa-sha2-nistp256")] as proposed by @jku but I never got it to work.

In the TUF spec v1.0.32 (released 2023-03-02) the key type was updated
for ecdsa keys from `ecdsa-sha2-nistp256`.
This PR introduces the new type and keeps support for the older key type.

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
@haydentherapper
Copy link

From the Sigstore community, thanks so much for this PR. We use awslabs/tough in our Rust client, so getting this merged in will help unblock Sigstore users.

@jku
Copy link

jku commented Mar 14, 2024

I first tried with #[serde(alias = "ecdsa-sha2-nistp256")] as proposed by @jku but I never got it to work.

👍

I think the code uses the Key struct when canonicalizing the signed content: my suggestion would then be wrong (as you must have the original keytype in the canonicalized content).

I'm sure there is a way to store the original keytype and use that when serialising even if using a single struct but I'm not sure it would be simpler than your suggestion

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
@kommendorkapten
Copy link
Contributor Author

Some local testing with the soon to be deployed TUF root for sigstore

kommendorkapten@m1m14:~/git/tough % ./target/debug/tuftool download out --root ~/git/root-signing/repository/repository/5.root.json -t http://localhost:8081/targets -m http://localhost:8081
Downloading targets to "out"
	-> fulcio_v1.crt.pem
	-> trusted_root.json
	-> fulcio_intermediate_v1.crt.pem
	-> artifact.pub
	-> rekor.pub
	-> ctfe.pub
	-> ctfe_2022.pub
	-> fulcio.crt.pem
kommendorkapten@m1m14:~/git/tough % git status
On branch new-ecdsa-key-type

Switching to default branch:

kommendorkapten@m1m14:~/git/tough % ./target/debug/tuftool download out --root ~/git/root-signing/repository/repository/5.root.json -t http://localhost:8081/targets -m http://localhost:8081
Failed to load repository: Failed to parse root metadata: unknown variant `ecdsa`, expected one of `rsa`, `ed25519`, `ecdsa-sha2-nistp256` at line 9 column 22
kommendorkapten@m1m14:~/git/tough % git status
On branch develop

@haydentherapper
Copy link

@webern Would you be able to take a look at this? Thanks!

@webern
Copy link
Contributor

webern commented Mar 18, 2024

I don't quite understand. Is the issue that "ecdsa" is now a valid "keytype" when it used to require the "keytype" to be "ecdsa-sha2-nistp256"?

Serde does have alias for cases like this, but in that case we would not know which way to serialize so maybe this is the best approach. Can you confirm my understanding of the issue?

Thanks.

@jku
Copy link

jku commented Mar 18, 2024

I don't quite understand. Is the issue that "ecdsa" is now a valid "keytype" when it used to require the "keytype" to be "ecdsa-sha2-nistp256"?

Yes. client should should accept both for backwards compat but "ecdsa" is considered correct.

Serde does have alias for cases like this, but in that case we would not know which way to serialize so maybe this is the best approach. Can you confirm my understanding of the issue?

Agreed: alias would work except if the serialized form is used for anything (because the canonicalized form would then be different and so signatures wouldn't match).

@astoycos
Copy link

Another data point here we're hitting this in the https://github.com/bpfman/bpfman via sigstore/sigstore-rs#338 and it's currently a build blocker for us at :main

@haydentherapper
Copy link

Thanks all!

@webern webern merged commit 43d589a into awslabs:develop Mar 22, 2024
7 checks passed
@kommendorkapten kommendorkapten deleted the new-ecdsa-key-type branch March 25, 2024 06:37
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