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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Dec 26, 2023
Merged

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

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
benashz
benashz reviewed Dec 18, 2023
View reviewed changes
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
@thyton @benashz
Update CHANGELOG.md
9800a4b 
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@thyton @benashz
Update path_config.go
9658061 
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@thyton @benashz
Update path_config.go
4623035 
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@thyton
address comments
4bf43e6
@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
benashz
benashz approved these changes Dec 22, 2023
View reviewed changes
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
@thyton @benashz
Update path_config_test.go
004ed50 
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@thyton @benashz
Update path_config_test.go
29b0077 
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
@thyton
address comments
33dc18f
@thyton
reformat
39ab45d
@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
Read config endpoint does not indicate if token_reviewer_jwt is set #68
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benashz benashz benashz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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