add provider ca auth-method support for azure #16298
Conversation
eikenb
added
type/enhancement
kisunji
changed the title
eikenb
force-pushed
the
eikenb/azure-vault-provider-auth
branch
from
a56a083
to
5f4e781
Compare
|
|
||
| // Checks to see if all the parameters needed to login have been hardcoded in the | ||
| // auth-method's config Parameters field. | ||
| func legacyCheck(params map[string]any) bool { |
There was a problem hiding this comment.
I really like the func name legacyCheck.. there is already a function called containsVaultLoginParams that has some overlap with this method.. What do you think about consolidating these two functions?
There was a problem hiding this comment.
Right, it is basically the same function just passing in the list of keys. I'll replace containsVaultLoginParams with legacyCheck (adding the expectedKeys as an arg) and update the AWS/GCP plugins to us it. Thanks.
There was a problem hiding this comment.
Noticed they have a key difference in testing... the containsVaultLoginParams returns true when it finds any of the hardcoded /login values where legacyCheck only returns true if all of them are present. In the case of Azure they are all required for it to work, were AWS/GCP different or did you go with the idea that if they had any of the /login fields it meant they were using that path and can return early?
There was a problem hiding this comment.
Yeah, exactly.. My thought was that if any of the /login args are present, then the user has most likely configured the CA provider for a direct login.. I think that it's reasonable for the func to check for all required args before returning true since most folks will probably want to use new auto auth config.
There was a problem hiding this comment.
Thanks @cthain, I'll go with all fields required then and make those changes.
There was a problem hiding this comment.
Started looking at updating AWS to this and it has multiple different ways of calling /login with different sets of parameters, thus there is no 1 set of required parameters to check.
So I'm flip-flopping on this and will make it use 'legacy mode' if it detects any of the 'legacy' or hard coded /login fields. Going to re-review my uses to make sure none of them have fields that overlap with non-/login-hack uses. (eg. role in azure is required for /login but also required for proper config)
Does the required dance with the local HTTP endpoint to get the required data for the jwt based auth setup in Azure. Keeps support for 'legacy' mode where all login data is passed on via the auth methods parameters.
New, improved name and used in more places.
eikenb
force-pushed
the
eikenb/azure-vault-provider-auth
branch
from
5f4e781
to
7fe2c8e
Compare
Does the required dance with the local HTTP endpoint to get the required data for the jwt based auth setup in Azure. Keeps support for 'legacy' mode where all login data is passed on via the auth methods parameters.
PR Checklist