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: enforce key param and key existence on role creation #12208

Merged
merged 9 commits into from Sep 8, 2021

Conversation

fairclothjm
Copy link
Contributor

@fairclothjm fairclothjm commented Jul 29, 2021

This fixes a bug found after merging: #12151.

A role should not be allowed to be created without a key parameter. Additionally, the key must exist.

@fairclothjm fairclothjm requested a review from a team July 29, 2021 16:33
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 29, 2021 16:34 Inactive
@vercel vercel bot temporarily deployed to Preview – vault July 29, 2021 16:34 Inactive
Copy link
Contributor

@pcman312 pcman312 left a comment

Choose a reason for hiding this comment

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

Looks good overall. Just a couple of small readability improvement suggestions

vault/identity_store_oidc.go Outdated Show resolved Hide resolved
website/content/api-docs/secret/identity/tokens.mdx Outdated Show resolved Hide resolved
vault/identity_store_oidc_test.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 29, 2021 18:24 Inactive
@fairclothjm fairclothjm changed the title identity: handle creation of role without a key parameter identity: do not allow creation of role with a non-existent key param Jul 29, 2021
@fairclothjm fairclothjm changed the title identity: do not allow creation of role with a non-existent key param identity: do not allow creation of role with a non-existent key Jul 29, 2021
@fairclothjm fairclothjm changed the title identity: do not allow creation of role with a non-existent key identity: enforce key param and key existence on role creation Jul 29, 2021
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 29, 2021 18:33 Inactive
@vercel vercel bot temporarily deployed to Preview – vault July 29, 2021 18:33 Inactive
@fairclothjm fairclothjm requested a review from a team July 29, 2021 19:25
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.

Suggestion for adding two additional test cases, implementation looks good though!

vault/identity_store_oidc_test.go Outdated Show resolved Hide resolved
vault/identity_store_oidc_test.go Show resolved Hide resolved
changelog/12208.txt Outdated Show resolved Hide resolved
@kalafut
Copy link
Contributor

kalafut commented Jul 29, 2021

We should mark the param as required in the path definition:

"key": {
Type: framework.TypeString,
Description: "The OIDC key to use for generating tokens. The specified key must already exist.",
},

@calvn calvn modified the milestones: 1.8.1, 1.9 Jul 29, 2021
@calvn
Copy link
Member

calvn commented Jul 29, 2021

Since we're treating this as a breaking change, I milestoned it for 1.9

This fixes a bug found after merging: #12151.

We found the odd behavior from #12151 but IIRC this existed before it (i.e. allowing key to be empty/missing). We can ship #12151 in 1.8.1 and hold this off for 1.9.

@vercel vercel bot temporarily deployed to Preview – vault July 30, 2021 20:01 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 30, 2021 20:01 Inactive
@vercel vercel bot temporarily deployed to Preview – vault July 30, 2021 20:02 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook July 30, 2021 20:02 Inactive
@fairclothjm
Copy link
Contributor Author

@calvn

Since we're treating this as a breaking change, I milestoned it for 1.9

This fixes a bug found after merging: #12151.

We found the odd behavior from #12151 but IIRC this existed before it (i.e. allowing key to be empty/missing). We can ship #12151 in 1.8.1 and hold this off for 1.9.

What I was trying to express there is that #12151 introduces a bug. An attempt to create a role with a key name that does not yet exist will fail with the error:

a role's token ttl cannot be longer than the verification_ttl of the key it references

This is what was discovered in the ENT tests that broke.

So if we are shipping #12151 in 1.8.1, I think there should be another fix to allow creating a role with a non-existent key.

@vercel vercel bot temporarily deployed to Preview – vault September 8, 2021 14:43 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook September 8, 2021 14:43 Inactive
@fairclothjm fairclothjm merged commit 8f0a72e into main Sep 8, 2021
@fairclothjm fairclothjm deleted the identity-token-key-check branch September 8, 2021 15:47
jartek pushed a commit to jartek/vault that referenced this pull request Sep 11, 2021
…corp#12208)

* identity: handle creation of role without a key parameter

* update docs to not require key parameter for creation of a role

* add changelog

* require key param when creating a role

* lock create/update role; remove now redundant key check

* update changelog and UTs

* update change log to refelct actual implementation

* remove deprecated test case
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