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

GCP and Azure Login methods for Go client library #13022

Merged
merged 9 commits into from Nov 12, 2021

Conversation

digivava
Copy link
Collaborator

@digivava digivava commented Nov 2, 2021

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.

@vercel vercel bot temporarily deployed to Preview – vault November 2, 2021 22:00 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 2, 2021 22:00 Inactive
api/auth/azure/azure.go Outdated Show resolved Hide resolved
api/auth/azure/azure.go Show resolved Hide resolved
api/auth/azure/azure.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault November 4, 2021 22:03 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 4, 2021 22:03 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 8, 2021 20:33 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 8, 2021 20:33 Inactive
api/auth/azure/azure.go Show resolved Hide resolved
api/auth/gcp/gcp.go Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 10, 2021 16:56 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 10, 2021 16:56 Inactive
Copy link
Member

@austingebauer austingebauer left a 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 Show resolved Hide resolved
// for a list of valid environments.
func WithCloud(cloud azure.Environment) LoginOption {
return func(a *AzureAuth) error {
a.resource = cloud.ResourceManagerEndpoint
Copy link
Member

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.

Copy link
Collaborator Author

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.

@vercel vercel bot temporarily deployed to Preview – vault November 10, 2021 22:35 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 10, 2021 22:35 Inactive
@@ -0,0 +1,184 @@
package gcp
Copy link
Member

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 👍

Copy link
Member

@austingebauer austingebauer left a 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 {
Copy link
Member

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.

@digivava digivava merged commit d9a0adc into main Nov 12, 2021
@digivava digivava deleted the digivava/gcp-azure-client-login branch November 12, 2021 17:32
@@ -0,0 +1,3 @@
```release-note:improvement
implements Login method in Go client libraries for GCP and Azure auth methods
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, sorry about that!

qk4l pushed a commit to qk4l/vault that referenced this pull request Feb 4, 2022
* Add native Login method for GCP auth backend

* Add native Login method for Azure auth backend

* Add changelog entry

* Use official azure library Environment struct rather than passing string, add timeouts

* Use v1.3.0 which now has interface definition

* Don't throw away error and close resp body

* Back to WithResource so we can support non-Azure URLs for aud
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.

None yet

5 participants