Avoid invoking token helper on login

[ruuda]

This fixes #23194.

Problem

vault login used to call the token helper in get mode, because it constructs an HTTP client, and the client automatically loads the token. For almost everything that is the right thing to do, but for login, that is the thing that is supposed to retrieve the token, and login itself does not require the token. In fact, vault login would erase the token on the client later on.

Calling the token helper in get mode is a problem, because if the token helper fails, that blocks the login. But the token helper might fail because it doesn't have a token yet.

The call to Get originates here, in the construction of the http client:

vault/command/base.go

Line 145 in 6ef2a60

token, err = helper.Get()

That is being called from here:

vault/command/login.go

Line 215 in 6ef2a60

client, err := c.Client()

Solution

Split Client into two parts: a ClientWithoutToken that does most of what Client did previously, except for setting a token. And Client that also sets the token. This does change when the token gets set on the client for calls to Client, but as far as I can tell, the remainder of the former Client, now ClientWithToken method does not rely on the token being set.

For all existing code except login, Client still behaves the way it did and nothing changes. For login in particular, we can now call ClientWithoutToken. This ensures that the token helper does not get called when the http client is initialized. The token helper still gets called to store the token later.

Open questions, testing

There is still this snippet that happens after constructing the client:

vault/command/login.go

Lines 221 to 225 in 6ef2a60

	// Evolving token formats across Vault versions have caused issues during CLI logins. Unless
	// token auth is being used, omit any token picked up from TokenHelper.
	if authMethod != "token" {
	client.SetToken("")
	}

I think this is now useless, the client will not have a token anyway so we don’t need to reset it. But it makes me wonder — if authMethod == "token", does my change break anything?

Also, I am not sure what the best way is to test this, if somebody can point me in the right direction, I would appreciate.

[hashicorp-cla]

All committers have signed the CLA.

[divyaac]

HI @ruuda, thank you so much for this PR! In fact, we actually require the client to set the token (and therefore pull the token value using the token helper). The reason is that in case the authentication type is "token" (https://github.com/hashicorp/vault/blob/main/command/login.go#L170), we need the token value to be set.