Allow setting a token name template on auth methods

[jorgemarey]

Fixes #19134 by allowing the set a token-name-template when creating auth methods

Documentation is pending this is set given an OK.

As an example. Currently this is the nomad auth:

❯ nomad acl auth-method create -name="test" -type="JWT" -max-token-ttl="24h" -token-locality=global -default=true -config "@auth-method.json"
Name              = test
Type              = JWT
Locality          = global
MaxTokenTTL       = 24h0m0s
Default           = true
Create Index      = 12
Modify Index      = 12
...

❯ nomad acl binding-rule create -description "test biding jwt" -auth-method "test"  -bind-type "management" -selector "\"platform_admin\" in list.groups" 
ID           = ee12f316-bf6f-27da-735c-a32ddd6465d0
Description  = test biding jwt
Auth Method  = test
Selector     = "\"platform_admin\" in list.groups"
Bind Type    = management
Bind Name    = <none>
Create Time  = 2023-11-20 10:24:25.875134058 +0000 UTC
Modify Time  = 2023-11-20 10:24:25.875134058 +0000 UTC
Create Index = 15
Modify Index = 15

❯ nomad login -login-token ${LOGIN_JWT}
Successfully logged in via JWT and test

Accessor ID  = 069b1604-f831-47d4-a98d-6609982364eb
Secret ID    = 46203a0a-8914-29b1-e45d-8b6a7dbd1490
Name         = JWT-test
Type         = management
Global       = true
Create Time  = 2023-11-20 10:24:55.436962476 +0000 UTC
Expiry Time  = 2023-11-21 10:24:55.436962476 +0000 UTC
Create Index = 17
Modify Index = 17
Policies     = n/a
Roles        = n/a

❯ nomad acl token list                 
Name             Type        Global  Accessor ID                           Expired
Bootstrap Token  management  true    1b470500-f53b-a69f-7eac-7c1db0ff02ee  false
JWT-test         management  true    069b1604-f831-47d4-a98d-6609982364eb  false

With this change users can do the following:

❯ nomad acl auth-method create -name="test" -type="JWT" -max-token-ttl="24h" -token-locality=global -token-name-format="{{ .auth_type }}-{{ .auth_name }}-{{ .claims.user }}-{{ .timestamp }}" -default=true -config "@auth-method.json"                       
Name              = test
Type              = JWT
Locality          = global
MaxTokenTTL       = 24h0m0s
Token name Format = {{ .auth_type }}-{{ .auth_name }}-{{ .claims.user }}-{{ .timestamp }}
Default           = true
Create Index      = 25
Modify Index      = 25
....

❯ nomad acl binding-rule create -description "test biding jwt" -auth-method "test"  -bind-type "management" -selector "\"platform_admin\" in list.groups" 
ID           = e6cb3954-d24c-0703-1336-4c79011ce7b3
Description  = test biding jwt
Auth Method  = test
Selector     = "\"platform_admin\" in list.groups"
Bind Type    = management
Bind Name    = <none>
Create Time  = 2023-11-20 10:30:02.846156597 +0000 UTC
Modify Time  = 2023-11-20 10:30:02.846156597 +0000 UTC
Create Index = 26
Modify Index = 26

❯ nomad login -login-token ${LOGIN_JWT}
Successfully logged in via JWT and test

Accessor ID  = 22bba409-930e-6eed-e30c-82905d877883
Secret ID    = 2973edc7-2a9b-ab83-56d4-e3fbfd1d1a1c
Name         = JWT-test-E048561-1700476205372278738
Type         = management
Global       = true
Create Time  = 2023-11-20 10:30:05.372329569 +0000 UTC
Expiry Time  = 2023-11-21 10:30:05.372329569 +0000 UTC
Create Index = 27
Modify Index = 27
Policies     = n/a
Roles        = n/a
~ 
❯ nomad acl token list
Name                                  Type        Global  Accessor ID                           Expired
Bootstrap Token                       management  true    1b470500-f53b-a69f-7eac-7c1db0ff02ee  false
JWT-test-JohnDoe-1700476205372278738  management  true    22bba409-930e-6eed-e30c-82905d877883  false

[jrasell]

Hi @jorgemarey and thanks for raising this PR, it's looking great. I have taken a first glance over the approach and wanted to discuss a potential change before diving into a full code review. Please let me know if you want me to take over any aspects of the work, which I would be happy to do to get this merged.

This approach uses Go template syntax, however, we also have the option of HIL available which is also used within the binding rule functionality. We could use this existing function which would move the token name format in your example to look something like ${auth_type}-${auth_name}-${claims.user}-${timestamp}.

I would also like to keep the number of "special" variables down to a minimum and remove the timestamp. ACL tokens already have a creation time parameter, so we should refer to this as the canonical way to view creation times.

It would be nice to get some tests added which exercise the new functionality, so we can ensure we do not break it when making changes in the future.

Thanks also for ensuring the current behaviour is maintained when the user does not supply a format!

[jorgemarey]

Hi @jrasell.

Hi @jorgemarey and thanks for raising this PR, it's looking great. I have taken a first glance over the approach and wanted to discuss a potential change before diving into a full code review. Please let me know if you want me to take over any aspects of the work, which I would be happy to do to get this merged.

I can do this, no problem.

This approach uses Go template syntax, however, we also have the option of HIL available which is also used within the binding rule functionality. We could use this existing function which would move the token name format in your example to look something like ${auth_type}-${auth_name}-${claims.user}-${timestamp}.

Yeah, that makes sense, I'll change the template to a HIL format

I would also like to keep the number of "special" variables down to a minimum and remove the timestamp. ACL tokens already have a creation time parameter, so we should refer to this as the canonical way to view creation times.

OK! I'll remove the timestamp

It would be nice to get some tests added which exercise the new functionality, so we can ensure we do not break it when making changes in the future.

Sure

[jrasell]

This is looking great @jorgemarey and I tested this successfully with Auth0 just now resulting in my tokens being named like OIDC-auth0-jamesrasell using the token name format. Most of the comments and suggestions are nit-picky and style changes.

I did wonder about your thoughts on the HIL rendering failure not being terminal, and rather log the error and fallback to using the OIDC-Auth0 default format? It feels better that formatting the token name doesn't stop ACL logins entirely, when a template syntax error occurs. Open to discuss this point. Thanks again!

[jorgemarey]

Hi @jrasell I made the requested changes.

With respect to the HIL rendering failure, I don't know what to do. On one hand, the operator expects the token to have that format and maybe it's important for them that all tokens have it. On the other hand, not allowing access to someone that has a correct binding just for that don't know if it's the right decision. We could check the HIL in the auth method creation, but that doesn't solve all problems as the claims configured in the name format could not be present in the JWT.

I think that I'm leaning towards let this fail and not login the user (a incorrect bindName will do the same) but I leave the final decision to you, let me know.

[jorgemarey]

Hi @jrasell I made the requested changes.

With respect to the HIL rendering failure, I don't know what to do. On one hand, the operator expects the token to have that format and maybe it's important for them that all tokens have it. On the other hand, not allowing access to someone that has a correct binding just for that don't know if it's the right decision. We could check the HIL in the auth method creation, but that doesn't solve all problems as the claims configured in the name format could not be present in the JWT.

I think that I'm leaning towards let this fail and not login the user (a incorrect bindName will do the same) but I leave the final decision to you, let me know.

[jorgemarey]

Hi again @jrasell I added the changes to the documentation. I don't know if I should change also the auth-method.js files accordingly.

[jrasell]

LGTM, thanks so much for this addition @jorgemarey!

One final item I just thought about; when upgrading a cluster with existing auth methods, the token name format will be an empty string. I think we will need to handle this within the formatting function, so that an empty string is returns a token name that is the default and existing behaviour.

additional thinking

[jorgemarey]

Hi @jrasell,

One final item I just thought about; when upgrading a cluster with existing auth methods, the token name format will be an empty string. I think we will need to handle this within the formatting function, so that an empty string is returns a token name that is the default and existing behaviour.

You're right, I'll make the change for that.

[jorgemarey]

Hi @jrasell I made that change. There's only a thing that I don't know if we should change. If you make a request to the API the token format will be empty if it was created previously, should we change that?

[jrasell]

Hi @jrasell I made that change. There's only a thing that I don't know if we should change. If you make a request to the API the token format will be empty if it was created previously, should we change that?

I am happy with that behaviour existing; if anyone really has a reason to change that, we can do that on a follow up. It also works quite well in the CLI in my opinion, making it obvious there is not format set:

$ nomad acl auth-method info auth0
Name              = auth0
Type              = OIDC
Locality          = global
Max Token TTL     = 10m0s
Token Name Format = <none>
Default           = true
Create Index      = 13
Modify Index      = 13

[jrasell]

LGTM and amazing work @jorgemarey, thanks so much!

I've tested this change in standalone and via an upgrade path to confirm the behaviour works as expected.

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.