VAULT-12564 Add new token_file auto-auth method

[VioletHynes]

This auth method doesn't actually authenticate, but instead validates that the token it has is valid by doing a lookup. This means that token authentication failures will happen as part of the auth step, instead of the auth step blindly ignoring incorrect tokens. Note that lookup-self is part of the default policy.

We do kind of abuse the auto-auth framework here with this method, but it's really the only way to get the effect that we want (auto-auth without actually authing, and using a token from a previous auth). I think the approach I've taken makes the most sense (and requires the least hackiness), but I'm open to other ideas. I think regardless of what we do, we'll need some level of exceptional behaviour for this auth method, due to how it is.

Renewing works fine, e.g.

2023-01-16T13:54:09.579-0500 [INFO]  vault-agent.auth.handler: renewed auth token
2023-01-16T13:54:52.166-0500 [INFO]  vault-agent.auth.handler: renewed auth token
2023-01-16T13:55:34.754-0500 [INFO]  vault-agent.auth.handler: renewed auth token

Docs will be part of another PR, just for ease of review.

[VioletHynes]

I would have more preferably checked if the auth method was in fact TokenFileMethod, but doing so would have caused a circular dependency. I'm open to better ways, if folks can think of any.

[ncabatoff]

You could add a new method to the AuthMethod interface, e.g. IsStaticCredentials. Or equivalently, you could introduce a new sub-interface, like

type TokenSource interface {
	Authenticate(context.Context, *api.Client) (string, http.Header, map[string]interface{}, error)
}

type AuthMethod interface {
        TokenSource
	NewCreds() chan struct{}
	CredSuccess()
	Shutdown()
}

Going further, we could have Authenticate do the request itself, instead of returning a path and header and data body, meaning

type TokenSource interface {
	Authenticate(context.Context, *api.Client) (*api.Secret, error)
}

This seems like a more natural and less cumbersome approach than the current Authenticate method signature.

[VioletHynes]

I did think about adding to the AuthMethod interface (a few times, actually, I had also toyed with AuthMethod having a new method for getting the important bits out of the returned *api.Secret, as well as this use-case), but it seemed strange, in that I'd expect that only tokenFileMethod would ever be different. It's unclear which is cleaner, to me, and both, to me, feel a little inelegant, just in different ways. If you prefer extending AuthMethod in this case, I'm happy to change approach!

[ncabatoff]

I don't have a problem with your current approach. My proposal is more disruptive and not that much better, so if it doesn't speak to you feel free to ignore it.

[peteski22]

Just to add my thoughts to this quickly:

I don't have a concrete suggestion as such to offer right now (sorry), but it feels like we're maybe doing something that's certainly workable, but perhaps doesn't follow existing ways we might be encapsulating (whatever the Go term is, modularisation?) the behaviour etc. and it bleeds into a place it might not need to be.

In the absence of an example to suggest an immediate alternative, I fall back to thinking, are we showing developer empathy, would this code be easy to maintain for the team, should we be trying to refactor it while we're in the code for this work item?

I ran the gocyclo and gocognit tools for the auth.go file and I think we're in danger of making code even harder to understand :(

auth.go/Run:

• cyclo: 54 => 67
• cognit: 136 => 184

As long as we're making a balanced judgement on this overall, I'm happy but wanted to share in case it helped. :)

[VioletHynes]

Yeah, an unfortunate implication is this feature is that it adds unavoidable complexity to Authenticate, as it needs to handle the case where:

• It authenticates
• (new!) It validates a previous authentication

There's downsides to either approach, but the reason I took the one I did was that I believe this approach minimizes complexity when adding new (or maintaining old) auto-auth methods to agent, which I believe is something we'll need to do/maintain more than this method here. I could be wrong, to be clear, but I wanted to be a bit clearer as to why I thought this was the best place to shoulder the complexity burden, and I'm still happy to change if folks feel like this isn't the best place to bear the complexity.

I appreciate you bringing it up, though, it's good to bring to the forefront of all of our minds, and think about.

[alviss7]

Hello,

Thank you very much for this PR, it's a feature I was looking forward to. I don't think I have the technical skills to do code reviews, but I tested it on my side, I didn't find any bug.

[ncabatoff]

You could add a new method to the AuthMethod interface, e.g. IsStaticCredentials. Or equivalently, you could introduce a new sub-interface, like

type TokenSource interface {
	Authenticate(context.Context, *api.Client) (string, http.Header, map[string]interface{}, error)
}

type AuthMethod interface {
        TokenSource
	NewCreds() chan struct{}
	CredSuccess()
	Shutdown()
}

Going further, we could have Authenticate do the request itself, instead of returning a path and header and data body, meaning

type TokenSource interface {
	Authenticate(context.Context, *api.Client) (*api.Secret, error)
}

This seems like a more natural and less cumbersome approach than the current Authenticate method signature.

[peteski22]

Looks good to me from a high level, Nick already shared some useful thoughts and catches. :)

I added one or two comments, I was trying to be helpful but if they aren't ... sorry!

I think there are some bits you're still tweaking but when you're ready, it's approved.

[peteski22]

Just wondering if this would be more in-keeping if it was camelCased like tokenFile not important just in case it was something you think looks neater.

[peteski22]

Just to add my thoughts to this quickly:

I don't have a concrete suggestion as such to offer right now (sorry), but it feels like we're maybe doing something that's certainly workable, but perhaps doesn't follow existing ways we might be encapsulating (whatever the Go term is, modularisation?) the behaviour etc. and it bleeds into a place it might not need to be.

In the absence of an example to suggest an immediate alternative, I fall back to thinking, are we showing developer empathy, would this code be easy to maintain for the team, should we be trying to refactor it while we're in the code for this work item?

I ran the gocyclo and gocognit tools for the auth.go file and I think we're in danger of making code even harder to understand :(

auth.go/Run:

• cyclo: 54 => 67
• cognit: 136 => 184

As long as we're making a balanced judgement on this overall, I'm happy but wanted to share in case it helped. :)

[VioletHynes]

If you're looking for a preview of the docs, I opened a draft PR here: #18783

I'll mark it as ready to review once I merge this one :)