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/oidc: fix duplicate keys in well-known #14543

Merged
merged 6 commits into from
Mar 17, 2022
Merged

Conversation

jasonodonnell
Copy link
Contributor

A bug was found where well-known keys would return duplicate entries when roles shared a key. This adds a check to ensure we haven't appended the keys already.

@jasonodonnell jasonodonnell changed the title identity/oidc: fix duplicate kids in well-known identity/oidc: fix duplicate keys in well-known Mar 16, 2022
@vercel vercel bot temporarily deployed to Preview – vault March 16, 2022 21:26 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 16, 2022 21:26 Inactive
Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

Thanks!

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

manual test looks good

#!/usr/bin/env bash
vault write identity/oidc/key/key1 rotation_period=2m verification_ttl=2m
vault write identity/oidc/role/role1 key=key1 ttl=1m
vault write identity/oidc/role/role2 key=key1 ttl=1m

curl -s \
  -X GET \
  http://127.0.0.1:8200/v1/identity/oidc/.well-known/keys \
  | jq -r ".keys[].kid"

Output:

Success! Data written to: identity/oidc/key/key1
Success! Data written to: identity/oidc/role/role1
Success! Data written to: identity/oidc/role/role2
34939e00-e875-943e-fea5-d03fd6c6953b
fa8c52f4-05f5-95b9-bffd-cd7fbc4acc25

@vercel vercel bot temporarily deployed to Preview – vault-storybook March 16, 2022 23:45 Inactive
@vercel vercel bot temporarily deployed to Preview – vault March 16, 2022 23:45 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 16, 2022 23:48 Inactive
@vercel vercel bot temporarily deployed to Preview – vault March 16, 2022 23:48 Inactive
Copy link
Contributor

@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 last comment, otherwise LGTM!

keyIDs[keyID] = struct{}{}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit: can we move the declaration of jwks closer, maybe somewhere in here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea 👍 Done in d16505f.

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.

4 participants