go-tuf icon indicating copy to clipboard operation
go-tuf copied to clipboard

feat: Implement full support for ECDSA and RSA keys

Open toby-jn opened this issue 3 years ago • 3 comments

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)
  • [x] 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:

  • [x] Tests have been added for the bug fix or new feature
  • [x] Docs have been added for the bug fix or new feature

toby-jn avatar Apr 16 '22 20:04 toby-jn

any chance of getting a review of this?

toby-jn avatar Apr 27 '22 15:04 toby-jn

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.

toby-jn avatar Apr 27 '22 17:04 toby-jn

Fixes issue #223

trishankatdatadog avatar Aug 03 '22 15:08 trishankatdatadog

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

trishankatdatadog avatar Aug 15 '22 20:08 trishankatdatadog

I'm closing in favor of #357

Thanks for kicking this off, @toby-jn 😄

znewman01 avatar Sep 06 '22 11:09 znewman01