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

[KeyVault] - Deploy attestation service as a test resource #16848

Merged
merged 10 commits into from
Aug 13, 2021

Conversation

maorleger
Copy link
Member

@maorleger maorleger commented Aug 10, 2021

What

  • When deploying a Managed HSM, deploy a mock attestation service from the EngSys managed Docker container
  • Update our Secure Key Release to work with a dynamically deployed attestation service

Why

For Secure Key Release we require an additional live resource - a service that can generate and sign tokens as well as provide
the OIDC Configuration to discover the jwks URI. We currently use a static test fixture that I have spun up and deployed to
Azure App Service; however, that is not sustainable and really not in line with our guidelines on test assets

This allows us to maintain an image and keep the code centralized and in a repository owned by EngSys.

See https://github.com/Azure/azure-sdk-tools/tree/main/tools/keyvault-mock-attestation for the source code for the container.

@ghost ghost added the KeyVault label Aug 10, 2021
@maorleger maorleger marked this pull request as ready for review August 10, 2021 23:05
@maorleger maorleger requested a review from sadasant as a code owner August 10, 2021 23:05
Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

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

I think this is ok, but I have the feeling that this just got 1.5x as complex as before.

I have the feeling an outsider might not clearly see how these changes interoperate. Should we add some kind of documentation to remember about this PR or these changes somewhere? What I’m trying to say is, next time we onboard someone on the Key Vault clients, how can we make their experience more straightforward?

I’m not proposing to address this feedback as part of this PR. If this argument doesn’t sound strong enough, I’m also ok with leaving this as a comment in case someone goes back to this PR from the future.

@maorleger
Copy link
Member Author

maorleger commented Aug 11, 2021

I think this is ok, but I have the feeling that this just got 1.5x as complex as before.

Agree - it's a lot added for a single scenario but we do need to deploy something for live testing SKR

I have the feeling an outsider might not clearly see how these changes interoperate. Should we add some kind of documentation to remember about this PR or these changes somewhere? What I’m trying to say is, next time we onboard someone on the Key Vault clients, how can we make their experience more straightforward?

Definitely, I can do the following:

I’m not proposing to address this feedback as part of this PR. If this argument doesn’t sound strong enough, I’m also ok with leaving this as a comment in case someone goes back to this PR from the future.

No worries, I dont think there's a huge rush since we have a static test resource. I can try to add it to this PR

Thanks for the feedback!

@maorleger maorleger force-pushed the keyvault-dynamic-skr branch from 6ab761c to 4586477 Compare August 11, 2021 17:43
@maorleger
Copy link
Member Author

/azp run js - keyvault-keys - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@maorleger maorleger force-pushed the keyvault-dynamic-skr branch from 4586477 to cb2f55f Compare August 12, 2021 16:30
@maorleger maorleger merged commit 12d6850 into Azure:main Aug 13, 2021
@maorleger maorleger deleted the keyvault-dynamic-skr branch August 13, 2021 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants