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

VAULT-12564 Add new token_file auto-auth method #18740

Merged
merged 14 commits into from
Jan 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/18740.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
agent: Added `token_file` auto-auth configuration to allow using a pre-existing token for Vault Agent.
```
4 changes: 4 additions & 0 deletions command/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (
"sync"
"time"

token_file "github.com/hashicorp/vault/command/agent/auth/token-file"

Choose a reason for hiding this comment

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

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.


ctconfig "github.com/hashicorp/consul-template/config"
"github.com/hashicorp/go-multierror"

Expand Down Expand Up @@ -368,6 +370,8 @@ func (c *AgentCommand) Run(args []string) int {
method, err = kubernetes.NewKubernetesAuthMethod(authConfig)
case "approle":
method, err = approle.NewApproleAuthMethod(authConfig)
case "token_file":
method, err = token_file.NewTokenFileAuthMethod(authConfig)
case "pcf": // Deprecated.
method, err = cf.NewCFAuthMethod(authConfig)
default:
Expand Down
116 changes: 93 additions & 23 deletions command/agent/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ func (ah *AuthHandler) Run(ctx context.Context, am AuthMethod) error {
var path string
var data map[string]interface{}
var header http.Header
var isTokenFileMethod bool

switch am.(type) {
case AuthMethodWithClient:
Expand Down Expand Up @@ -254,9 +255,22 @@ func (ah *AuthHandler) Run(ctx context.Context, am AuthMethod) error {
}

// This should only happen if there's no preloaded token (regular auto-auth login)
// or if a preloaded token has expired and is now switching to auto-auth.
// or if a preloaded token has expired and is now switching to auto-auth.
if secret.Auth == nil {
secret, err = clientToUse.Logical().WriteWithContext(ctx, path, data)
isTokenFileMethod = path == "auth/token/lookup-self"
Copy link
Contributor Author

@VioletHynes VioletHynes Jan 18, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@VioletHynes VioletHynes Jan 18, 2023

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if isTokenFileMethod {
token, _ := data["token"].(string)
lookupSelfClient, err := clientToUse.Clone()
if err != nil {
ah.logger.Error("failed to clone client to perform token lookup")
return err
}
lookupSelfClient.SetToken(token)
secret, err = lookupSelfClient.Auth().Token().LookupSelf()
VioletHynes marked this conversation as resolved.
Show resolved Hide resolved
} else {
secret, err = clientToUse.Logical().WriteWithContext(ctx, path, data)
}

// Check errors/sanity
if err != nil {
ah.logger.Error("error authenticating", "error", err, "backoff", backoffCfg)
Expand All @@ -269,6 +283,8 @@ func (ah *AuthHandler) Run(ctx context.Context, am AuthMethod) error {
}
}

var leaseDuration int

switch {
case ah.wrapTTL > 0:
if secret.WrapInfo == nil {
Expand Down Expand Up @@ -319,28 +335,77 @@ func (ah *AuthHandler) Run(ctx context.Context, am AuthMethod) error {
}

default:
if secret == nil || secret.Auth == nil {
ah.logger.Error("authentication returned nil auth info", "backoff", backoffCfg)
metrics.IncrCounter([]string{"agent", "auth", "failure"}, 1)
// We handle the token_file method specially, as it's the only
// auth method that isn't actually authenticating, i.e. the secret
// returned does not have an Auth struct attached
isTokenFileMethod := path == "auth/token/lookup-self"
if isTokenFileMethod {
// We still check the response of the request to ensure the token is valid
// i.e. if the token is invalid, we will fail in the authentication step
if secret == nil || secret.Data == nil {
ah.logger.Error("token file validation failed, token may be invalid", "backoff", backoffCfg)
metrics.IncrCounter([]string{"agent", "auth", "failure"}, 1)

if backoff(ctx, backoffCfg) {
continue
if backoff(ctx, backoffCfg) {
continue
}
return err
}
return err
}
if secret.Auth.ClientToken == "" {
ah.logger.Error("authentication returned empty client token", "backoff", backoffCfg)
metrics.IncrCounter([]string{"agent", "auth", "failure"}, 1)
token, ok := secret.Data["id"].(string)
if !ok || token == "" {
ah.logger.Error("token file validation returned empty client token", "backoff", backoffCfg)
metrics.IncrCounter([]string{"agent", "auth", "failure"}, 1)

if backoff(ctx, backoffCfg) {
continue
if backoff(ctx, backoffCfg) {
continue
}
return err
}

duration, _ := secret.Data["ttl"].(json.Number).Int64()
leaseDuration = int(duration)
renewable, _ := secret.Data["renewable"].(bool)
secret.Auth = &api.SecretAuth{
ClientToken: token,
LeaseDuration: int(duration),
Renewable: renewable,
}
ah.logger.Info("authentication successful, sending token to sinks")
ah.OutputCh <- token
if ah.enableTemplateTokenCh {
ah.TemplateTokenCh <- token
}

tokenType := secret.Data["type"].(string)
if tokenType == "batch" {
ah.logger.Info("note that this token type is batch, and batch tokens cannot be renewed", "ttl", leaseDuration)
}
} else {
if secret == nil || secret.Auth == nil {
ah.logger.Error("authentication returned nil auth info", "backoff", backoffCfg)
metrics.IncrCounter([]string{"agent", "auth", "failure"}, 1)

if backoff(ctx, backoffCfg) {
continue
}
return err
}
if secret.Auth.ClientToken == "" {
ah.logger.Error("authentication returned empty client token", "backoff", backoffCfg)
metrics.IncrCounter([]string{"agent", "auth", "failure"}, 1)

if backoff(ctx, backoffCfg) {
continue
}
return err
}

leaseDuration = secret.LeaseDuration
ah.logger.Info("authentication successful, sending token to sinks")
ah.OutputCh <- secret.Auth.ClientToken
if ah.enableTemplateTokenCh {
ah.TemplateTokenCh <- secret.Auth.ClientToken
}
return err
}
ah.logger.Info("authentication successful, sending token to sinks")
ah.OutputCh <- secret.Auth.ClientToken
if ah.enableTemplateTokenCh {
ah.TemplateTokenCh <- secret.Auth.ClientToken
}

am.CredSuccess()
Expand All @@ -364,10 +429,15 @@ func (ah *AuthHandler) Run(ctx context.Context, am AuthMethod) error {
return err
}

// Start the renewal process
ah.logger.Info("starting renewal process")
metrics.IncrCounter([]string{"agent", "auth", "success"}, 1)
go watcher.Renew()
// We don't want to trigger the renewal process for tokens with
// unlimited TTL, such as the root token.
if leaseDuration == 0 && isTokenFileMethod {
ncabatoff marked this conversation as resolved.
Show resolved Hide resolved
ah.logger.Info("not starting token renewal process, as token has unlimited TTL")
} else {
ah.logger.Info("starting renewal process")
go watcher.Renew()
}

LifetimeWatcherLoop:
for {
Expand Down
83 changes: 83 additions & 0 deletions command/agent/auth/token-file/token_file.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package token_file

import (
"context"
"errors"
"fmt"
"net/http"
"os"
"strings"

"github.com/hashicorp/go-hclog"
"github.com/hashicorp/vault/api"
"github.com/hashicorp/vault/command/agent/auth"
)

type tokenFileMethod struct {
logger hclog.Logger
mountPath string

cachedToken string
tokenFilePath string
}

func NewTokenFileAuthMethod(conf *auth.AuthConfig) (auth.AuthMethod, error) {
if conf == nil {
return nil, errors.New("empty config")
}
if conf.Config == nil {
return nil, errors.New("empty config data")
}

a := &tokenFileMethod{
logger: conf.Logger,
mountPath: "auth/token",
}

tokenFilePathRaw, ok := conf.Config["token_file_path"]
VioletHynes marked this conversation as resolved.
Show resolved Hide resolved
if !ok {
return nil, errors.New("missing 'token_file_path' value")
}
a.tokenFilePath, ok = tokenFilePathRaw.(string)
if !ok {
return nil, errors.New("could not convert 'token_file_path' config value to string")
}
if a.tokenFilePath == "" {
return nil, errors.New("'token_file_path' value is empty")
}

return a, nil
}

func (a *tokenFileMethod) Authenticate(ctx context.Context, client *api.Client) (string, http.Header, map[string]interface{}, error) {
token, err := os.ReadFile(a.tokenFilePath)
if err != nil {
if a.cachedToken == "" {
return "", nil, nil, fmt.Errorf("error reading token file and no cached token known: %w", err)
}
a.logger.Warn("error reading token file", "error", err)
}
if len(token) == 0 {
if a.cachedToken == "" {
return "", nil, nil, errors.New("token file empty and no cached token known")
}
a.logger.Warn("token file exists but read empty value, re-using cached value")
} else {
a.cachedToken = strings.TrimSpace(string(token))
}

// i.e. auth/token/lookup-self
return fmt.Sprintf("%s/lookup-self", a.mountPath), nil, map[string]interface{}{
"token": a.cachedToken,
}, nil
}

func (a *tokenFileMethod) NewCreds() chan struct{} {
return nil
}

func (a *tokenFileMethod) CredSuccess() {
}

func (a *tokenFileMethod) Shutdown() {
}
81 changes: 81 additions & 0 deletions command/agent/auth/token-file/token_file_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package token_file

import (
"os"
"path/filepath"
"testing"

log "github.com/hashicorp/go-hclog"
"github.com/hashicorp/vault/command/agent/auth"
"github.com/hashicorp/vault/sdk/helper/logging"
)

func TestNewTokenFileAuthMethodEmptyConfig(t *testing.T) {
logger := logging.NewVaultLogger(log.Trace)
_, err := NewTokenFileAuthMethod(&auth.AuthConfig{
Logger: logger.Named("auth.method"),
Config: map[string]interface{}{},
})
if err == nil {
t.Fatal("Expected error due to empty config")
}
}

func TestNewTokenFileEmptyFilePath(t *testing.T) {
logger := logging.NewVaultLogger(log.Trace)
_, err := NewTokenFileAuthMethod(&auth.AuthConfig{
Logger: logger.Named("auth.method"),
Config: map[string]interface{}{
"token_file_path": "",
},
})
if err == nil {
t.Fatalf("Expected error when giving empty file path")
}
}

func TestNewTokenFileAuthenticate(t *testing.T) {
tokenFile, err := os.Create(filepath.Join(t.TempDir(), "token_file"))
tokenFileContents := "super-secret-token"
if err != nil {
t.Fatal(err)
}
tokenFileName := tokenFile.Name()
tokenFile.Close() // WriteFile doesn't need it open
os.WriteFile(tokenFileName, []byte(tokenFileContents), 0o666)
defer os.Remove(tokenFileName)

logger := logging.NewVaultLogger(log.Trace)
am, err := NewTokenFileAuthMethod(&auth.AuthConfig{
Logger: logger.Named("auth.method"),
Config: map[string]interface{}{
"token_file_path": tokenFileName,
},
})
if err != nil {
t.Fatal(err)
}

path, headers, data, err := am.Authenticate(nil, nil)
if err != nil {
t.Fatal(err)
}
if path != "auth/token/lookup-self" {
t.Fatalf("Incorrect path, was %s", path)
}
if headers != nil {
t.Fatalf("Expected no headers, instead got %v", headers)
}
if data == nil {
t.Fatal("Data was nil")
}
tokenDataFromAuthMethod := data["token"].(string)
if tokenDataFromAuthMethod != tokenFileContents {
t.Fatalf("Incorrect token file contents return by auth method, expected %s, got %s", tokenFileContents, tokenDataFromAuthMethod)
}

_, err = os.Stat(tokenFileName)
if err != nil {
t.Fatal("Token file removed")
}
}
Loading