Avoid invoking token helper on login

[ruuda]

Resolves #312.

Problem

bao 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:

openbao/command/base.go

Line 145 in 6ef2a60

token, err = helper.Get()

That is being called from here:

openbao/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:

openbao/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 add tests for this, if somebody can point me in the right direction, I would appreciate.

Note about Source-Available License Policy

I adapted this patch from one I originally submitted to Hashicorp Vault in hashicorp/vault#23209. However, the change was not accepted there and never included into a Hashicorp release, so I think it’s fine to submit it here?

The pull request was closed there with a message that’s unclear to me, but given that it took more than half a year and me posting and asking in multiple places before a person even looked at it, I don’t have high hopes for getting a clarification there. I hope OpenBao is more open to community contributions. If the approach in this pull request is not the right one, I’m happy to change it if somebody can explain what the correct approach would be.

Target Release

No particular target, by default this was pre-filled to 1.14.7.

[cipherboy]

@ruuda in this case, since it was your own PR that you own the rights to, it is fine. In general, taking PRs from upstream from others isn't accepted. :-)

I'll have to take a closer look at this, but did you happen to handle @divyaac's comment? At first glance it looks valid; a token helper could be used in conjunction with bao login -method=token, which would expect the helper to already have set said token. Thoughts? :-)

[cipherboy]

In particular, down below where token is cleared when the authMethod != "token", I'd add an else condition to re-trigger the helper logic on (new) lines 187...197 above in command/base.go.

Other than that, LGTM!

[ruuda]

At first glance it looks valid; a token helper could be used in conjunction with bao login -method=token, which would expect the helper to already have set said token. Thoughts? :-)

I don’t think I understand what is going on with bao login -method=token.

• As far as I can tell, the only thing it ends up doing is making a request to /v1/auth/token/lookup-self so it can print the token details, which bao token lookup also does.
• One use case for it is to store an externally obtained token in the token helper. That would mean that for -method=token, login should set the token on the client to the user-provided token. This behavior is currently lost in this PR because token := client.Token() is no longer called.
• Another use case for this would be to act as a kind of alias to bao token lookup.

So I see three approaches:

1. If the purpose of login -method=token is to be an alternative to token lookup, then it makes sense to me that it should consult the token helper, and then yes in login.go I should add a branch for authMethod == "token". (Or maybe a bit cleaner would be to just initialize client to the regular Client() when authMethod == "token".)

2. If the purpose of login -method=token is to store an externally obtained token in the token helper, then we shouldn’t consult the token helper, because it might not have a token, and if it fails that prevents the login attempt. In this case, I think the right thing to do is to restore the token := client.Token() behavior but now in login.go, to read the token from the environment when authMethod == "token".

3. A possible middle ground is to take the second behavior when a token is present in the environment or on the command line, but the first one when there is none.

Does any of this make sense, or am I misunderstanding something?

[cipherboy]

\o Hello @ruuda,

Does any of this make sense, or am I misunderstanding something?

I might be missing something, but in approach 2, why do you think:

and if it fails that prevents the login attempt.

is bad?

This roughly aligned with my understanding, I think? A user want to log in (bao login). They want a token (and may not have one set). If they do, use that one. If they don't, consult the token helper and use that one. Now if the token helper fails, they're explicitly logging in, so... they wouldn't have a token. And so they'd want to know that and fail bao login, IMO?

Aha, so my line numbers are not greedy enough.

Let's expand it to something like (the full existing behavior):

} else { // authMethod == "token"
 token := client.Token() // get from env
 if token == "" {
  // invoke token helper and err if necessary
 }
 client.SetToken(token) // set token
}

Thoughts?

I think you are broadly right though, if you knew ahead of time that you were going to auth with token auth, its a rather useless method in the general case and could probably just drop the call to bao login entirely. But if you were attempting to write a more generic script with a couple of calls (set up an initial user account? start provisioning a database?) and you wanted to enforce login somehow and let the user choose it, you could perhaps support use token login to do this?

[ruuda]

A user want to log in (bao login). They want a token (and may not have one set). If they do, use that one. If they don't, consult the token helper and use that one. Now if the token helper fails, they're explicitly logging in, so... they wouldn't have a token. And so they'd want to know that and fail bao login, IMO?

Yes it does make sense to me now! I was confused about the order of things, if you provide a token in the environment or on the command line, but the token helper doesn’t have it yet, then we should not check the token helper, and the login should succeed. I’ll update the PR accordingly.

[ruuda]

I’m testing this further, and the current behavior of bao login -method=token, even if you pass a VAULT_TOKEN in the environment, is to ask the user interactively. The prompt is only skipped if you explicitly pass the token as a CLI argument. Even if there is a valid token in ~/.vault-token (which is where the default token helper stores it), bao login -method=token does not use it.

If we make bao login -method=token consult the token helper now, that would change that behavior. In particular, it would change the behavior of bao login without additional arguments, which defaults to -method=token, in the case where you have an old expired token in ~/.vault-token. (Which for me is very common, it happens every time I start my working day.) In the past it would then have prompted for a token interactively, but if we consult the token helper, it would print a permission denied error.

I think it is probably best not to change that behavior? I will make the case where the token is provided as a CLI arg store it in the token helper then, but not change how it works without additional args. @cipherboy what do you think?

Edit: Ah but that behavior, to make bao login store the token in the token helper when passed on the CLI, but keep prompting interactively otherwise — that’s exactly what this PR in its current form already does 😅

[cipherboy]

Huh, TIL!

Then I guess we're good. Thank you @ruuda!

[cipherboy]

LGTM! Thanks @ruuda. Mind merging this one @naphelps?