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

asraa · 2022-08-17T19:31:56Z

Please fill in the fields below to submit a pull request. The more information that is provided, the better.

Fixes #223
Release Notes:

breaking change: ECDSA public key verification now use TUF compliant PEM-encoding keys. For backwards compatibility with hex-encoded keys verification, client libraries must import pkg/deprecated
feat: Adds support for ECDSA and RSA signers
feat: Repository CLI command allows a --scheme flag to specify key scheme.

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:

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.
Completes support for the RSA signer and fixes public key encoding.

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

cc @toby-jn: The only changes I made was adding deprecation support and updating your branch!

trishankatdatadog · 2022-08-17T19:54:30Z

So does this supersede #270 ?

asraa · 2022-08-17T20:05:33Z

So does this supersede #270 ?

Yes! It's all the changes there with backwards compatibility for just ECDSA verification: i.e. if a current root (sigstore) is using hex-encoded keys, then they will still be able to verify (no hex-encoded signers supported). Adds testing for making sure deprecation still verifies at the key and repo testing level.

asraa · 2022-08-17T20:05:57Z

I can also split out just the ECDSA changes. Hoping to get that in ASAP since the next sigstore TUF root to rotate those out relies on it.

toby-jn · 2022-08-18T08:48:45Z

Great thanks, LGTM. I did start trying to rebase my changes onto master over the weekend to make these updates, but there were some conflicts that I needed to go read up on and then real life got in the way, so thanks for taking this over.

trishankatdatadog · 2022-08-18T14:23:20Z

Thanks very much for all your hard work, Toby! Sorry I didn't get around to reviewing it in time: swamped by work, too...

trishankatdatadog

Good stuff, thanks @asraa (and @toby-jn)!

Sorry for my late review, but a few comments...

cmd/tuf/gen_key.go

pkg/keys/ecdsa.go

pkg/keys/ecdsa_test.go

pkg/keys/rsa.go

pkg/keys/rsa_test.go

repo.go

asraa · 2022-08-23T18:29:35Z

@theupdateframework/go-tuf-maintainers could I please get one more review on this? really need this

trishankatdatadog · 2022-08-23T18:55:57Z

@theupdateframework/go-tuf-maintainers could I please get one more review on this? really need this

Yes, sorry, was on PTO, will look tonight

asraa · 2022-08-23T20:38:22Z

No worries at all -- didn't realize that! Feel free to do it tomorrow, I hope you had some rest :)

trishankatdatadog

Looks great to me, thanks very much! Just a few comments...

cmd/tuf/gen_key.go

pkg/deprecated/deprecated_repo_test.go

pkg/keys/ecdsa.go

pkg/keys/rsa.go

pkg/keys/ecdsa.go

pkg/keys/rsa.go

trishankatdatadog

Thanks very much, Asra!

So just to double-check: what's the worst that can happen re:#363? We are not able to accept malformed/unusable public keys after unmarshaling, correct?

After this is confirmed, I'm happy to attach my approval ASAP tomorrow. Thanks!

cmd/tuf/gen_key.go

asraa · 2022-08-25T14:29:13Z

So just to double-check: what's the worst that can happen re:#363? We are not able to accept malformed/unusable public keys after unmarshaling, correct?

Good question, and I'll attach my reply to the issue as well. Currently, we use those functions after we create a verifier in functionality like keys.GetVerifier
or a signer through keys.GetSigner

go-tuf/pkg/keys/keys.go

Lines 57 to 69 in 355e39c

    
           func GetVerifier(key *data.PublicKey) (Verifier, error) { 
        
           	st, ok := VerifierMap.Load(key.Type) 
        
           	if !ok { 
        
           		return nil, ErrInvalidKey 
        
           	} 
        
           	s := st.(func() Verifier)() 
        
           	if err := s.UnmarshalPublicKey(key); err != nil { 
        
           		return nil, fmt.Errorf("tuf: error unmarshalling key: %w", err) 
        
           	} 
        
           	return s, nil 
        
           } 
        
           func GetSigner(key *data.PrivateKey) (Signer, error) {

So as long as they're verified during UnmarshalPublicKey or UnmarshalPrivateKey (which now they are), then we will always have a "well-formed" enough public key to Marshal or get the PublicData() from.

In case someone accidentally corrupts those keys in the key store or TUF metadata, then we would return errors like tuf: error unmarshalling key: on loading them in as Verifier or Signer.

So worst case, yes, the key becomes un-usable on load.

That being said, it would always be good practice to re-validate the keys upon marshalling even if they were loaded in a good state, just in case. (for the issue)

trishankatdatadog

Well-done, @asraa (and @toby-jn), thank you very much! Let's go 🚀

asraa · 2022-09-01T15:18:36Z

@znewman01 would you be able to PTAL? I really need this for sigstore ga!

znewman01 · 2022-09-01T16:23:34Z

ACK can look today (sorry, thought review was blocked for some reason)

znewman01

Do you need to check "breaking change" for this PR?

I left some little comments, nice to resolve but not blocking.

My final concern (again, not blocking) has to do with deprecation. I think it's okay to immediately start producing keys using the new encoding from this library, but for repos that already exist, old clients will be around for a long time. Do we have a migration story there? I guess just wait until hopefully all the old clients are gone and then cut over?

repo.go

pkg/keys/ecdsa.go

asraa · 2022-09-02T14:26:36Z

but for repos that already exist, old clients will be around for a long time. Do we have a migration story there? I guess just wait until hopefully all the old clients are gone and then cut over?

Right! So they can still use pkg/deprecated for verification, and can still construct . To migrate, they need to re-sign using the new keys, like we are doing in Sigstore. I could add the deprecated signer to pkg/deprecated, which would allow them to still Sign with the keys in their key store with the deprecated format. I think this is worth it so I'll add.

EDIT: I just realized that adding the deprecated signer AND verifier into the same package for compatibility is problematic for users who need to make the transition. E.g. I don't want the signer to ONLY be the deprecated version. So: the deprecated package should actually handle EITHER OR format during the transition. WDYT?

EDIT on EDIT: Ecdsa Signers were never supported before. So go-tuf only supported ecdsa verification with bad keys. So I think we're in the clear as is.

pkg/deprecated/ecdsa.go

znewman01 · 2022-09-02T15:02:23Z

To migrate, they need to re-sign using the new keys, like we are doing in Sigstore.

Won't this immediately break every old client?

I could add the deprecated signer to pkg/deprecated[...]I think this is worth it so I'll add.

+1

EDIT: I just realized that adding the deprecated signer AND verifier into the same package for compatibility is problematic for users who need to make the transition. E.g. I don't want the signer to ONLY be the deprecated version. So: the deprecated package should actually handle EITHER OR format during the transition. WDYT?

I'm not sure I'm parsing. I think there are the following possible behaviors:

For parsing:

(default) only accept the new format
(configurable) accept both old and new formats
(could be an option, but I don't think we should add it) accept only the old format

This can probably be globally configurable, via the init() trick.

For writing new keys:

(default) write keys in the new format
(configurable) write keys in the old format

IMO we'd want to configure this on a key-by-key basis, rather than globally.

It'd be nice if we could do both, but I don't think that's possible.

asraa · 2022-09-02T15:42:33Z

Won't this immediately break every old client?

Those old clients would need to import pkg/deprecated into their binaries. Originally I had deprecated support as a default, but then we opted to move it to an explicit package.

+1

ECDSA Signing was never supported FYI. I just realized this as I was implementing. So I don't think it breaks old clients. They would have already implemented their own signing logic (as we did).

(configurable) write keys in the old format

Don't think we need to opt to do this anymore, since it was never supported in the first place. go-tuf only had verification support for keys, which remains on import of pkg/deprecated

D: sorry this is confusing!

For parsing, the PR implements default new format, configurable on both. I agree it would be nice to do on a per-key basis, but the problem is keys are identified via the SAME scheme type string.

trishankatdatadog · 2022-09-02T15:47:40Z

Those old clients would need to import pkg/deprecated into their binaries. Originally I had deprecated support as a default, but then we opted to move it to an explicit package.

Hold on: old clients that do not import pkg/deprecated will not break so long as the server keeps producing these deprecated-style public keys.

asraa · 2022-09-02T15:49:57Z

Hold on: old clients that do not import pkg/deprecated will not break so long as the server keeps producing these deprecated-style public keys.

Maybe we're confused! Old clients that use old go-tuf certainly won't break because they're using an old go-tuf version. Clients that update their go-tuf dependency will break, because now ECDSA verifier expects TUF-compliant keys. Unless they import that deprecated package that understands both. RIght?

trishankatdatadog · 2022-09-02T15:59:30Z

Maybe we're confused! Old clients that use old go-tuf certainly won't break because they're using an old go-tuf version. Clients that update their go-tuf dependency will break, because now ECDSA verifier expects TUF-compliant keys. Unless they import that deprecated package that understands both. RIght?

Oui. Sorry, just need to be super-clear here about what we meant about "old clients" here and so on. Thanks for the clarification!

znewman01 · 2022-09-02T21:19:28Z

IIUC, serializing keys was never done directly by go-tuf. If that's true, then I am okay with

(I'm a little confused by the discussion about signing and verifying—the signature format was always okay, right? We're just talking about signers/verifiers because that's the part of the codebase where key serialization happened?)

To migrate, they need to re-sign using the new keys, like we are doing in Sigstore.

Won't this immediately break every old client?

Those old clients would need to import pkg/deprecated into their binaries. Originally I had deprecated support as a default, but then we opted to move it to an explicit package.

Let me clarify: won't that immediately break every deployed client that's still using the old version of go-tuf?

I'm still fuzzy on the migration story. "Wait a long time before cutting over" isn't a great solution IMO; you typically don't want to break clients no matter how old.

I guess an alternative would be to replace the keys with some other key type (say, RSA for the sake of example) whose format was never affected. Then, we can distribute a new 5.root.json with only RSA signatures (which cuts off the historical ECDSA-hex signatures, and then we could actually turn off the pkg/deprecated flag for parsing new clients. That brings the repo into compliance with the TUF spec (err, as long as you ignore everything before 5.root.json).

pkg/deprecated/ecdsa.go

asraa · 2022-09-06T15:56:42Z

(I'm a little confused by the discussion about signing and verifying—the signature format was always okay, right? We're just talking about signers/verifiers because that's the part of the codebase where key serialization happened?)

Correct, signature format was always correct, it's just the way that the public key was serialized in the root metadata or how the private key was stored in the repo keystore.

Let me clarify: won't that immediately break every deployed client that's still using the old version of go-tuf?

If they don't update their go-tuf version, they are unaffected: hex ecdsa keys will be the implementation they have in that go-tuf version. Maybe you're asking, if someone is using an old version old client, and the repository owner adds new ECDSA keys their metadata, will the old deployed client break? Yes, but that's akin to using a new feature, right? I'm not sure there's a way to get compatibility here.

you typically don't want to break clients no matter how old.

I'd say specification deviation and lack of cross-language compatibility is one potential reason :) My feeling though is that this ONLY affected go-tuf clients who were implementing their own ECDSA signers that were compatible with the bad ECDSA verifiers in go-tuf, so they are already aware of managing this implementation.

Then, we can distribute a new 5.root.json with only RSA signatures (which cuts off the historical ECDSA-hex signatures, and then we could actually turn off the pkg/deprecated flag for parsing new clients.

I see. The current plan would have already supported this, since the 5.root.json would have 5 old ECDSA signatures and 5 new ECDSA signatures. Old clients would think that the new keys are bogus, but the old keys are still there as is. There is a danger of failure on parsing though, rather than ignoring.

We can't change key material because that would require overwriting the keyholders existing keys: they would not longer be able to sign with ECDSA

znewman01 · 2022-09-07T15:40:45Z

Note: use feat!: for a breaking change

asraa · 2022-09-07T16:36:34Z

When I have approval, I'll squash mine and Toby's commits to one and fix the commit to have feat!

asraa · 2022-09-07T17:45:00Z

Squashed, and commit message fixed, added DCO to fix Toby's old DCO issue

…ecification * 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 <asraa@google.com> Co-authored-by: Toby Bristow <toby.bristow@qush.com> Signed-off-by: Asra Ali <asraa@google.com>

Signed-off-by: Asra Ali <asraa@google.com>

trishankatdatadog reviewed Aug 18, 2022

View reviewed changes

asraa mentioned this pull request Aug 19, 2022

Should SignMessage guard against large metadata? #358

Closed

asraa requested review from joshuagl and znewman01 August 23, 2022 18:29

trishankatdatadog reviewed Aug 24, 2022

View reviewed changes

trishankatdatadog mentioned this pull request Aug 24, 2022

test: Fuzzing go 1.18 #365

Closed

5 tasks

trishankatdatadog reviewed Aug 25, 2022

View reviewed changes

cmd/tuf/gen_key.go Outdated Show resolved Hide resolved

trishankatdatadog previously approved these changes Aug 25, 2022

View reviewed changes

trishankatdatadog requested a review from mnm678 August 25, 2022 14:47

znewman01 previously approved these changes Sep 1, 2022

View reviewed changes

repo.go Show resolved Hide resolved

pkg/keys/ecdsa.go Outdated Show resolved Hide resolved

znewman01 reviewed Sep 2, 2022

View reviewed changes

pkg/deprecated/ecdsa.go Outdated Show resolved Hide resolved

asraa dismissed stale reviews from znewman01 and trishankatdatadog via 25dbbe4 September 2, 2022 15:51

znewman01 reviewed Sep 2, 2022

View reviewed changes

pkg/deprecated/ecdsa.go Outdated Show resolved Hide resolved

pkg/deprecated/ecdsa.go Outdated Show resolved Hide resolved

pkg/deprecated/ecdsa.go Outdated Show resolved Hide resolved

znewman01 mentioned this pull request Sep 6, 2022

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

Closed

5 tasks

asraa force-pushed the ecdsa-key branch from ffec83c to df003d9 Compare September 7, 2022 17:44

asraa force-pushed the ecdsa-key branch from df003d9 to 2db153d Compare September 7, 2022 17:45

trishankatdatadog previously approved these changes Sep 7, 2022

View reviewed changes

znewman01 previously approved these changes Sep 7, 2022

View reviewed changes

add comment

1736e3c

Signed-off-by: Asra Ali <asraa@google.com>

asraa dismissed stale reviews from znewman01 and trishankatdatadog via 1736e3c September 7, 2022 19:18

znewman01 approved these changes Sep 7, 2022

View reviewed changes

trishankatdatadog approved these changes Sep 7, 2022

View reviewed changes

asraa enabled auto-merge (squash) September 7, 2022 20:04

asraa merged commit 61872a3 into theupdateframework:master Sep 7, 2022

znewman01 mentioned this pull request Sep 28, 2022

Update Root and Targets sigstore/root-signing#410

Closed

ianlewis mentioned this pull request Oct 27, 2022

error updating to TUF remote mirror: tuf: invalid key slsa-framework/slsa-verifier#325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

asraa commented Aug 17, 2022 •

edited

trishankatdatadog commented Aug 17, 2022

asraa commented Aug 17, 2022

asraa commented Aug 17, 2022

toby-jn commented Aug 18, 2022

trishankatdatadog commented Aug 18, 2022

trishankatdatadog left a comment

asraa commented Aug 23, 2022

trishankatdatadog commented Aug 23, 2022

asraa commented Aug 23, 2022

trishankatdatadog left a comment

trishankatdatadog left a comment •

edited

asraa commented Aug 25, 2022

trishankatdatadog left a comment

asraa commented Sep 1, 2022

znewman01 commented Sep 1, 2022

znewman01 left a comment

asraa commented Sep 2, 2022 •

edited

znewman01 commented Sep 2, 2022

asraa commented Sep 2, 2022 •

edited

trishankatdatadog commented Sep 2, 2022

asraa commented Sep 2, 2022

trishankatdatadog commented Sep 2, 2022

znewman01 commented Sep 2, 2022

asraa commented Sep 6, 2022 •

edited

znewman01 commented Sep 7, 2022

asraa commented Sep 7, 2022

asraa commented Sep 7, 2022

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

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

Conversation

asraa commented Aug 17, 2022 • edited

trishankatdatadog commented Aug 17, 2022

asraa commented Aug 17, 2022

asraa commented Aug 17, 2022

toby-jn commented Aug 18, 2022

trishankatdatadog commented Aug 18, 2022

trishankatdatadog left a comment

Choose a reason for hiding this comment

asraa commented Aug 23, 2022

trishankatdatadog commented Aug 23, 2022

asraa commented Aug 23, 2022

trishankatdatadog left a comment

Choose a reason for hiding this comment

trishankatdatadog left a comment • edited

Choose a reason for hiding this comment

asraa commented Aug 25, 2022

trishankatdatadog left a comment

Choose a reason for hiding this comment

asraa commented Sep 1, 2022

znewman01 commented Sep 1, 2022

znewman01 left a comment

Choose a reason for hiding this comment

asraa commented Sep 2, 2022 • edited

znewman01 commented Sep 2, 2022

asraa commented Sep 2, 2022 • edited

trishankatdatadog commented Sep 2, 2022

asraa commented Sep 2, 2022

trishankatdatadog commented Sep 2, 2022

znewman01 commented Sep 2, 2022

asraa commented Sep 6, 2022 • edited

znewman01 commented Sep 7, 2022

asraa commented Sep 7, 2022

asraa commented Sep 7, 2022

asraa commented Aug 17, 2022 •

edited

trishankatdatadog left a comment •

edited

asraa commented Sep 2, 2022 •

edited

asraa commented Sep 2, 2022 •

edited

asraa commented Sep 6, 2022 •

edited