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

ACL Token ExpirationTTL is broken or not documented properly #15960

Closed
gbolo opened this issue Jan 30, 2023 · 4 comments · Fixed by #15999
Closed

ACL Token ExpirationTTL is broken or not documented properly #15960

gbolo opened this issue Jan 30, 2023 · 4 comments · Fixed by #15999

Comments

@gbolo
Copy link

gbolo commented Jan 30, 2023

Nomad version

Nomad v1.4.3 (f464aca721d222ae9c1f3df643b3c3aaa20e2da7)

Issue

According to the documentation:

Create Token POST /acl/token

Parameters:

  • ExpirationTTL (duration: 0s) - This is a convenience field and if set will initialize the ExpirationTime field to a value of CreateTime + ExpirationTTL

As described above, one would assume that they can use a human readable string such as "1h" or "30m". However, this is not the case. Seems like the nomad API does not properly unmarshal this field into time.Duration. As such, only an integer representing nano seconds seems to be an acceptable value for this field, but that is not documented. Also it is not documented that there is an acceptable range: between 1m0s and 24h0m0s according to the API responses I am receiving when using an integer.

Reproduction steps

wont accept friendly string (despite being documented as a "convenience field" ;)

< POST /v1/acl/token HTTP/1.1
< Host: 127.0.0.1:4646
< User-Agent: Ansible Nomad API Module
< Accept-Encoding: gzip, deflate, br
< Accept: application/json
< Connection: keep-alive
< Content-Type: application/json
< X-Nomad-Token: <REDACTED>
< Content-Length: 98
< 
< {"Name": "gbolo", "Type": "client", "Policies": ["gbolo"], "Global": false, "ExpirationTTL": "1h"}

> HTTP/1.1 400 Bad Request
> Content-Encoding: gzip
> Vary: Accept-Encoding
> Date: Mon, 30 Jan 2023 16:58:03 GMT
> Content-Length: 109
> 
json: cannot unmarshal string into Go struct field ACLToken.ExpirationTTL of type time.Duration

apparently there are also undocumented min/max values for this field:

< POST /v1/acl/token HTTP/1.1
< Host: 127.0.0.1:4646
< User-Agent: Ansible Nomad API Module
< Accept-Encoding: gzip, deflate, br
< Accept: application/json
< Connection: keep-alive
< Content-Type: application/json
< X-Nomad-Token: <REDACTED>
< Content-Length: 101
< 
< {"Name": "gbolo", "Type": "client", "Policies": ["gbolo"], "Global": false, "ExpirationTTL": 1000000}

> HTTP/1.1 400 Bad Request
> Content-Encoding: gzip
> Vary: Accept-Encoding
> Date: Mon, 30 Jan 2023 17:00:11 GMT
> Content-Length: 110
> 
token 0 invalid: 1 error occurred:
        * expiration time cannot be less than 1m0s in the future (was 1ms)
< POST /v1/acl/token HTTP/1.1
< Host: 127.0.0.1:4646
< User-Agent: Ansible Nomad API Module
< Accept-Encoding: gzip, deflate, br
< Accept: application/json
< Connection: keep-alive
< Content-Type: application/json
< X-Nomad-Token: <REDACTED>
< Content-Length: 109
< 
< {"Name": "gbolo", "Type": "client", "Policies": ["gbolo"], "Global": false, "ExpirationTTL": 100000000000000}

> HTTP/1.1 400 Bad Request
> Content-Encoding: gzip
> Vary: Accept-Encoding
> Date: Mon, 30 Jan 2023 17:01:15 GMT
> Content-Length: 118
> 
token 0 invalid: 1 error occurred:
        * expiration time cannot be more than 24h0m0s in the future (was 27h46m40s)

Expected Result

I expect that the friendly strings should work, or that it is documented that this field should be an integer that represents how many nano seconds in the future that this token will be valid for. Also please document that acceptable range.

@gbolo gbolo added the type/bug label Jan 30, 2023
@jrasell jrasell self-assigned this Jan 30, 2023
@jrasell
Copy link
Member

jrasell commented Jan 30, 2023

Hi @gbolo and thanks for raising this issue.

I expect that the friendly strings should work

This is how it should be working, so I'll take a look into this along with improving the documentation around min/max values.

@jrasell jrasell added this to Needs Triage in Nomad - Community Issues Triage via automation Jan 30, 2023
@jrasell jrasell moved this from Needs Triage to Triaging in Nomad - Community Issues Triage Jan 30, 2023
@gbolo
Copy link
Author

gbolo commented Jan 30, 2023

Thanks @jrasell

Would you mind if I also take a stab at this with you. Thinking it might be a good opportunity for a me to make my first PR to this project, as I been using nomad for about a year now and enjoy writing go code.

@jrasell
Copy link
Member

jrasell commented Jan 30, 2023

Absolutely and thank you!

I can't assign the ticket to you as you're not an internal member, so I'll keep it self assigned. If you have any questions feel free to ping me and I'll try to respond quickly. As an initial pointer, I believe on main there is code we use for AuthMethod marshal/unmarshl functionality that needs to be replicated for this problem. This can be seen within the internal structs code as well as the public API SDK

@gbolo
Copy link
Author

gbolo commented Jan 30, 2023

Absolutely and thank you!

I can't assign the ticket to you as you're not an internal member, so I'll keep it self assigned. If you have any questions feel free to ping me and I'll try to respond quickly. As an initial pointer, I believe on main there is code we use for AuthMethod marshal/unmarshl functionality that needs to be replicated for this problem. This can be seen within the internal structs code as well as the public API SDK

Ahh I see, seems like this PR may need a more significant amount of work then I have initially anticipated (may not be a good fit for my first PR here). My apologies for flip flopping on this @jrasell

Please carry on with your approach and I will stay tuned for the solution to this to familiarise myself with the code base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging a pull request may close this issue.

2 participants