Add initial Azure KMS support #76
Conversation
pkg/kms/azure/azure.go
Outdated
| return nil, errors.New("AZURE_TENANT_ID is not set") | ||
| } | ||
|
|
||
| azureClientID := os.Getenv("AZURE_CLIENT_ID") |
There was a problem hiding this comment.
I don't know much about Azure - are these typically set as environment variables? Would it make sense for them to be passed as flags instead?
There was a problem hiding this comment.
I was following a similar approach that I saw for vault. I will review that when apply the rebate and refactor to the new way
Thanks for you review and feedback
There was a problem hiding this comment.
Thanks! I asked the same question on the vault one, and I guess that environment variable is the standard, canonical method of specifying the address, so it made sense to keep.
There was a problem hiding this comment.
I think viper.GetString works with both flags and env vars?
There was a problem hiding this comment.
@dlorenc @cpanato from what I remember, in Azure the default is by environment variables, alternatively a file could be used or cli device login. https://github.com/Azure/azure-sdk-for-go#more-authentication-details
The env based login approach that is currently implemented is usually used for application based login with either an Azure Application (client secret/cert) or Azure Managed Service Identity (works only on azure based vms and services, where this is available) is used for the login.
I think for the cosign cli it would be valuable to implement also the auth.NewAuthorizerFromCli(), which then uses an installed azure cli to execute a device login, where a browser is openend and the user can do his single sign on. With this method not only technical users, but also regular cli users could run the signing.
Maybe this could be toggled via cli flag.
There was a problem hiding this comment.
I had that in my mind as well, we also use the env var is in CAPZ (cluster API for azure)
Is that ok to keep as-is for this iteration? @dlorenc
There was a problem hiding this comment.
I think also that a specific sign in by user might be a use case, that is not that relevant when only a CI is doing the signing. Maybe something to consider for the future :)
cpanato
force-pushed
the
azure-kms
branch
3 times, most recently
from
499a486
to
e529ce4
Compare
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
|
integrated with cosign |
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
|
@dlorenc this is an initial implementation, I'm sure there are lots of things to improve. But I think we can do any followups and also maybe other persons more expert than I can help here as well :) the video above shows the commands with cosign to generate the key, sign, and verify! :) let me know what you think |
|
Awesome! |
Add initial Azure KMS support, Just saw now that there are some refactors to do. I will apply those.
Opening as a draft for gathering any feedback