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

Crypto: Support Azure Key Vault #3128

Merged
merged 7 commits into from
Jun 3, 2024
Merged

Crypto: Support Azure Key Vault #3128

merged 7 commits into from
Jun 3, 2024

Conversation

@reinkrul reinkrul force-pushed the crypto/azure-keyvault branch from 3b17fa3 to 511923c Compare May 21, 2024 04:32
@reinkrul reinkrul force-pushed the crypto/azure-keyvault branch from 8951009 to 4da23b0 Compare May 28, 2024 10:14
@reinkrul reinkrul requested review from gerardsn and woutslakhorst and removed request for gerardsn May 28, 2024 10:21
@reinkrul reinkrul marked this pull request as ready for review May 28, 2024 10:53
@reinkrul
Copy link
Member Author

@woutslakhorst can you review this?

My main concern is that we need to provide Azure Key Vault with a key name, for which I chose to use the verification method's key ID. But, Azure Key Vault only allows [a-z0-9-] in key names, so we have to "dumb down" the key name, replacing all illegal characters with -.
The alternative is having a SQL table which maps verification method IDs (key IDs) to keys in Azure Key Vault, but then mapping keys in Azure Key Vault back to DIDs becomes hard(er) again. So I think this is the lesser evil.

But, if we want to support creating keys in key storage that require the key thumbprint (e.g. did:nuts or did:jwk), we need that coupling again, since we must determine the key name for Vault before creating the key.

I also checked if we can rename the key in Key Vault, but that's not possible.

crypto/crypto.go Outdated Show resolved Hide resolved
crypto/jwx.go Outdated Show resolved Hide resolved
crypto/crypto.go Show resolved Hide resolved
docs/pages/deployment/storage.rst Show resolved Hide resolved
docs/pages/deployment/storage.rst Outdated Show resolved Hide resolved
crypto/storage/azure/keyvault.go Show resolved Hide resolved
crypto/storage/azure/keyvault.go Show resolved Hide resolved
crypto/storage/azure/keyvault.go Outdated Show resolved Hide resolved
crypto/storage/azure/keyvault.go Outdated Show resolved Hide resolved
crypto/storage/azure/keyvault.go Outdated Show resolved Hide resolved
Copy link
Member

@gerardsn gerardsn left a comment

Choose a reason for hiding this comment

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

LGTM
just a comment on the panic comment

@reinkrul reinkrul force-pushed the crypto/azure-keyvault branch from d1c101c to 1df084b Compare June 3, 2024 17:23
@reinkrul reinkrul merged commit e96bb54 into master Jun 3, 2024
7 of 9 checks passed
@reinkrul reinkrul deleted the crypto/azure-keyvault branch June 3, 2024 17:44
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