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

Added azure kms support #89

Merged
merged 6 commits into from
Jun 2, 2023
Merged

Added azure kms support #89

merged 6 commits into from
Jun 2, 2023

Conversation

kommendorkapten
Copy link
Collaborator

No description provided.

@jku
Copy link
Owner

jku commented May 26, 2023

This doesn't work as it is as the GCP input is just the KMS key id, not the fully formed URI: the idea is that user input is something they can just copy-paste from GCP KMS UI. User does not need to form the URI, the GCPSigner.import_() method does that.

The AzureSigner.import_() arguments can be whatever you want,but I don't know why you would want to make your users build the full URI? Why not let AzureSigner build the URI from some logical values that the user understands (like keyid and vaultname)?

I think now that we have mutliple different keys, we want to modify the UI so it first asks which keytype we want to use

1. Sigstore
2. Google Cloud KMS
3. Azure KMS
Please select online keytype:

Feel free to choose how you want to enter the azure details but it sounds like something like this would make sense:

Please enter vault name: 
Please enter key name:

@kommendorkapten
Copy link
Collaborator Author

kommendorkapten commented May 30, 2023

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
@kommendorkapten
Copy link
Collaborator Author

Tests are failing as the AzureSigner does not exist in securesystemslib. It works locally so when the pr against securesystemslib is merged I'll rerun the tests.

Comment on lines 177 to 179
type=click.IntRange(0, 4),
default=0,
show_default=False,
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about defaulting to sigstore (and not hiding the default)?

I'm a bit conflicted:

  • it's pretty clear the sigstore support is a little experimental still, but...
  • mostly for demonstration purposes I'd like to have default values that (once the current dependency fiasco is solved) just work if you keep pressing enter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I agree. Default to sigstore makes sense as that would make it easy to get on board. I'll update. I think for a real situation, when you are actually configuring a trust root, you don't blindly press enter, you think carefully about your config so there is no need to rely on the default values.

Copy link
Collaborator Author

@kommendorkapten kommendorkapten Jun 2, 2023

Choose a reason for hiding this comment

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

Also one thing I would like to have feedback on. The local testing key, before it was based on a "known value", now it's a "hidden option" (4). You are ok with that?

Copy link
Owner

Choose a reason for hiding this comment

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

hiding option 4 seems good to me

@kommendorkapten kommendorkapten marked this pull request as ready for review June 2, 2023 06:00
Copy link
Owner

@jku jku left a comment

Choose a reason for hiding this comment

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

looks good to me, apart from the default online key change

playground/tests/e2e.sh Outdated Show resolved Hide resolved
playground/signer/playground_sign/delegate.py Outdated Show resolved Hide resolved
Updated the default option when chosing online key type.

Signed-off-by: Fredrik Skogman <kommendorkapten@github.com>
@jku jku merged commit 94217d1 into jku:main Jun 2, 2023
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.

2 participants