Implemented tough-kms to include AWS KMS as a new keysource #198

srgothi92 · 2020-07-27T17:21:09Z

Issue #, if available:
N/A

Description of changes:
-tough-kms implements the "KeySource" trait found in tough
-Unit test cases are added for tough-kms utilizing rusoto_mock
-Integration Tests are added in tuftool, to test 3 keysource: tough-kms, tough-ssm, local key.
-To run integration tests use feature flag "integ" while running test

Testing Done:
Ran Unit tests
Ran Integration Tests
Manually created root.json and used kms key for signing.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

zmrow

Nice work!

I didn't get through tough-kms/tests/all_test.rs, but here's what I have so far.

tough-kms/Cargo.toml

tough-kms/src/error.rs

tuftool/tests/create_repository_integration.rs

tough/src/error.rs

tough-kms/src/lib.rs

srgothi92

Added comments for Design related questions.

tough/src/key_source.rs

srgothi92

I added few other comments in last conversation. But, I could not see them any more. Adding them back.

tough-ssm/src/lib.rs

tuftool/src/root.rs

tough/src/key_source.rs

webern

I didn't get all the way through this. It seems like something is wrong with our design if adding a new key source causes a lot of churn in tough. The idea behind the key source trait was to ensure that tough (to the extent possible) doesn't know anything about what's under the hood in a key source.

Leaving the 'heavy lifting' of creating a key outside the realm of tough would be better if it prevents us from adding dependencies to tough.

I know you're working with Zack on this. I'd be happy to join a call to join any discussion. Thanks!

Nice start!

Cargo.toml

tough-kms/src/client.rs

tough-kms/src/error.rs

tough-kms/src/lib.rs

tough-ssm/src/lib.rs

tough/Cargo.toml

tough/src/key_source.rs

iliana · 2020-08-05T21:38:05Z

First, run cargo check --all to regenerate the Cargo.lock, and commit that. CI is requiring that your Cargo.lock is accurate and maps to the dependencies you are using across the project.

No Rusoto dependencies should exist in the main tough crate. I moved all of the added structs to tough-kms locally and realized the reason you may have done this: the Sign trait's sign method returns a Result<T, tough::Error> instead of allowing the implementation to return a more generic error. Because we have to implement Sign differently for KMS, this is impossible without adding the code directly to tough.

If you're comfortable with it, modify the Sign::sign trait signature to return a generic error, similar to the methods in KeySource (they return a Result<T, E> where E is Box<dyn std::error::Error + Send + Sync + 'static>>). Alternately, if you like, I can make this change in a separate PR, and you can rebase on top of it and carry on.

srgothi92 · 2020-08-05T22:18:42Z

Response to @iliana comment:
1:
Thanks.

2:
Not just because of Sign trait's sign method returns a Result<T, tough::Error>. In Sign object I would need KmsClient to sign the messages. For LocalKeySource and tough-ssm, sign object has private key available to sign the messages. Which is not the case with Kms. The solution I currently implemented adds rusoto_kms as dependency and makes a call to KMS directly using Kmsclient available with Sign trait. Which definitely is not the right way.
Solution Suggestion 1: I am thinking, if we can expose a module in tough_kms that is used by Sign trait to sign the messages, avoiding direct dependency on KmsClient, not sure if it would result in cyclic dependency. tough_kms provides sign object and sign object needs tough_kms.
Solution Suggestion 2: We can get rid of Sign trait and let KeySource expose sign and tuf_key method. Its upto keySource implementer to cache the private key and use the cached key for multiple sign request.

3:
I can take care of it, if required.

iliana · 2020-08-06T01:36:25Z

In Sign object I would need KmsClient to sign the messages.

KeySource::as_sign returns a Box<dyn Sign>, and KmsRsaKey (which has your KmsClient) implements Sign. This separation allows you to move KmsRsaKey to tough-kms.

zmrow · 2020-08-13T14:45:04Z

We just realized that we probably shouldn't create a tokio runtime inside these libraries, so we probably shouldn't ship this until #213 is fixed. :(

webern

Nice! As I understand it, because tough-kms needs to implement sign, the sign trait was changed to return a generic error to break dependency cycle. Looks good, looks clean.

iliana · 2020-08-24T20:48:56Z

Merging this into develop fails to compile — there are two new Sign implementations that need to have their signatures fixed. You'll probably want to rebase and resolve those issues.

I'm fixing that locally and continuing to review.

srgothi92 · 2020-08-24T22:24:42Z

Merging this into develop fails to compile — there are two new Sign implementations that need to have their signatures fixed. You'll probably want to rebase and resolve those issues.

I'm fixing that locally and continuing to review.

Done

iliana

A first pass of thoughts.

I'm unable to verify signatures produced by this code. I'm investigating why, because the answer is not obvious to me.

tuftool/tests/create_repository_integration.rs

tough-kms/Cargo.toml

tuftool/tests/create_repository_integration.rs

tough-kms/src/lib.rs

* tough-kms implements the "KeySource" trait found in tough * A new method "create" is added to "KeySource" trait to allow KeySource implementer to create Key. * Integration Tests are added in tuftool create to test 3 keysource. * To run integration tests use feature flag "integration-tests" while running test PR comments: * Removed toug-kms create and created issue awslabs#211 to handle it. * KMS CMK needs to be manually created separately * Moved Sign implementation for KmsRsaKey to tough_kms crate

srgothi92 · 2020-08-25T19:16:15Z

Fixed some of the code review comments, with multiple stupid force pushes. you might have to look at two of them to see the complete changes.

The DER-encoded document we are getting from the GetPublicKey API is in the SPKI format. `Decoded<RsaPem>`'s string form is SPKI, but its byte form is RSAPublicKey. Previously this code created the Decoded object from the byte form, which caused it to be double-wrapped in an SPKI document. Because we don't have access to tough's SPKI-parsing logic (not that any other crates should), we encode the certificate as a PEM and create the Decoded object from the string form.

iliana · 2020-08-25T22:04:32Z

ae2f466 fixes the verification issue! The commit message explains, but when we initially discussed how to go about creating the Decoded<RsaPem> I forgot that the byte form of it needs to be an RSAPublicKey document, not a SubjectPublicKeyInfo document. This confusion caused us to double-wrap the public key in the SPKI wrapper, resulting in this extremely confusing situation:

$ jq -r .signed.keys.a6c9b56dd7ce0a87fab1fa1c39d37afefcfb0692c05b2ddaa207919f4d1173d1.keyval.public ~/tmp/root.json \
> | openssl asn1parse
    0:d=0  hl=4 l= 442 cons: SEQUENCE          
    4:d=1  hl=2 l=  13 cons: SEQUENCE          
    6:d=2  hl=2 l=   9 prim: OBJECT            :rsaEncryption
   17:d=2  hl=2 l=   0 prim: NULL              
   19:d=1  hl=4 l= 423 prim: BIT STRING        
$ jq -r .signed.keys.a6c9b56dd7ce0a87fab1fa1c39d37afefcfb0692c05b2ddaa207919f4d1173d1.keyval.public ~/tmp/root.json \
> | openssl asn1parse -strparse 19
    0:d=0  hl=4 l= 418 cons: SEQUENCE          
    4:d=1  hl=2 l=  13 cons: SEQUENCE          
    6:d=2  hl=2 l=   9 prim: OBJECT            :rsaEncryption
   17:d=2  hl=2 l=   0 prim: NULL              
   19:d=1  hl=4 l= 399 prim: BIT STRING

tough-kms/tests/data/response_public_key.json

tuftool/tests/create_repository_integration.rs

srgothi92 · 2020-08-26T20:10:35Z

PR comments fixes.

iliana · 2020-08-27T17:48:52Z

We just realized that we probably shouldn't create a tokio runtime inside these libraries, so we probably shouldn't ship this until #213 is fixed. :(

I'd rather merge this first, then do the async party. :)

iliana

Final stretch!

tough-kms/src/lib.rs

tuftool/tests/create_repository_integration.rs

…fication fix

srgothi92 · 2020-08-27T22:31:15Z

Fixed PR comments. Check 1st force-pushed for all relevant changes, 2nd was for missed cargo fmt -- --check

zmrow

🌃

lgtm

zmrow requested review from webern, iliana and zmrow July 30, 2020 20:02

zmrow reviewed Jul 30, 2020

View reviewed changes

srgothi92 commented Jul 30, 2020

View reviewed changes

tough/src/key_source.rs Outdated Show resolved Hide resolved

srgothi92 commented Jul 30, 2020

View reviewed changes

tough-ssm/src/lib.rs Outdated Show resolved Hide resolved

tuftool/src/root.rs Outdated Show resolved Hide resolved

tough/src/key_source.rs Outdated Show resolved Hide resolved

webern reviewed Aug 1, 2020

View reviewed changes

srgothi92 force-pushed the develop branch from ecbb642 to f1974cf Compare August 10, 2020 22:07

webern approved these changes Aug 19, 2020

View reviewed changes

srgothi92 force-pushed the develop branch 3 times, most recently from 7bd0bfe to 8896bf8 Compare August 24, 2020 22:17

iliana reviewed Aug 24, 2020

View reviewed changes

srgothi92 force-pushed the develop branch 2 times, most recently from d059c90 to 0fa9c97 Compare August 25, 2020 18:32

srgothi92 force-pushed the develop branch from 0fa9c97 to 2144a08 Compare August 25, 2020 18:34

iliana reviewed Aug 25, 2020

View reviewed changes

tough-kms/tests/data/response_public_key.json Outdated Show resolved Hide resolved

iliana reviewed Aug 25, 2020

View reviewed changes

tuftool/tests/create_repository_integration.rs Outdated Show resolved Hide resolved

tuftool/tests/create_repository_integration.rs Outdated Show resolved Hide resolved

tuftool/tests/create_repository_integration.rs Show resolved Hide resolved

webern approved these changes Aug 27, 2020

View reviewed changes

iliana reviewed Aug 27, 2020

View reviewed changes

srgothi92 force-pushed the develop branch from 9e123af to db28f45 Compare August 27, 2020 21:57

Fixed PR comments for and Test case for toug-kms after signature veri…

dc27668

…fication fix

srgothi92 force-pushed the develop branch from db28f45 to dc27668 Compare August 27, 2020 22:13

zmrow approved these changes Aug 28, 2020

View reviewed changes

iliana approved these changes Aug 28, 2020

View reviewed changes

Merge branch 'develop' into develop

b653976

iliana merged commit fb48c63 into awslabs:develop Aug 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented tough-kms to include AWS KMS as a new keysource #198

Implemented tough-kms to include AWS KMS as a new keysource #198

srgothi92 commented Jul 27, 2020 •

edited

Loading

zmrow left a comment

srgothi92 left a comment

srgothi92 left a comment

webern left a comment

iliana commented Aug 5, 2020

srgothi92 commented Aug 5, 2020

iliana commented Aug 6, 2020

zmrow commented Aug 13, 2020

webern left a comment

iliana commented Aug 24, 2020

srgothi92 commented Aug 24, 2020

iliana left a comment

srgothi92 commented Aug 25, 2020

iliana commented Aug 25, 2020 •

edited

Loading

srgothi92 commented Aug 26, 2020

iliana commented Aug 27, 2020

iliana left a comment

srgothi92 commented Aug 27, 2020

zmrow left a comment

Implemented tough-kms to include AWS KMS as a new keysource #198

Implemented tough-kms to include AWS KMS as a new keysource #198

Conversation

srgothi92 commented Jul 27, 2020 • edited Loading

zmrow left a comment

Choose a reason for hiding this comment

srgothi92 left a comment

Choose a reason for hiding this comment

srgothi92 left a comment

Choose a reason for hiding this comment

webern left a comment

Choose a reason for hiding this comment

iliana commented Aug 5, 2020

srgothi92 commented Aug 5, 2020

iliana commented Aug 6, 2020

zmrow commented Aug 13, 2020

webern left a comment

Choose a reason for hiding this comment

iliana commented Aug 24, 2020

srgothi92 commented Aug 24, 2020

iliana left a comment

Choose a reason for hiding this comment

srgothi92 commented Aug 25, 2020

iliana commented Aug 25, 2020 • edited Loading

srgothi92 commented Aug 26, 2020

iliana commented Aug 27, 2020

iliana left a comment

Choose a reason for hiding this comment

srgothi92 commented Aug 27, 2020

zmrow left a comment

Choose a reason for hiding this comment

srgothi92 commented Jul 27, 2020 •

edited

Loading

iliana commented Aug 25, 2020 •

edited

Loading