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

[WIP] Support custom max Nomad token name length [supersedes https://github.com/hashicorp/vault/pull/4361] #5117

Merged
merged 10 commits into from
Aug 16, 2018
7 changes: 7 additions & 0 deletions builtin/logical/nomad/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ import (
"github.com/hashicorp/vault/logical/framework"
)

// maxTokenNameLength is the maximum length for the name of a Nomad access
// token
const maxTokenNameLength = 256

// Factory returns a Nomad backend that satisfies the logical.Backend interface
func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, error) {
b := Backend()
if err := b.Setup(ctx, conf); err != nil {
Expand All @@ -16,6 +21,7 @@ func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend,
return b, nil
}

// Backend returns the configured Nomad backend
func Backend() *backend {
var b backend
b.Backend = &framework.Backend{
Expand Down Expand Up @@ -66,5 +72,6 @@ func (b *backend) client(ctx context.Context, s logical.Storage) (*api.Client, e
if err != nil {
return nil, err
}

return client, nil
}
187 changes: 184 additions & 3 deletions builtin/logical/nomad/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ package nomad
import (
"context"
"fmt"
"math/rand"
"os"
"reflect"
"strconv"
"testing"
"time"

Expand All @@ -14,6 +16,18 @@ import (
"github.com/ory/dockertest"
)

// randomWithPrefix is used to generate a unique name with a prefix, for
// randomizing names in acceptance tests
func randomWithPrefix(name string) string {
reseed()
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reseed function for? It is seeding the global rand source, but then you're using a new random source in your Sprintf. You can alternatively also use an existing utility like base62.Random() to generate random strings.

return fmt.Sprintf("%s-%d", name, rand.New(rand.NewSource(time.Now().UnixNano())).Int())
}

// Seeds random with current timestamp
func reseed() {
rand.Seed(time.Now().UTC().UnixNano())
}

func prepareTestContainer(t *testing.T) (cleanup func(), retAddress string, nomadToken string) {
nomadToken = os.Getenv("NOMAD_TOKEN")

Expand All @@ -29,8 +43,8 @@ func prepareTestContainer(t *testing.T) (cleanup func(), retAddress string, noma
}

dockerOptions := &dockertest.RunOptions{
Repository: "djenriquez/nomad",
Tag: "latest",
Repository: "catsby/nomad",
Tag: "0.8.4",
Cmd: []string{"agent", "-dev"},
Env: []string{`NOMAD_LOCAL_CONFIG=bind_addr = "0.0.0.0" acl { enabled = true }`},
}
Expand Down Expand Up @@ -142,7 +156,8 @@ func TestBackend_config_access(t *testing.T) {
}

expected := map[string]interface{}{
"address": connData["address"].(string),
"address": connData["address"].(string),
"max_token_length": maxTokenNameLength,
}
if !reflect.DeepEqual(expected, resp.Data) {
t.Fatalf("bad: expected:%#v\nactual:%#v\n", expected, resp.Data)
Expand Down Expand Up @@ -300,3 +315,169 @@ func TestBackend_CredsCreateEnvVar(t *testing.T) {
t.Fatalf("resp is error: %v", resp.Error())
}
}

func TestBackend_max_token_length(t *testing.T) {
config := logical.TestBackendConfig()
config.StorageView = &logical.InmemStorage{}
b, err := Factory(context.Background(), config)
if err != nil {
t.Fatal(err)
}

cleanup, connURL, connToken := prepareTestContainer(t)
defer cleanup()

testCases := []struct {
title string
roleName string
tokenLength int
envLengthString string
}{
{
title: "Default",
},
{
title: "ConfigOverride",
tokenLength: 64,
},
{
title: "ConfigOverride-LongName",
roleName: "testlongerrolenametoexceed64charsdddddddddddddddddddddddd",
tokenLength: 64,
},
{
title: "ConfigOverride-LongName-notrim",
roleName: "testlongerrolenametoexceed64charsdddddddddddddddddddddddd",
},
{
title: "ConfigOverride-LongName-envtrim",
roleName: "testlongerrolenametoexceed64charsdddddddddddddddddddddddd",
envLengthString: "16",
Copy link
Contributor

Choose a reason for hiding this comment

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

Some good additional test cases would be this envvar being out of range or not a valid integer.

},
}

for _, tc := range testCases {
t.Run(tc.title, func(t *testing.T) {
// setup config/access
connData := map[string]interface{}{
"address": connURL,
"token": connToken,
}
expected := map[string]interface{}{
"address": connURL,
"max_token_length": maxTokenNameLength,
}

expectedTokenNameLength := maxTokenNameLength

if tc.tokenLength != 0 {
connData["max_token_length"] = tc.tokenLength
expected["max_token_length"] = tc.tokenLength
expectedTokenNameLength = tc.tokenLength
}

if tc.envLengthString != "" {
os.Setenv("NOMAD_MAX_TOKEN_LENGTH", tc.envLengthString)
defer os.Unsetenv("NOMAD_MAX_TOKEN_LENGTH")
i, _ := strconv.Atoi(tc.envLengthString)
expected["max_token_length"] = i
expectedTokenNameLength = i
}

confReq := logical.Request{
Operation: logical.UpdateOperation,
Path: "config/access",
Storage: config.StorageView,
Data: connData,
}

resp, err := b.HandleRequest(context.Background(), &confReq)
if err != nil || (resp != nil && resp.IsError()) || resp != nil {
t.Fatalf("failed to write configuration: resp:%#v err:%s", resp, err)
}
confReq.Operation = logical.ReadOperation
resp, err = b.HandleRequest(context.Background(), &confReq)
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("failed to write configuration: resp:%#v err:%s", resp, err)
}

// verify token length is returned in the config/access query
if !reflect.DeepEqual(expected, resp.Data) {
t.Fatalf("bad: expected:%#v\nactual:%#v\n", expected, resp.Data)
}
// verify token is not returned
if resp.Data["token"] != nil {
t.Fatalf("token should not be set in the response")
}

// create a role to create nomad credentials with
// Seeds random with current timestamp

if tc.roleName == "" {
tc.roleName = "test"
}
roleTokenName := randomWithPrefix(tc.roleName)

confReq.Path = "role/" + roleTokenName
confReq.Operation = logical.UpdateOperation
confReq.Data = map[string]interface{}{
"policies": []string{"policy"},
"lease": "6h",
}
resp, err = b.HandleRequest(context.Background(), &confReq)
if err != nil {
t.Fatal(err)
}

confReq.Operation = logical.ReadOperation
confReq.Path = "creds/" + roleTokenName
resp, err = b.HandleRequest(context.Background(), &confReq)
if err != nil {
t.Fatal(err)
}
if resp == nil {
t.Fatal("resp nil")
}
if resp.IsError() {
t.Fatalf("resp is error: %v", resp.Error())
}

// extract the secret, so we can query nomad directly
generatedSecret := resp.Secret
generatedSecret.TTL = 6 * time.Hour

var d struct {
Token string `mapstructure:"secret_id"`
Accessor string `mapstructure:"accessor_id"`
}
if err := mapstructure.Decode(resp.Data, &d); err != nil {
t.Fatal(err)
}

// Build a client and verify that the credentials work
nomadapiConfig := nomadapi.DefaultConfig()
nomadapiConfig.Address = connData["address"].(string)
nomadapiConfig.SecretID = d.Token
client, err := nomadapi.NewClient(nomadapiConfig)
if err != nil {
t.Fatal(err)
}

// default query options for Nomad queries ... not sure if needed
qOpts := &nomadapi.QueryOptions{
Namespace: "default",
}

// connect to Nomad and verify the token name does not exceed the
// max_token_length
token, _, err := client.ACLTokens().Self(qOpts)
if err != nil {
t.Fatal(err)
}

if len(token.Name) > expectedTokenNameLength {
t.Fatalf("token name exceeds max length (%d): %s (%d)", expectedTokenNameLength, token.Name, len(token.Name))
}
})
}
}
33 changes: 30 additions & 3 deletions builtin/logical/nomad/path_config_access.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ package nomad

import (
"context"
"log"
"os"
"strconv"

"github.com/hashicorp/errwrap"
"github.com/hashicorp/vault/logical"
Expand All @@ -23,6 +26,14 @@ func pathConfigAccess(b *backend) *framework.Path {
Type: framework.TypeString,
Description: "Token for API calls",
},

"max_token_length": &framework.FieldSchema{
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this parameter should be named max_token_name_length.

Type: framework.TypeInt,
Description: "Max length for generated Nomad tokens",
// Default length is 256 as of
Copy link
Contributor

Choose a reason for hiding this comment

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

I would leave these comments out since they can get out of date pretty quickly.

// https://github.com/hashicorp/nomad/blob/21682427f3474f92cc589832efe72850a61c83a7/nomad/structs/structs.go#L116
Default: maxTokenNameLength,
},
},

Callbacks: map[logical.Operation]framework.OperationFunc{
Expand Down Expand Up @@ -73,7 +84,8 @@ func (b *backend) pathConfigAccessRead(ctx context.Context, req *logical.Request

return &logical.Response{
Data: map[string]interface{}{
"address": conf.Address,
"address": conf.Address,
"max_token_length": conf.MaxTokenLength,
},
}, nil
}
Expand All @@ -96,6 +108,20 @@ func (b *backend) pathConfigAccessWrite(ctx context.Context, req *logical.Reques
conf.Token = token.(string)
}

// max_token_length has default of 256
conf.MaxTokenLength = data.Get("max_token_length").(int)
envMaxTokenLength := os.Getenv("NOMAD_MAX_TOKEN_LENGTH")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need an environment variable for this value. These are usually reserved for sensitive values or cloud SDK symmetry and most things should just get store directly in barrier storage.

if envMaxTokenLength != "" {
// if we find NOMAD_MAX_max_token_length in the env and can parse it, override
// the default length
i, err := strconv.Atoi(envMaxTokenLength)
if err != nil {
log.Printf("[WARN] error parsing NOMAD_MAX_TOKEN_LENGTH, using default 256")
} else {
conf.MaxTokenLength = i
}
}

entry, err := logical.StorageEntryJSON("config/access", conf)
if err != nil {
return nil, err
Expand All @@ -115,6 +141,7 @@ func (b *backend) pathConfigAccessDelete(ctx context.Context, req *logical.Reque
}

type accessConfig struct {
Address string `json:"address"`
Token string `json:"token"`
Address string `json:"address"`
Token string `json:"token"`
MaxTokenLength int `json:"max_token_length"`
}
13 changes: 11 additions & 2 deletions builtin/logical/nomad/path_creds_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ func pathCredsCreate(b *backend) *framework.Path {

func (b *backend) pathTokenRead(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
name := d.Get("name").(string)
conf, _ := b.readConfigAccess(ctx, req.Storage)
// establish a default
tokenNameLength := maxTokenNameLength
if conf != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should only set the value to the config value if it is greater than zero since that will be the default when deserializing.

tokenNameLength = conf.MaxTokenLength
}

role, err := b.Role(ctx, req.Storage, name)
if err != nil {
Expand Down Expand Up @@ -58,8 +64,11 @@ func (b *backend) pathTokenRead(ctx context.Context, req *logical.Request, d *fr

// Handling nomad maximum token length
// https://github.com/hashicorp/nomad/blob/d9276e22b3b74674996fb548cdb6bc4c70d5b0e4/nomad/structs/structs.go#L115
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this comment too.

if len(tokenName) > 64 {
tokenName = tokenName[0:63]
// Note: if the given role name is suffeciently long, the UnixNano() portion
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: suffeciently

// of the pseudo randomized token name is the part that gets trimmed off,
// weaking it's randomness.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: weakening

if len(tokenName) > tokenNameLength {
tokenName = tokenName[0:tokenNameLength]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the more idiomatic way of doing this is tokenName[:tokenNameLength]

}

// Create it
Expand Down