Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adding import functionality #13

Merged
merged 28 commits into from
Jul 1, 2024
Merged

feat: adding import functionality #13

merged 28 commits into from
Jul 1, 2024

Conversation

johnson2427
Copy link
Collaborator

@johnson2427 johnson2427 commented Jun 24, 2024

What I did

Adding import for Eth private keys to AWS KMS

fixes: #12

How I did it

Added functionality, trying to emulate This Article in python

How to verify it

ape aws kms import 'testAlias' 'Description' 'private-key'

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
  • New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

@johnson2427 johnson2427 self-assigned this Jun 24, 2024
ape_aws/client.py Outdated Show resolved Hide resolved
ape_aws/client.py Show resolved Hide resolved
ape_aws/kms/_cli.py Outdated Show resolved Hide resolved
ape_aws/kms/_cli.py Show resolved Hide resolved
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall a bit confused why we are storing the private keys on disk once they are imported

ape_aws/kms/_cli.py Outdated Show resolved Hide resolved
ape_aws/kms/_cli.py Outdated Show resolved Hide resolved
ape_aws/kms/_cli.py Outdated Show resolved Hide resolved
ape_aws/accounts.py Outdated Show resolved Hide resolved
ape_aws/accounts.py Outdated Show resolved Hide resolved
ape_aws/accounts.py Outdated Show resolved Hide resolved
ape_aws/accounts.py Outdated Show resolved Hide resolved
ape_aws/accounts.py Outdated Show resolved Hide resolved
@johnson2427
Copy link
Collaborator Author

Overall a bit confused why we are storing the private keys on disk once they are imported

I built out the ability to allow ape-aws to create a key for the user just to give us a visual of what that would look like in case it was something you may have wanted. I originally built out this functionality to get a key in the correct format so I knew what I was dealing with, with respect to KMS. KMS has different requirements for key formats, and with the requirement of needing to use the public key and private key together to encrypt the private key to send, I needed to know what KMS was looking for. After building out the feature, I figured I'd let you look at it to see if it was something you wanted. If you did want it, we needed the ability to store that key.

ape_aws/kms/_cli.py Outdated Show resolved Hide resolved
ape_aws/kms/_cli.py Outdated Show resolved Hide resolved
@johnson2427 johnson2427 requested a review from fubuloubu July 1, 2024 14:51
README.md Outdated Show resolved Hide resolved
Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually display on your machine? I hadn't seen this before

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions, looking close though

ape_aws/kms/_cli.py Outdated Show resolved Hide resolved
ape_aws/kms/_cli.py Outdated Show resolved Hide resolved
ape_aws/kms/_cli.py Outdated Show resolved Hide resolved
ape_aws/kms/_cli.py Outdated Show resolved Hide resolved
)
response = kms_client.import_key(import_key_spec)
if response["ResponseMetadata"]["HTTPStatusCode"] != 200:
cli_ctx.abort("Key failed to import into KMS")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it give any message in case of a failure? might want to display it

Copy link
Collaborator Author

@johnson2427 johnson2427 Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the response I get when I get a failure

ERROR: Key failed to import into KMS

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well yeah, that's what the above line will give you

I'm asking if there's any field in response that will help you understand the issue that came up which we can print w/ .abort

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhhhh, let me check what we can give to the user

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boto3 seems to catch most of these errors, I tried to get a bad response, but I believe boto3 is handling the errors. To be sure, this is how I'm handling it

johnson2427 and others added 3 commits July 1, 2024 12:52
Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>
Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>
Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>
@johnson2427
Copy link
Collaborator Author

Does this actually display on your machine? I hadn't seen this before

which are you referring to?

@fubuloubu
Copy link
Member

Does this actually display on your machine? I hadn't seen this before

which are you referring to?

Where it was saying SUCCESS (ape-aws): ..., I don't think that the (ape-aws) actually shows up in the display

@johnson2427
Copy link
Collaborator Author

Does this actually display on your machine? I hadn't seen this before

which are you referring to?

Where it was saying SUCCESS (ape-aws): ..., I don't think that the (ape-aws) actually shows up in the display

oh, yeah that was showing up in my returns, but if it isn't showing up in yours, I'm not too worried about it

@fubuloubu
Copy link
Member

Does this actually display on your machine? I hadn't seen this before

which are you referring to?

Where it was saying SUCCESS (ape-aws): ..., I don't think that the (ape-aws) actually shows up in the display

oh, yeah that was showing up in my returns, but if it isn't showing up in yours, I'm not too worried about it

I didn't actually try it, might be a new feature of Ape v0.8

@johnson2427 johnson2427 merged commit 66695cf into main Jul 1, 2024
18 checks passed
@johnson2427 johnson2427 deleted the feat/add-key-import branch July 1, 2024 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add import for KMS feature
2 participants