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

identity: do not allow a role's token_ttl to be longer than verification_ttl #12151

Merged
merged 13 commits into from Jul 29, 2021

Conversation

fairclothjm
Copy link
Contributor

@fairclothjm fairclothjm commented Jul 22, 2021

Fixes issue #11441 which reported that OIDC identity tokens can have a TTL longer than their signing key will be visible.

Tests

Test role invalid TTL

Test that an attempt to create a new role checks that the token ttl is not greater than the key's verification_ttl

# set verification_ttl=1m
vault write identity/oidc/key/a-key rotation_period=1m verification_ttl=1m allowed_client_ids="*"

# try to create a role with ttl=60m -- Fails
vault write identity/oidc/role/a-role key=a-key ttl=60m

Error:

Error writing data to identity/oidc/role/a-role: Error making API request.

URL: PUT http://127.0.0.1:8200/v1/identity/oidc/role/a-role
Code: 400. Errors:

* a role's token_ttl cannot be longer than the verification_ttl of the key it references

Test key invalid TTL

Test that an update to an existing key that is referenced by a role cannot have its verification_ttl exceed the role's ttl

# set verification_ttl=4m
vault write identity/oidc/key/a-key rotation_period=1m verification_ttl=4m allowed_client_ids="*"

# create a role with ttl=4m -- Success
vault write identity/oidc/role/a-role key=a-key ttl=4m

# update key's verification_ttl=2m -- Fails
vault write identity/oidc/key/a-key rotation_period=1m verification_ttl=2m allowed_client_ids="*"

Error:

Error writing data to identity/oidc/key/a-key: Error making API request.

URL: PUT http://127.0.0.1:8200/v1/identity/oidc/key/a-key
Code: 400. Errors:

* unable to update key "a-key" because it is currently referenced by one or more roles with a token_ttl greater than 120 seconds

When updating a key, ensure any roles referencing the key do not already
have a token_ttl greater than the key's verification_ttl
@vercel vercel bot temporarily deployed to Preview – vault July 22, 2021 19:49 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 22, 2021 19:49 Inactive
changelog/12151.txt Outdated Show resolved Hide resolved
Copy link
Member

@calvn calvn left a comment

Choose a reason for hiding this comment

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

We could also consider changing the line where we ultimately set on ID token's Expiry value during token generation by using the floor of the two.

vault/identity_store_oidc.go Outdated Show resolved Hide resolved
vault/identity_store_oidc.go Show resolved Hide resolved
@fairclothjm
Copy link
Contributor Author

@calvn

We could also consider changing the line where we ultimately set on ID token's Expiry value during token generation by using the floor of the two.

In that case, would we want to add a warning if we are not using a value set by the user explicitly? It does seem like a simpler option.

@calvn
Copy link
Member

calvn commented Jul 22, 2021

Yeah, a warning would be useful. I also didn't imply that we should remove the checks that you added. I was suggesting that we could do both :)

vault/identity_store_oidc.go Outdated Show resolved Hide resolved
- remove make slice in favor of var delcaration
- remove unneeded if check
- validate expiry value during token generation
- update changelog as bug
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 23, 2021 16:50 Inactive
@vercel vercel bot temporarily deployed to Preview – vault July 23, 2021 16:50 Inactive
@vercel vercel bot temporarily deployed to Preview – vault July 23, 2021 20:21 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 23, 2021 20:21 Inactive
@vercel vercel bot temporarily deployed to Preview – vault July 23, 2021 20:28 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 23, 2021 20:28 Inactive
@vercel vercel bot temporarily deployed to Preview – vault July 26, 2021 14:39 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 26, 2021 14:39 Inactive
Copy link
Member

@calvn calvn left a comment

Choose a reason for hiding this comment

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

Few comments, but otherwise looks good!

vault/identity_store_oidc.go Outdated Show resolved Hide resolved
vault/identity_store_oidc.go Outdated Show resolved Hide resolved
vault/identity_store_oidc.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 27, 2021 14:04 Inactive
@vercel vercel bot temporarily deployed to Preview – vault July 27, 2021 14:04 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 27, 2021 14:22 Inactive
@vercel vercel bot temporarily deployed to Preview – vault July 27, 2021 14:22 Inactive
Copy link
Member

@calvn calvn left a comment

Choose a reason for hiding this comment

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

One comment, looks good otherwise!

vault/identity_store_oidc_test.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 28, 2021 21:54 Inactive
@vercel vercel bot temporarily deployed to Preview – vault July 28, 2021 21:54 Inactive
Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
@vercel vercel bot temporarily deployed to Preview – vault July 28, 2021 22:14 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 28, 2021 22:14 Inactive
Copy link
Member

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

LGTM!

@fairclothjm fairclothjm merged commit 62c0cdb into main Jul 29, 2021
@fairclothjm fairclothjm deleted the oidc-token-ttl-cap branch July 29, 2021 01:34
calvn pushed a commit that referenced this pull request Jul 29, 2021
…ion_ttl (#12151)

* do not allow token_ttl to be longer than verification_ttl

* add verification when updating an existing key

When updating a key, ensure any roles referencing the key do not already
have a token_ttl greater than the key's verification_ttl

* add changelog

* remove unneeded UT check and comment

* refactor based on PR comments

- remove make slice in favor of var delcaration
- remove unneeded if check
- validate expiry value during token generation
- update changelog as bug

* refactor get roles referencing target key names logic

* add note about thread safety to helper func

* update func comment

* sort array and refactor func names

* add warning to return response

* remove unnecessary code from unit test

* Update vault/identity_store_oidc.go

Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>

Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
calvn added a commit that referenced this pull request Jul 30, 2021
…ion_ttl (#12151) (#12213)

* do not allow token_ttl to be longer than verification_ttl

* add verification when updating an existing key

When updating a key, ensure any roles referencing the key do not already
have a token_ttl greater than the key's verification_ttl

* add changelog

* remove unneeded UT check and comment

* refactor based on PR comments

- remove make slice in favor of var delcaration
- remove unneeded if check
- validate expiry value during token generation
- update changelog as bug

* refactor get roles referencing target key names logic

* add note about thread safety to helper func

* update func comment

* sort array and refactor func names

* add warning to return response

* remove unnecessary code from unit test

* Update vault/identity_store_oidc.go

Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>

Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>

Co-authored-by: John-Michael Faircloth <fairclothjm@users.noreply.github.com>
Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
jartek pushed a commit to jartek/vault that referenced this pull request Sep 11, 2021
…ion_ttl (hashicorp#12151)

* do not allow token_ttl to be longer than verification_ttl

* add verification when updating an existing key

When updating a key, ensure any roles referencing the key do not already
have a token_ttl greater than the key's verification_ttl

* add changelog

* remove unneeded UT check and comment

* refactor based on PR comments

- remove make slice in favor of var delcaration
- remove unneeded if check
- validate expiry value during token generation
- update changelog as bug

* refactor get roles referencing target key names logic

* add note about thread safety to helper func

* update func comment

* sort array and refactor func names

* add warning to return response

* remove unnecessary code from unit test

* Update vault/identity_store_oidc.go

Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>

Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants