-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
GCP and Azure Login methods for Go client library #13022
Conversation
…ing, add timeouts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments on the Azure library (LGTM otherwise). Moving on to GCP next 👍
api/auth/azure/azure.go
Outdated
// for a list of valid environments. | ||
func WithCloud(cloud azure.Environment) LoginOption { | ||
return func(a *AzureAuth) error { | ||
a.resource = cloud.ResourceManagerEndpoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something like WithResource
makes sense here instead of WithCloud
. From the context of this library, the resource
is used as a query parameter when requesting an access token from the Azure instance metadata service (IMDS). This value determines the API access that's given to the token (see resource parameter description).
Ultimately, this must match what was configured as the resource for the plugin config, since it'll be expected to appear as the aud
claim on the access token (JWT). See the validation code on the plugin side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @elsesiy I've changed this back to WithResource(url string) because as Austin explained to me, this value is just used to populate the aud
claim so it needs to match whatever URL the auth backend was configured with. (Which may not actually be an Azure URL, it just depends on what the one who configured Vault set it as.) No Azure API access is specifically given.
Users are still welcome to use the autorest constants you mentioned to populate the string though if they know theirs is an Azure one.
resource
is admittedly a confusing field all around. I'm keeping the default as the Azure public cloud ARM endpoint just to help those users who probably configured their Vault servers with it since that's what "resource" usually means to them.
@@ -0,0 +1,184 @@ | |||
package gcp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be cool to eventually replace the login helper (see cli.go) in the plugin with this code. It'd make it quick to add support for obtaining GCE tokens from within GCE instances 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
} | ||
} | ||
|
||
func WithIAMAuth(serviceAccountEmail string) LoginOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be cool to also have an option to just supply a JWT. For example, you can obtain a signed JWT using gcloud or curl (examples). Not a blocker though, can always add this later.
@@ -0,0 +1,3 @@ | |||
```release-note:improvement | |||
implements Login method in Go client libraries for GCP and Azure auth methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note for next time - we'd want this file to have a submodule as a prefix to this line - in this case, it would be something like: api: Implement Login method in...
I'll tag you as a reviewer on an ENT PR so the changelog will format properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry about that!
Continuing with the addition of native Login methods for each auth method to the Go client library, in order to simplify the authentication process for users who may struggle to craft the login request body on their own. As before, there is one module per auth method.