feat: Implement full support for ECDSA and RSA keys #270

toby-jn · 2022-04-16T20:44:05Z

Release Notes:

Implements full support for ECDSA and RSA key types.
When generating a key with the gen-key sub command, the --type flag can be specified to select a key type (according the the types in https://theupdateframework.github.io/specification/latest/#keytype). Defaults to ed25519 keys.

Types of changes:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of the changes being introduced by the pull request:

Previously support for ECDSA and RSA keys was only partially implemented:

Only the verifier/public key type was implemented for ECDSA.
ECDSA public keys were parsed as the hex encoding of the raw point serialised as ASN.1 . The TUF specification states that they should be encoded as PEM https://theupdateframework.github.io/specification/latest/#keyval-ecdsa-public.
The RSA signer was not fully implemented, the marshal and unmarshal methods returned not implemented errors.
The RSA public key mixed up the PEM encoding, it encoded as PKIX, but then set the header to RSA PUBLIC KEY which is used for PKCS1 encoding. While the spec doesn't specify which encoding should be used (and just specifies "PEM"), the python reference implementation uses PKIX for both ECDSA and RSA keys.
The RSA verifier attempted to parse the public key as PKCS1 and PKIX. While the spec does not make this clear, the python implementation only uses PKIX.
The gen-key subcommand does not allow key types to be specified, added a --type flag to allow the user to select the key type to be generated.

As well as the above fixes, some additional improvements to the library were made:

Defined new go types for KeyType, KeyScheme and HashAlgorithm, rather than just using strings, and fixed a few places where the key type and scheme had been mixed up.
Added a new data.PKIXPublicKey type which abstracts away a lot of the boiler plate for marshalling/unmarshalling public keys.

I opted not to maintain backwards compatibility as there is currently no implementation to generate ECDSA or RSA keys, and the go-tuf implementation wouldn't have worked with keys serialised according the the TUF specification, so there didn't seem any benefit to maintaining this functionality.

Please verify and check that the pull request fulfills the following requirements:

Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

toby-jn · 2022-04-27T15:57:40Z

any chance of getting a review of this?

mnm678

This looks great. My only concern is the backwards compatibility issue. In the long term I think this is what we need to do (use the spec compliant key types). We could just do this patch as part of a major release. What do others think? @asraa @trishankatdatadog

toby-jn · 2022-04-27T17:49:23Z

Making it backwards compatible with what go-tuf was doing before wouldn't be that difficult.. however the old implementation doesn't follow the spec, and is incompatible with py-tuf and it also doesn't have the necessary functions to actually generate ecdsa/rsa keys, so for someone to have a compatibility issue here they'd have need to have done quite a lot of work outside of go-tuf to make keys in the right format.

asraa

One problem I see with the ECDSA keys is that if you're using an existing TUF root with ecdsa keys, your root key IDs won't match on update. This is a big problem when you're trying to sign the next root. I think we have to make this backwards compatible, maybe with a flag or an option

trishankatdatadog · 2022-08-03T15:32:44Z

Fixes issue #223

asraa

Thanks! Hoping to revive this so we can get go-tuf python compliant in the next release.

I just have one proposition on supporting the old key types for a short period of time so users can have a migration plan away from it.

Let me know if you need any help! I'm happy to contribute.

asraa · 2022-08-11T20:32:08Z

cmd/tuf/gen_key.go

@@ -23,23 +25,35 @@ form of TUF_{{ROLE}}_PASSPHRASE

 Options:
  --expires=<days>   Set the root metadata file to expire <days> days from now.
+  --type=<type>      Set the type of key to generate [default: ed25519].


could you please document the other key type possibilities? it's a bit hard to discern in the code because they are consts.

asraa · 2022-08-15T19:54:16Z

pkg/keys/ecdsa.go

 	return p.key
 }

-func (p *p256Verifier) UnmarshalPublicKey(key *data.PublicKey) error {
+func (p *ecdsaVerifier) UnmarshalPublicKey(key *data.PublicKey) error {


Question: To keep backwards compatibility with existing roots (at least for 1-2 releases), could we do something like rename the old ecdsaVerifier to deprecatedEcdsaVerifier, and attempt to json.Unmarshal into that one?

I'm hoping to be able to still support old verifiers.

For example, if TUF attempts to UnmarshalPublicKey on an old ecdsa key, the resulting object would return the deprecatd Public() and Marshal() as the received data.

asraa · 2022-08-15T19:55:16Z

pkg/keys/ecdsa_test.go

+
+var _ = Suite(EcdsaSuite{})
+
+func (EcdsaSuite) TestSignVerify(c *C) {


if you go with backwards compatibility, could you add a test with hard-coded deprecated key material, and make sure that properties like PublicData() and Public() and Verify() still work?

trishankatdatadog · 2022-08-15T20:16:58Z

Damn, I got busy, sorry. Let me look ASAP, maybe tonight.

znewman01 · 2022-09-06T11:13:23Z

I'm closing in favor of #357

Thanks for kicking this off, @toby-jn 😄

…357) * * fix!: ECDSA verifiers now expect PEM-encoded public keys per TUF specification * feat: ECDSA signers are now implemented * feat: RSA verifiers and signers are implemented BREAKING CHANGE: ECDSA verifiers expect PEM-encoded public keys. If you rely on previous behavior of hex-encoded public keys for verifiers, then you must import pkg/deprecated/set_ecdsa that will allow a fallback for hex-encoded ECDSA keys. Co-authored-by: Asra Ali <[email protected]> Co-authored-by: Toby Bristow <[email protected]> Signed-off-by: Asra Ali <[email protected]> * add comment Signed-off-by: Asra Ali <[email protected]> Signed-off-by: Asra Ali <[email protected]> Co-authored-by: Toby Bristow <[email protected]>

toby-jn added 4 commits April 16, 2022 21:14

add support for ecdsa and rsa keys

731bc3e

Make specific types for key types, schemes and hash algorithms

05a6b19

add type flag to gen-key command

4a565fc

test fixes

488fb52

toby-jn mentioned this pull request Apr 16, 2022

ecdsa keyval.public format is hex-encoded DER, should be PEM #223

Closed

mnm678 reviewed Apr 27, 2022

View reviewed changes

asraa reviewed May 6, 2022

View reviewed changes

asraa mentioned this pull request Jul 21, 2022

bug: python-tuf incompatibility because of hex-encoded ECDSA keys sigstore/root-signing#329

Closed

trishankatdatadog self-requested a review August 3, 2022 15:25

asraa reviewed Aug 15, 2022

View reviewed changes

trishankatdatadog mentioned this pull request Aug 17, 2022

feat: Support ecdsa and RSA keys (#270 with backwards compatibility) #357

Merged

5 tasks

trishankatdatadog removed their request for review August 18, 2022 01:55

znewman01 closed this Sep 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement full support for ECDSA and RSA keys #270

feat: Implement full support for ECDSA and RSA keys #270

toby-jn commented Apr 16, 2022 •

edited

Loading

toby-jn commented Apr 27, 2022

mnm678 left a comment

toby-jn commented Apr 27, 2022

asraa left a comment

trishankatdatadog commented Aug 3, 2022

asraa left a comment

asraa Aug 11, 2022

asraa Aug 15, 2022

asraa Aug 15, 2022

trishankatdatadog commented Aug 15, 2022

znewman01 commented Sep 6, 2022


		var _ = Suite(EcdsaSuite{})

		func (EcdsaSuite) TestSignVerify(c *C) {

feat: Implement full support for ECDSA and RSA keys #270

feat: Implement full support for ECDSA and RSA keys #270

Conversation

toby-jn commented Apr 16, 2022 • edited Loading

toby-jn commented Apr 27, 2022

mnm678 left a comment

Choose a reason for hiding this comment

toby-jn commented Apr 27, 2022

asraa left a comment

Choose a reason for hiding this comment

trishankatdatadog commented Aug 3, 2022

asraa left a comment

Choose a reason for hiding this comment

asraa Aug 11, 2022

Choose a reason for hiding this comment

asraa Aug 15, 2022

Choose a reason for hiding this comment

asraa Aug 15, 2022

Choose a reason for hiding this comment

trishankatdatadog commented Aug 15, 2022

znewman01 commented Sep 6, 2022

toby-jn commented Apr 16, 2022 •

edited

Loading