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

indicate that token reviewer jwt is set on config read #221

Merged
merged 10 commits into from
Dec 26, 2023

Conversation

thyton
Copy link
Contributor

@thyton thyton commented Dec 18, 2023

Overview

The read config endpoint does not expose the token_reviewer_jwt field for security reasons. Indication if it is set or not can save users from the kubernetes login route to verify.

Design of Change

Add a key value pair to the response data on config read to indicate that the token reviewer jwt is set.

Related Issues/Pull Requests

[ ] Issue #68

Contributor Checklist

[ ] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
My Docs PR Link

[ ] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)

~/go/src/github.com/hashicorp/vault-plugin-auth-kubernetes (VAULT-1664-indicate-token-reviewer-jwt-set) $ make integration-test
cd integrationtest && INTEGRATION_TESTS=true CGO_ENABLED=0 KUBE_CONTEXT="kind-vault-plugin-auth-kubernetes" go test '-test.v' -count=1 -timeout=20m ./...
?   	github.com/hashicorp/vault-plugin-auth-kubernetes/integrationtest/k8s	[no test files]
=== RUN   TestSuccess
--- PASS: TestSuccess (0.14s)
=== RUN   TestSuccessWithTokenReviewerJwt
--- PASS: TestSuccessWithTokenReviewerJwt (0.09s)
=== RUN   TestSuccessWithNamespaceLabels
--- PASS: TestSuccessWithNamespaceLabels (0.09s)
=== RUN   TestFailWithMismatchNamespaceLabels
--- PASS: TestFailWithMismatchNamespaceLabels (0.09s)
=== RUN   TestFailWithBadTokenReviewerJwt
--- PASS: TestFailWithBadTokenReviewerJwt (0.08s)
=== RUN   TestUnauthorizedServiceAccountErrorCode
--- PASS: TestUnauthorizedServiceAccountErrorCode (0.08s)
=== RUN   TestAudienceValidation
=== RUN   TestAudienceValidation/config:_a,_JWT:_b
=== RUN   TestAudienceValidation/config:_unset,_JWT:_default
=== RUN   TestAudienceValidation/config:_unset,_JWT:_a
=== RUN   TestAudienceValidation/config:_default,_JWT:_default
=== RUN   TestAudienceValidation/config:_default,_JWT:_a
=== RUN   TestAudienceValidation/config:_a,_JWT:_a
--- PASS: TestAudienceValidation (0.46s)
    --- PASS: TestAudienceValidation/config:_a,_JWT:_b (0.07s)
    --- PASS: TestAudienceValidation/config:_unset,_JWT:_default (0.08s)
    --- PASS: TestAudienceValidation/config:_unset,_JWT:_a (0.08s)
    --- PASS: TestAudienceValidation/config:_default,_JWT:_default (0.08s)
    --- PASS: TestAudienceValidation/config:_default,_JWT:_a (0.07s)
    --- PASS: TestAudienceValidation/config:_a,_JWT:_a (0.08s)
PASS
ok  	github.com/hashicorp/vault-plugin-auth-kubernetes/integrationtest	2.180s

[ ] Backwards compatible

@benashz benashz self-requested a review December 18, 2023 15:24
Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

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

Looking good! Had a few comments/suggestions.

path_config.go Outdated Show resolved Hide resolved
path_config.go Outdated Show resolved Hide resolved
path_config_test.go Outdated Show resolved Hide resolved
path_config_test.go Outdated Show resolved Hide resolved
path_config_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
thyton and others added 4 commits December 19, 2023 20:43
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@thyton
Copy link
Contributor Author

thyton commented Dec 22, 2023

Thank you for the feedback, @benashz! I've addressed all the comments. It's ready when you have a chance.

@thyton thyton requested a review from benashz December 22, 2023 16:19
Copy link
Contributor

@benashz benashz 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. Just had a few minor suggestions. After those are addressed: 👍

path_config_test.go Outdated Show resolved Hide resolved
path_config_test.go Outdated Show resolved Hide resolved
path_config_test.go Show resolved Hide resolved
thyton and others added 4 commits December 26, 2023 21:54
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@thyton thyton merged commit 6f9c733 into main Dec 26, 2023
8 checks passed
@thyton thyton deleted the VAULT-1664-indicate-token-reviewer-jwt-set branch December 26, 2023 15:17
@thyton thyton linked an issue Feb 19, 2024 that may be closed by this pull request
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.

Read config endpoint does not indicate if token_reviewer_jwt is set
2 participants