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

catsby · 2018-08-16T14:54:21Z

Continuing #4361 to allow users to supply a custom max length for Nomad credential token names. For Nomad version 0.8.3 and before, the max length for the name of the token is 64 characters. With Nomad 0.8.4+, the max length is now 256.

Here we apply a default max length of 256, but allow users to override that with either the max_token_length attribute, or using the NOMAD_MAX_TOKEN_LENGTH environment variable.

NOTE: I've switched the tests to use my fork of the Dockerized Nomad at catsby/nomad on docker hub. The current version is 0.8.3 and I needed 0.8.4 to support the new max length.

Updating the docs now and will push when they are done. Opening this PR now for early review.

chrishoffman · 2018-08-16T14:56:00Z

builtin/logical/nomad/path_config_access.go

+			"max_token_length": &framework.FieldSchema{
+				Type:        framework.TypeInt,
+				Description: "Max length for generated Nomad tokens",
+				// Default length is 256 as of


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

chrishoffman · 2018-08-16T14:57:19Z

builtin/logical/nomad/path_config_access.go

@@ -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")


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.

chrishoffman · 2018-08-16T14:59:29Z

builtin/logical/nomad/path_creds_create.go

+	conf, _ := b.readConfigAccess(ctx, req.Storage)
+	// establish a default
+	tokenNameLength := maxTokenNameLength
+	if conf != nil {


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

chrishoffman · 2018-08-16T15:00:13Z

builtin/logical/nomad/path_creds_create.go

+	// of the pseudo randomized token name is the part that gets trimmed off,
+	// weaking it's randomness.
+	if len(tokenName) > tokenNameLength {
+		tokenName = tokenName[0:tokenNameLength]


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

chrishoffman · 2018-08-16T15:00:41Z

builtin/logical/nomad/path_config_access.go

@@ -23,6 +26,14 @@ func pathConfigAccess(b *backend) *framework.Path {
 				Type:        framework.TypeString,
 				Description: "Token for API calls",
 			},
+
+			"max_token_length": &framework.FieldSchema{


Maybe this parameter should be named max_token_name_length.

chrishoffman · 2018-08-16T15:01:02Z

builtin/logical/nomad/path_creds_create.go

@@ -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


You can remove this comment too.

kalafut · 2018-08-16T15:09:43Z

builtin/logical/nomad/backend_test.go

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


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.

kalafut · 2018-08-16T15:14:35Z

builtin/logical/nomad/backend_test.go

+		{
+			title:           "ConfigOverride-LongName-envtrim",
+			roleName:        "testlongerrolenametoexceed64charsdddddddddddddddddddddddd",
+			envLengthString: "16",


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

kalafut · 2018-08-16T15:55:01Z

builtin/logical/nomad/path_creds_create.go

-	// https://github.com/hashicorp/nomad/blob/d9276e22b3b74674996fb548cdb6bc4c70d5b0e4/nomad/structs/structs.go#L115
-	if len(tokenName) > 64 {
-		tokenName = tokenName[0:63]
+	// Note: if the given role name is suffeciently long, the UnixNano() portion


typo: suffeciently

kalafut · 2018-08-16T15:55:22Z

builtin/logical/nomad/path_creds_create.go

-		tokenName = tokenName[0:63]
+	// Note: if the given role name is suffeciently long, the UnixNano() portion
+	// of the pseudo randomized token name is the part that gets trimmed off,
+	// weaking it's randomness.


typo: weakening

kalafut

Minor typos, otherwise LGTM!

catsby · 2018-08-16T16:00:28Z

Test results:

[vault][nomad-token-length](3)$ make testacc TEST=./builtin/logical/nomad TESTARGS="-run=TestBackend"                                                                                                                                                      ✭
==> Checking that code complies with gofmt requirements...
==> Checking that build is using go version >= 1.10...
go generate
VAULT_ACC=1 go test -tags='' ./builtin/logical/nomad -v -run=TestBackend -timeout=45m
=== RUN   TestBackend_config_access
--- PASS: TestBackend_config_access (6.55s)
        backend_test.go:68: [WARN] Generated Master token: cdb66d87-2d6b-6858-ea0a-26eb60158a32
=== RUN   TestBackend_renew_revoke
--- PASS: TestBackend_renew_revoke (7.27s)
        backend_test.go:68: [WARN] Generated Master token: 96eaaa95-c799-5b42-861f-699cee6d3aa8
        backend_test.go:216: [WARN] Generated token: e0a6e441-b8a0-c384-1a02-73e5ce119333 with accessor 145d3551-f5bc-6dbc-9b9c-acc97a15ca15
        backend_test.go:227: [WARN] Verifying that the generated token works...
        backend_test.go:259: [WARN] Verifying that the generated token does not exist...
=== RUN   TestBackend_CredsCreateEnvVar
--- PASS: TestBackend_CredsCreateEnvVar (6.86s)
        backend_test.go:68: [WARN] Generated Master token: bb5a1071-17c1-7dfb-42a8-81019ce1dd39
=== RUN   TestBackend_max_token_name_length
=== RUN   TestBackend_max_token_name_length/Default
=== RUN   TestBackend_max_token_name_length/ConfigOverride
=== RUN   TestBackend_max_token_name_length/ConfigOverride-LongName
=== RUN   TestBackend_max_token_name_length/ConfigOverride-LongName-notrim
--- PASS: TestBackend_max_token_name_length (7.77s)
        backend_test.go:68: [WARN] Generated Master token: ffef4d75-3992-1986-fd8e-e95ef719b93a
    --- PASS: TestBackend_max_token_name_length/Default (0.21s)
    --- PASS: TestBackend_max_token_name_length/ConfigOverride (0.21s)
    --- PASS: TestBackend_max_token_name_length/ConfigOverride-LongName (0.21s)
    --- PASS: TestBackend_max_token_name_length/ConfigOverride-LongName-notrim (0.21s)
PASS
ok      github.com/hashicorp/vault/builtin/logical/nomad        (cached)

chrishoffman · 2018-08-16T16:04:57Z

builtin/logical/nomad/path_config_access.go

+			"max_token_name_length": &framework.FieldSchema{
+				Type:        framework.TypeInt,
+				Description: "Max length for name of generated Nomad tokens",
+				Default:     maxTokenNameLength,


One more thing here. You should probably not set the default value in the path config. This will mean we will store this value in storage and if we change the default again, they will have an explicit value. You can just omit the Default.

Omitting the default will leave a zero value in the struct and be serialized like that. We could change the accessConfig.MaxTokenNameLength to be an *int and use nil there, or just ignore the value when it's zero. I'll assume the later for now

Or set it the default to -1 maybe?

I think the zero value is fine, it is the same as default. I don't think we need to do any int pointer work. Does this api support create vs update? If so, you should be able to handle the case where they don't provide the value and just patch the updated fields.

burdandrei and others added 3 commits August 14, 2018 16:22

Nomad: updating max token length to 256

b9aa628

Initial support for supporting custom max token name length for Nomad

b1ad204

simplify/correct tests

3bd07ee

chrishoffman added this to the 0.11 milestone Aug 16, 2018

chrishoffman reviewed Aug 16, 2018

View reviewed changes

kalafut reviewed Aug 16, 2018

View reviewed changes

document nomad max_token_name_length

024fc96

kalafut reviewed Aug 16, 2018

View reviewed changes

catsby added 3 commits August 16, 2018 10:26

removed support for max token length env var. Rename field for clarity

73e77f4

cleanups after removing env var support

aba9594

move RandomWithPrefix to testhelpers

ea46e62

kalafut reviewed Aug 16, 2018

View reviewed changes

kalafut previously approved these changes Aug 16, 2018

View reviewed changes

fix spelling

3c3aa32

catsby dismissed kalafut’s stale review via 3c3aa32 August 16, 2018 15:59

chrishoffman reviewed Aug 16, 2018

View reviewed changes

catsby added 2 commits August 16, 2018 11:59

Remove default 256 value. Use zero as a sentinel value and ignore it

5e54700

update docs

39ff353

chrishoffman approved these changes Aug 16, 2018

View reviewed changes

chrishoffman merged commit ca1e2a7 into master Aug 16, 2018

chrishoffman mentioned this pull request Aug 16, 2018

Nomad: updating max token length to 256 #4361

Closed

kalafut deleted the nomad-token-length branch December 7, 2021 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

catsby commented Aug 16, 2018 •

edited

Loading

chrishoffman Aug 16, 2018

chrishoffman Aug 16, 2018

chrishoffman Aug 16, 2018

chrishoffman Aug 16, 2018

chrishoffman Aug 16, 2018

chrishoffman Aug 16, 2018

kalafut Aug 16, 2018

kalafut Aug 16, 2018

kalafut Aug 16, 2018

kalafut Aug 16, 2018

kalafut left a comment

catsby commented Aug 16, 2018

chrishoffman Aug 16, 2018

catsby Aug 16, 2018

chrishoffman Aug 16, 2018

chrishoffman Aug 16, 2018

		@@ -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

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

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

Conversation

catsby commented Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalafut left a comment

Choose a reason for hiding this comment

catsby commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catsby commented Aug 16, 2018 •

edited

Loading