Use idtoken for auth with basic as fallback #49
Conversation
v2/cmd/voucher_client/config.go
Outdated
| newClient.SetBasicAuth(defaultConfig.Username, defaultConfig.Password) | ||
| var newClient *client.Client | ||
| newClient, err := client.NewAuthClient(defaultConfig.Server) | ||
| if err != nil { |
There was a problem hiding this comment.
FYI, this will not fail in GKE, so this will be executed only if voucher runs in a non-GCP environment. You need to use NewClient and SetBasicAuth directly, without AuthClient for it to work in GKE.
There was a problem hiding this comment.
I wonder if we should have some flag to indicate that auth is required, so if I want to use the voucher_client cli I can specify that this requires auth via id tokens. Then, we can call either NewClient or AuthClient depending on auth being enabled or not...
There was a problem hiding this comment.
That could be another option for sure. I was considering that, but I thought it'd be cleaner to choose between 2 functions instead of providing a flag or a boolean.
But good point, maybe it'd be more convenient here specifically. This way it'd be easier to switch between GKE and CloudRun.
There was a problem hiding this comment.
I can definitely add support for that here! Since this is basically standalone from rest of voucher
There was a problem hiding this comment.
Yeah. My thought here was that we wouldn't be getting an error back and it would try to create a client with Auth and it would fail the vouch checks as we've seen in auto-voucher. With the flag approach, we're making sure it's backwards compatible, so we know for sure which client to instantiate based on the flag
|
Updated the code to add support of configuring auth type |
|
how are we testing this? |
| newClient, err := client.NewAuthClient(defaultConfig.Server) | ||
| return newClient, err | ||
| default: | ||
| newClient, err := client.NewClient(defaultConfig.Server) |
There was a problem hiding this comment.
I ran the check manually (check if basic auth works) + will be testing it with cloudbuild later!
There was a problem hiding this comment.
Idea: be more explicit about how the basic option flows here (it's the same as the "default" case).
case "basic":
fallthroughIdea: maybe strings.ToLower(defaultConfig.Auth) ? The user can probably be trusted, but --auth IDTOKEN using basic auth is certainly unexpected.
There was a problem hiding this comment.
good point! I definitely like the idea of having default return an error! Will implement the changes you suggested
There was a problem hiding this comment.
LGTM, and backwards compat.
I pondered if because "basic" is included as the default value, maybe the case "basic": clause should be made explicit, and the default: clause made to return an error (e.g. fmt.Errorf("invalid auth value: %q", defaultConfig.Auth)).
My thinking is that since the user has to be actively breaking something (by passing the --auth flag), it might be productive to fail fast.
| newClient, err := client.NewAuthClient(defaultConfig.Server) | ||
| return newClient, err | ||
| default: | ||
| newClient, err := client.NewClient(defaultConfig.Server) |
There was a problem hiding this comment.
Idea: be more explicit about how the basic option flows here (it's the same as the "default" case).
case "basic":
fallthroughIdea: maybe strings.ToLower(defaultConfig.Auth) ? The user can probably be trusted, but --auth IDTOKEN using basic auth is certainly unexpected.
With #48, adding idtoken auth for
voucher_client. It auth fails, we fallback to basic