Refactor keys to allow more key types and add RSA key verifier

[asraa]

Local tests passes, still working on interop tests and double-checking for clean-up/small changes

This change makes it more friendly to add new keytypes:

• There are now two data types, data.Key and data.PrivateKey representing the json data for public and private keys
• There are two interfaces Signer and Verifier in the keys package that define conversions between the data formats and methods that are needed by the other modules
• There is a map KeyMap that registers a key type with the Signer/Verifier implementation. You don't need both to register in the key map (e.g. go-tuf only verifies ecdsa, does not support signing)

Ah, another pain point: maybe there should be a file moving refactor so that the library code is in pkg/. these a keys/ package also conflicts with the keys/ folder that go-tuf creates for storing keys....

[coveralls]

Pull Request Test Coverage Report for Build 1221983175

• 108 of 188 (57.45%) changed or added relevant lines in 7 files are covered.
• 171 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.3%) to 69.178%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
local_store.go	19	20	95.0%
verify/db.go	9	11	81.82%
repo.go	9	12	75.0%
pkg/keys/keys.go	6	20	30.0%
pkg/keys/ed25519.go	57	80	71.25%
pkg/keys/ecdsa.go	3	40	7.5%

Files with Coverage Reduction	New Missed Lines	%
verify/verify.go	1	71.08%
client/delegations.go	8	78.67%
repo.go	162	72.1%

Totals
Change from base Build 1175464948:	-0.3%
Covered Lines:	1809
Relevant Lines:	2615

---

💛 - Coveralls

[asraa]

addressed some comments

[d-niu]

Overall, I really like the generalized approach to key types and separating the interfaces for signing/verification accordingly.

My biggest question is around the structure of the Signer interface + implementing a signer struct for each key. Right now, it's not very flexible for generating signatures (or verifications) in a kms.

Ideally, I would like to use as much of the same code to create keys with go's crypto package and any type of external key gen system. Right now, I'm thinking that creating keys, signing, verifying, and getting public keys should all be interface methods of kms. I'm not too sure what the best approach is here, but I think making the Signer and Verifier more abstract or even changing how we store things in the Key Map would work.

Here's the general structure of what I was thinking for key management systems. Though I have to admit, I wasn't really writing this with the current way keys are generating in memory in mind :/

[trishankatdatadog]

I think I am ok with this. I understand @d-niu's concerns about extending this API to abstract away things like where the signing operation is actually being done, but I think this can be fixed later.

Can you finish moving the files for keys under keys/ to pkg?

[asraa]

Right now, I'm thinking that creating keys, signing, verifying, and getting public keys should all be interface methods of kms. I'm not too sure what the best approach is here, but I think making the Signer and Verifier more abstract or even changing how we store things in the Key Map would work.

Hm I think Signer and Verify should be separate (although they can be the same object in the case of a KMS client). Looking at your code, the part that probably won't fit is the crypto.Signer operation, since I don't think KMS easily supports crypto.Signer interface without a lot of finagling. Do you know what the Sign parameters should look like?

Edit: Wanna chat about the way things are stored in the Key Map? The Key Map is just meant to translate "type name" to an "empty type implementation". UnmarshalKey is meant to take the public key data from the JSON (probably including key resource ID) and try to instantiate the kms client. A method (NOT in the interface), like GenerateHashiVaultKey would look like the func HVClient(keyResourceID string) (KMSClient, error) you have. Does that make sense?

[asraa]

OK simplified the interface even more.

[asraa]

I might be missing something, but why can't GetVerifier() be a method of the key?

GetVerifier as a method data.Key? because it would introduce a circular dependency. (data.Key shouldn't know about the key implementations)

Edit: If you were looking at that test -- the purpose of GetVerifier is to take the JSON data.Key found in metadata and translate that to a verifier. I was testing basically testing marshalling to JSON and GetVerifier'ing back. So you create a key and sign and marshal as the repo builder, and then GetVerifier and verify as the client.

[trishankatdatadog]

Happy to unblock this for now, we can refine API if need be later

[trishankatdatadog]

Oh, one more thing: we need RSA support, no?

[asraa]

Sounds good, we'll need to work how KMS signers will deal with crypto.Signer. (Probably we'll just define the Sign(...) method in the interface). Sigstore has a massive workaround for it, but we should keep it simple.

[asraa]

Oh, one more thing: we need RSA support, no?

I have a local follow-up PR!

[trishankatdatadog]

Who else should we have review approve this? @hosseinsia @joshuagl @mnm678? Should not both be DD

[d-niu]

Edit: Wanna chat about the way things are stored in the Key Map? The Key Map is just meant to translate "type name" to an "empty type implementation". UnmarshalKey is meant to take the public key data from the JSON (probably including key resource ID) and try to instantiate the kms client. A method (NOT in the interface), like GenerateHashiVaultKey would look like the func HVClient(keyResourceID string) (KMSClient, error) you have. Does that make sense?

So, generating different types of keys, signing, and verifying in kms should not necessarily be under a separate kms interface. Instead, the connection to a kms client like hashicorp vault should be generated when you use a key from the key map and realize that it has some other attributes during UnmarshalKey, for instance.

[asraa]

Instead, the connection to a kms client like hashicorp vault should be generated when you use a key from the key map and realize that it has some other attributes during UnmarshalKey, for instance.

Maybe the constructors held in the map should have an Context or Params object?

[d-niu]

Maybe the constructors held in the map should have an Context or Params object?

For sure. Would context/params just hold something to indicate whether its locally generated or in a kms though? We probably won't store kms access tokens or paths in these params.

[asraa]

Added RSA key implementation.

[hosseinsia]

Great job! Started reviewing. Some minor comments so far.

[hosseinsia]

Fantastic work!
LGTM!

[hosseinsia]

LGTM

[trishankatdatadog]

Thank you both very much, @asraa and @hosseinsia!

I would strongly encourage @joshuagl to review and approve to ensure there is no collusion 🙂

[trishankatdatadog]

Approving in order to unblock. Great work, Asra! 🎉

[tamird]

This PR added a dependency on github.com/pkg/errors. I've sent #166 to replace the dependency with standard library functions.