feat(openbao): import the JWT secrets plugin under infra/openbao - #647
Open
balajinvda wants to merge 8 commits into
Open
feat(openbao): import the JWT secrets plugin under infra/openbao#647balajinvda wants to merge 8 commits into
balajinvda wants to merge 8 commits into
Conversation
OpenBao is approved for GitHub, and its image bakes in a JWT secrets plugin built from a fork that lives only on GitLab. A reproducible public build needs that source here. The plugin is third-party: outfoxx/vault-plugin-secrets-jwt, Apache-2.0, Copyright 2021 Outfox, Inc. 73 of its 74 commits are upstream work. It is placed under infra/ rather than src/ because src/ is first-party NVCF code and this is not; nesting it under infra/openbao keeps it with the Dockerfile and build script that pin it, the same shape as infra/cassandra. Attribution is preserved rather than overwritten: the Apache-2.0 LICENSE and the upstream HEADER stay, all 17 upstream files keep their Outfox header, and NOTICE enumerates every NVIDIA modification as section 4(b) requires. Only the two files NVIDIA authored carry an NVIDIA header. AGENTS.md tells the copyright stamper to stay out. The friendlyid-go dependency is gone. Upstream mariuszs/friendlyid-go carries no license at all, so it is not redistributable, and the repository's own OSRB report already required its removal with no source copied. plugin/friendlyid.go is an independent base62 implementation written from the definition, with tests covering fixed width, round-trip, collisions and alphabet containment. The module path is renamed to the in-repo path. We do not track upstream, so resolving inside the monorepo is worth more than keeping diffs readable against Outfox. Upstream project machinery is deliberately not imported: GitHub Actions workflows above all, which would otherwise become live workflows in this repository, plus goreleaser, the upstream Dockerfile, install script, Makefile and linter config. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
The imported plugin is a standalone Go module with no BUILD.bazel and no entry in go.work.bazel, so the Bazel matrix never sees it and nothing would compile or test it. NVIDIA owns modifications to this code now, so that gap matters. It is kept out of the root graph on purpose. Its module graph is 278 modules including hashicorp/vault/api and hashicorp/vault/sdk, and the root module uses no hashicorp/vault at all today. Joining go.work.bazel would pull all of it into minimal version selection for every service in the repository in order to build one plugin binary that ships inside a single image. This job is plain go scoped to that one directory, path-filtered so it only runs when the plugin changes. It also asserts friendlyid-go stays out of the module graph. That project has no license and is not redistributable, so its return would be a licensing regression rather than a build failure, and nothing else in CI would notice. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded an isolated OpenBao JWT secrets plugin. The change includes configuration and role APIs, JWT signing, JWKS publication, key lifecycle management, licensing and documentation, CI checks, containerized tests, dependency inventory updates, and OpenBao packaging. ChangesOpenBao JWT secrets plugin
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (16)
infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/backend_test.go-92-92 (1)
92-92: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
+ 1adds one nanosecond, not one second.
config.KeyRotationPeriodis atime.Duration, so the untyped constant1is one nanosecond. The tests wait for the rotation period plus a one-nanosecond margin. The margin depends on the small delay between key creation and the sleep call, so these tests can fail intermittently under load. Use an explicit margin such astime.Second. This is upstream test code; the CI guideline requiresgo test ./...to pass, so a stable margin is worthwhile.As per coding guidelines: "The plugin's tests must pass with `go test ./...`".💚 Proposed change (apply to each sleep)
- time.Sleep(config.KeyRotationPeriod + 1) + time.Sleep(config.KeyRotationPeriod + time.Second)Also applies to: 138-138, 192-192, 202-202, 212-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/backend_test.go` at line 92, Update every time.Sleep call in the affected backend tests to add an explicit one-second margin to config.KeyRotationPeriod instead of the implicit one-nanosecond + 1. Apply this consistently to the sleeps near the existing rotation-period checks, preserving the test flow while making key-rotation timing reliable.Source: Coding guidelines
infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/policy_signer.go-50-52 (1)
50-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject or intentionally convert non-string role headers.
Role.Headersaccepts arbitrary values. Numeric and boolean values reachfmt.Sprintf("%s", v)and become strings such as%!s(float64=1.5). Reject non-string values or use an intentional conversion such as%v.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/policy_signer.go` around lines 50 - 52, Update the ExtraHeaders processing in the policy signer to handle non-string Role.Headers values intentionally: either validate and reject values that are not strings, or convert them using a general value format such as %v. Ensure numeric and boolean headers never produce malformed %!s(...) output while preserving existing string header behavior.infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/util.go-70-78 (1)
70-78: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winImport the hash implementations explicitly. The plugin currently gets
crypto/sha1,crypto/sha256, andcrypto/sha512throughcrypto/x509, but this creates unrelated transitive coupling. Add blank imports for those packages socreateKeyIdandPolicySigner.signcannot panic if that dependency changes. Keep SHA-1 unless consumers can migrate because changing it changes existingkidvalues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/util.go` around lines 70 - 78, Update the imports in util.go to explicitly blank-import crypto/sha1, crypto/sha256, and crypto/sha512, ensuring the hash implementations used by createKeyId and PolicySigner.sign are registered independently of crypto/x509. Preserve SHA-1 and the existing createKeyId output so current kid values remain unchanged.infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_sign.go-153-157 (1)
153-157: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe
%Tverb reports the wrong type for a non-string audience entry. Both files use the same copied audience loop. Each passesaudEntryto%T. After the type assertion fails,audEntryholds the zero-valuestring, so the message always reportsstringand never names the actual type. PassrawAudEntryinstead.
infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_sign.go#L153-L157: change the operand on Line 156 fromaudEntrytorawAudEntry.infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_roles.go#L264-L268: change the operand on Line 267 fromaudEntrytorawAudEntry.The audience validation block is duplicated between the two files. Extract it into a shared helper so a single fix covers both paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_sign.go` around lines 153 - 157, The audience validation duplicated in path_sign.go lines 153-157 and path_roles.go lines 264-268 reports the asserted zero-value string type; extract the shared validation into a common helper and pass rawAudEntry to the %T diagnostic so both paths report the actual non-string type. Update both call sites to use the helper while preserving existing invalid-request behavior.infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_roles_test.go-138-150 (1)
138-150: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the result of the
subclaim role write.Line 139 assigns
errand never checks it. Line 147 overwriteserr. The NVIDIA change allowssubin the roleclaimsfield, but no assertion covers that behavior. Add the success check.💚 Proposed fix
- // added for nv - err := writeRole(b, storage, role, role+".example.com", map[string]interface{}{"sub": "allowed"}, map[string]interface{}{}) - - // added for nv - // sub claim is allowed in role's claims field - // if err == nil { - // t.Fatalf("Create role should have failed") - // } + // The 'sub' claim is permitted in the role 'claims' field. + err := writeRole(b, storage, role, role+".example.com", map[string]interface{}{"sub": "allowed"}, map[string]interface{}{}) + if err != nil { + t.Fatalf("create role with 'sub' claim should have succeeded: %s\n", err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_roles_test.go` around lines 138 - 150, Update the first writeRole call in the role test to assert that creating a role with the sub claim succeeds, before reusing err for the foo claim case. Keep the existing failure assertion for the subsequent writeRole call unchanged.infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_config.go-155-157 (1)
155-157: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
%vfor the[]intslice in the error message.
%sapplied to[]intrenders as[%!s(int=2048) %!s(int=3072) ...]. The operator sees a malformed list of supported values.🐛 Proposed fix
- return logical.ErrorResponse("unsupported rsa_key_bits, must be one of %s", AllowedRSAKeyBits), logical.ErrInvalidRequest + return logical.ErrorResponse("unsupported rsa_key_bits, must be one of %v", AllowedRSAKeyBits), logical.ErrInvalidRequest🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_config.go` around lines 155 - 157, Update the error response in the rsa_key_bits validation within the configuration handler to format AllowedRSAKeyBits with a verb suitable for []int, so the supported values render correctly while preserving the existing invalid-request behavior.infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_sign.go-127-138 (1)
127-138: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLine 136 omits the operand for
%T.The message renders as
'sub' claim was %!T(MISSING), not string. PassrawSub.The related
%Tdefect at Line 156 shares a root cause withpath_roles.goLine 267. See the consolidated comment.🐛 Proposed fix
- return logical.ErrorResponse("'sub' claim was %T, not string"), logical.ErrInvalidRequest + return logical.ErrorResponse("'sub' claim was %T, not string", rawSub), logical.ErrInvalidRequest🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_sign.go` around lines 127 - 138, Update the type-mismatch error response in the claims validation logic to pass rawSub as the operand for the %T formatter. Preserve the existing error message and invalid-request return behavior.infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_sign.go-38-49 (1)
38-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove unsupported
headersfrom the sign request test.
pathSigndoes not declare or readkeyHeaders. The SDK ignores the key and adds a warning.TestPrivateHeaderpasses because the role already definestid=12345. If per-request headers are intended, add the schema, read the field, and validate it againstconfig.allowedHeadersMap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_sign.go` around lines 38 - 49, Remove the unsupported headers field from the sign request test associated with pathSign, since pathSign does not declare or read keyHeaders. Keep the test focused on the role-defined tid=12345 claim; only add per-request header support if implementing the full schema, field-reading, and config.allowedHeadersMap validation path.infra/openbao/plugins/vault-plugin-secrets-jwt/README.md-125-137 (1)
125-137: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
headersterminology in the Allowed Headers section.The section documents
allowed_headers, but the example text callsissandpath“claims.” Replace “claims” with “headers” to match the API field and command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/README.md` around lines 125 - 137, Update the Allowed Headers section in the README to refer to iss and path as headers rather than claims, while leaving the allowed_headers command and surrounding documentation unchanged.infra/openbao/plugins/vault-plugin-secrets-jwt/README.md-181-187 (1)
181-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQuote wildcard patterns in shell examples.
An unquoted
*is expanded by the shell when matching files exist. Quote bothsubject_patternandaudience_patternin the configuration and role commands.Proposed fix
- vault write jwt/config subject_pattern=*.example.com + vault write jwt/config subject_pattern='*.example.com' - vault write jwt/config audience_pattern=*.example.com + vault write jwt/config audience_pattern='*.example.com' - vault write jwt/roles/test-role subject_pattern=*.example.com + vault write jwt/roles/test-role subject_pattern='*.example.com' - vault write jwt/roles/test-role audience_pattern=*.example.com + vault write jwt/roles/test-role audience_pattern='*.example.com'Also applies to: 268-274
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/README.md` around lines 181 - 187, Quote the wildcard values in the README command examples for both subject_pattern and audience_pattern, including the corresponding configuration and role commands referenced by the comment, so the shell passes the literal *. Preserve the existing commands and patterns otherwise.infra/openbao/plugins/vault-plugin-secrets-jwt/README.md-142-149 (1)
142-149: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the remaining user-facing spelling and grammar errors.
Fix
ES256algorithm,2048 bit,stings, andrewrote. These errors reduce clarity in the usage documentation.Also applies to: 189-190, 309-310
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/README.md` around lines 142 - 149, Correct the remaining spelling and grammar errors in the README: add the missing space in “ES256 algorithm,” hyphenate “2048-bit,” and fix the misspellings “stings” and “rewrote.” Apply these edits at the referenced documentation occurrences while preserving the surrounding usage instructions.Source: Linters/SAST tools
infra/openbao/plugins/vault-plugin-secrets-jwt/AGENTS.md-3-7 (1)
3-7: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSeparate repository policy from Apache-2.0 requirements.
Apache-2.0 section 4(b) requires prominent notices in modified files. It does not require a complete modification inventory in
NOTICE. Keep the inventory because repository policy requires it, but describe the legal obligation accurately.
infra/openbao/plugins/vault-plugin-secrets-jwt/AGENTS.md#L3-L7: replace the section 4(b) attribution with repository-policy wording.infra/openbao/plugins/vault-plugin-secrets-jwt/NOTICE#L19-L20: replace “As required by section 4(b)” with wording that identifies the list as the repository's compliance record.As per coding guidelines, preserve upstream headers and record NVIDIA-authored changes in
NOTICE.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/AGENTS.md` around lines 3 - 7, Update the section 4(b) attribution in infra/openbao/plugins/vault-plugin-secrets-jwt/AGENTS.md lines 3-7 to describe the modification inventory as repository policy, not an Apache-2.0 requirement. Update infra/openbao/plugins/vault-plugin-secrets-jwt/NOTICE lines 19-20 to identify the list as the repository’s compliance record instead of saying it is required by section 4(b); preserve upstream headers and the NVIDIA change inventory.Source: Coding guidelines
infra/openbao/plugins/vault-plugin-secrets-jwt/README.md-5-5 (1)
5-5: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse signing terminology for the supported algorithms.
This plugin signs JWTs.
ES256,ES384,ES512, and theRS*algorithms are signature algorithms, not encryption algorithms. Rename this section and use “asymmetric signing” so users do not infer token confidentiality.Proposed wording
-# Encryption and Key Management +# Signing and Key Management ... -The plugin supports a subset of the asymmetric encryption algorithms outlined in the JWT +The plugin supports a subset of the asymmetric signing algorithms outlined in the JWT specification. ... -Note: Due to its reliance on asymmetric encryption, the plugin will not support symmetric algorithms. +Note: The plugin uses asymmetric signing algorithms and does not support symmetric algorithms.Also applies to: 33-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/README.md` at line 5, Update the README section heading and related wording covering the supported ES256, ES384, ES512, and RS* algorithms to use “signing” terminology, specifically describing them as asymmetric signing rather than encryption, while preserving the existing algorithm documentation.infra/openbao/plugins/vault-plugin-secrets-jwt/README.md-2-2 (1)
2-2: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a valid heading hierarchy.
The document jumps from an H1 directly to H3 for the introductory subtitle and
Early Access. Change both headings to H2.Also applies to: 29-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/README.md` at line 2, Update the introductory subtitle and the “Early Access” heading in the README to use H2 markers instead of H3, preserving the existing heading text and valid hierarchy beneath the document’s H1.Source: Linters/SAST tools
infra/openbao/plugins/vault-plugin-secrets-jwt/README.md-312-315 (1)
312-315: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the Quick Links list render as Markdown.
The four-space indentation makes the list a code block. Remove the indentation so the links render as a Markdown list.
Proposed fix
- - Vault Website: https://www.vaultproject.io - - Main Project Github: https://www.github.com/hashicorp/vault - - JWT docs: https://jwt.io +- Vault Website: https://www.vaultproject.io +- Main Project Github: https://www.github.com/hashicorp/vault +- JWT docs: https://jwt.io🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/README.md` around lines 312 - 315, Remove the leading indentation from each item under the “Quick Links” heading so the Vault Website, Main Project Github, and JWT docs entries render as a Markdown list rather than a code block.Source: Linters/SAST tools
infra/openbao/plugins/vault-plugin-secrets-jwt/README.md-96-104 (1)
96-104: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse a repository-owned image or remove the container claim.
outfoxx/vaultis the upstream image. This repository contains no image definition, andNOTICEstates that the upstream Dockerfile was omitted. Document the repository build path and image name, or remove these instructions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/README.md` around lines 96 - 104, Update the Container section in the README to remove the unsupported outfoxx/vault testing instructions, or replace them with the repository-owned image name and documented build path. Do not retain a claim that this repository provides a pre-packaged container without identifying its local build definition and image.
🧹 Nitpick comments (4)
infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/config.go (1)
252-252: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
DefaultConfigshares the package-levelDefaultAllowedClaimsslice.Every default
Configpoints at the same backing array. A later in-place modification ofAllowedClaimson one config changes the default for all other backends in the process. Copy the slice.♻️ Proposed change
- c.AllowedClaims = DefaultAllowedClaims + c.AllowedClaims = append([]string(nil), DefaultAllowedClaims...)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/config.go` at line 252, Update DefaultConfig where AllowedClaims is initialized so each Config receives an independent copy of DefaultAllowedClaims rather than sharing its backing array; preserve the existing default values while preventing later in-place mutations from affecting other configurations.infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_sign_test.go (1)
30-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge the two near-identical sign helpers.
getSignedTokenWithClaimsandgetSignedTokenWithoutClaimsdiffer only in the operation and in whetherDatais set. Everything after theHandleRequestcall is duplicated. Extract one helper that takes the operation and an optional data map.The merge also fixes a duplicated defect. Line 71 and Line 130 format
errinto the message, buterris nil at that point because the precedingFetchJWKScall succeeded. The message renderserror locating unique public keys: %!s(<nil>). Report the key ID and the match count instead.♻️ Proposed message fix
matchingPublicKeys := publicKeys.Key(token.Headers[0].KeyID) if len(matchingPublicKeys) != 1 { - return fmt.Errorf("error locating unique public keys: %s", err) + return fmt.Errorf("expected 1 public key for kid %q, got %d", token.Headers[0].KeyID, len(matchingPublicKeys)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_sign_test.go` around lines 30 - 151, Merge getSignedTokenWithClaims and getSignedTokenWithoutClaims into one shared helper that accepts the request operation and optional request data, preserving the existing claims and headers destination behavior. Update both callers to supply their respective operation and data configuration, then use the shared post-HandleRequest token-validation flow. In the unique public-key failure path, report token.Headers[0].KeyID and the matching key count instead of formatting the stale nil err value.infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_roles.go (1)
246-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelete the commented-out
subcheck and record the deviation.The NVIDIA modification disables the reserved-
subrestriction. The disabled code stays in the file as a comment block. Remove the block. Keep one short comment that states the behavior change, and record the modification inNOTICEin the same commit, as the plugin path instructions require. If the restriction must return later, open an issue instead of keeping the code in a comment.The same pattern appears in
path_roles_test.goLines 138-145.As per coding guidelines: "record each such change in
NOTICEin the same commit" and "create a ticket for follow-up work instead of leaving a TODO in code".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_roles.go` around lines 246 - 251, Remove the commented-out reserved-sub validation block near the role claims handling in path_roles.go and retain only a brief comment documenting that NVIDIA disables this restriction. Apply the same cleanup to the corresponding block in path_roles_test.go, and record both modifications in NOTICE as required; do not leave TODOs or disabled code comments.Source: Coding guidelines
infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_jwks_test.go (1)
74-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test compares the endpoint output with the same function it calls.
FetchJWKSreachesgetPublicKeysthroughb.pathJwksRead. Line 74 callsgetPublicKeysdirectly. Both sides of the Line 92 comparison come from the same producer, so the test verifies the JSON round trip only. It cannot detect a defect insidegetPublicKeys, including the zero-value key entries described inpath_jwks.goLines 81-113.Add an assertion on the key contents. Check that each returned key has a non-nil
Key, the expectedUsevalue, and aKeyIDthat matches the active policy versions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_jwks_test.go` around lines 74 - 94, Update the JWKS test around b.pathJwksRead and the expectedKeySet comparison so expected data is not validated solely against getPublicKeys. Add assertions for every returned key verifying Key is non-nil, Use has the expected value, and KeyID matches the active policy versions, while preserving the existing endpoint comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/openbao-jwt-plugin.yml:
- Around line 71-76: Update the “Assert no unlicensed dependency” workflow step
to capture the successful output of `go list -m all` before searching it, and
make any failure of that command fail the step rather than treating it as
absence. Search the captured module list for `mariuszs/friendlyid-go` without
piping `go list` directly into `grep`, while preserving the existing error and
exit behavior when the dependency is found.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/NOTICE`:
- Around line 11-14: Update the attribution statements in NOTICE to apply only
to upstream-originated files, and add AGENTS.md and CLAUDE.md to the
NVIDIA-authored modification inventory with their correct header treatment.
Preserve upstream copyright headers only on files that retain upstream origins,
and ensure both guidance files are explicitly recorded.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/config.go`:
- Around line 186-201: Update saveConfig so its err != nil path returns the
assigned internal error instead of nil. Preserve the existing successful return
behavior when the RSA or signature algorithm is supported, ensuring unsupported
cases propagate the error and do not report a successful save.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_jwks.go`:
- Around line 81-113: Update the JWKS construction in the policy key loop to
initialize an empty Keys slice and append only fully usable public keys. Skip
missing versions, PEM decode failures, parse failures, and entries without
either a valid FormattedPublicKey or RSAKey; assign metadata only after a key is
confirmed, then append the completed jose.JSONWebKey instead of indexing a
fixed-length slice.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_roles_test.go`:
- Around line 204-215: Update the audience assertion in the test to compare the
response’s claims["aud"] value against the expected audience array defined by
the test, rather than the locally extracted audience variable. Keep the existing
response parsing and missing-claim checks intact.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_roles.go`:
- Around line 96-105: The role schema exposes keyMaxAllowedAudiences and
keyAllowedClaims without storing or applying them. Either implement both fields
end to end—add corresponding Role fields, populate them in pathRolesWrite via
d.GetOk, validate the audience limit against config.MaxAudiences, and use the
role-specific values in the audience-count and signing logic—or remove both
schema entries and their descriptions so unsupported settings are not accepted.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/util.go`:
- Around line 37-44: Update friendlyIdGenerator.id to use uuid.NewRandom instead
of uuid.NewUUID, preserving the existing error handling and encodeBase62UUID
conversion. Confirm that no consumers require the current version-1 jti format
before making this change.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/test/jwtverify/jwtverify.go`:
- Around line 48-60: Update validateToken’s JWKS retrieval to use an http.Client
configured with a bounded Timeout instead of http.Get/default client, while
preserving the existing response-body close and read/error handling.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/test/stress-test.sh`:
- Around line 11-13: Update the stress script’s worker lifecycle around fail and
the stress job launch sites: track every background worker PID, wait for each
worker before the parent exits, and propagate a nonzero status when any worker
fails. Ensure fail still terminates the worker process group while the script
ultimately returns failure instead of exiting successfully.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/test/test.sh`:
- Around line 4-7: Both test.sh and stress-test.sh must wait for the background
Vault server to become ready before the first CLI request. Add a health or
status polling step after exporting VAULT_ADDR and before vault login in each
script, preserving the existing server startup and process handling.
---
Minor comments:
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/AGENTS.md`:
- Around line 3-7: Update the section 4(b) attribution in
infra/openbao/plugins/vault-plugin-secrets-jwt/AGENTS.md lines 3-7 to describe
the modification inventory as repository policy, not an Apache-2.0 requirement.
Update infra/openbao/plugins/vault-plugin-secrets-jwt/NOTICE lines 19-20 to
identify the list as the repository’s compliance record instead of saying it is
required by section 4(b); preserve upstream headers and the NVIDIA change
inventory.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/backend_test.go`:
- Line 92: Update every time.Sleep call in the affected backend tests to add an
explicit one-second margin to config.KeyRotationPeriod instead of the implicit
one-nanosecond + 1. Apply this consistently to the sleeps near the existing
rotation-period checks, preserving the test flow while making key-rotation
timing reliable.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_config.go`:
- Around line 155-157: Update the error response in the rsa_key_bits validation
within the configuration handler to format AllowedRSAKeyBits with a verb
suitable for []int, so the supported values render correctly while preserving
the existing invalid-request behavior.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_roles_test.go`:
- Around line 138-150: Update the first writeRole call in the role test to
assert that creating a role with the sub claim succeeds, before reusing err for
the foo claim case. Keep the existing failure assertion for the subsequent
writeRole call unchanged.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_sign.go`:
- Around line 153-157: The audience validation duplicated in path_sign.go lines
153-157 and path_roles.go lines 264-268 reports the asserted zero-value string
type; extract the shared validation into a common helper and pass rawAudEntry to
the %T diagnostic so both paths report the actual non-string type. Update both
call sites to use the helper while preserving existing invalid-request behavior.
- Around line 127-138: Update the type-mismatch error response in the claims
validation logic to pass rawSub as the operand for the %T formatter. Preserve
the existing error message and invalid-request return behavior.
- Around line 38-49: Remove the unsupported headers field from the sign request
test associated with pathSign, since pathSign does not declare or read
keyHeaders. Keep the test focused on the role-defined tid=12345 claim; only add
per-request header support if implementing the full schema, field-reading, and
config.allowedHeadersMap validation path.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/policy_signer.go`:
- Around line 50-52: Update the ExtraHeaders processing in the policy signer to
handle non-string Role.Headers values intentionally: either validate and reject
values that are not strings, or convert them using a general value format such
as %v. Ensure numeric and boolean headers never produce malformed %!s(...)
output while preserving existing string header behavior.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/util.go`:
- Around line 70-78: Update the imports in util.go to explicitly blank-import
crypto/sha1, crypto/sha256, and crypto/sha512, ensuring the hash implementations
used by createKeyId and PolicySigner.sign are registered independently of
crypto/x509. Preserve SHA-1 and the existing createKeyId output so current kid
values remain unchanged.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/README.md`:
- Around line 125-137: Update the Allowed Headers section in the README to refer
to iss and path as headers rather than claims, while leaving the allowed_headers
command and surrounding documentation unchanged.
- Around line 181-187: Quote the wildcard values in the README command examples
for both subject_pattern and audience_pattern, including the corresponding
configuration and role commands referenced by the comment, so the shell passes
the literal *. Preserve the existing commands and patterns otherwise.
- Around line 142-149: Correct the remaining spelling and grammar errors in the
README: add the missing space in “ES256 algorithm,” hyphenate “2048-bit,” and
fix the misspellings “stings” and “rewrote.” Apply these edits at the referenced
documentation occurrences while preserving the surrounding usage instructions.
- Line 5: Update the README section heading and related wording covering the
supported ES256, ES384, ES512, and RS* algorithms to use “signing” terminology,
specifically describing them as asymmetric signing rather than encryption, while
preserving the existing algorithm documentation.
- Line 2: Update the introductory subtitle and the “Early Access” heading in the
README to use H2 markers instead of H3, preserving the existing heading text and
valid hierarchy beneath the document’s H1.
- Around line 312-315: Remove the leading indentation from each item under the
“Quick Links” heading so the Vault Website, Main Project Github, and JWT docs
entries render as a Markdown list rather than a code block.
- Around line 96-104: Update the Container section in the README to remove the
unsupported outfoxx/vault testing instructions, or replace them with the
repository-owned image name and documented build path. Do not retain a claim
that this repository provides a pre-packaged container without identifying its
local build definition and image.
---
Nitpick comments:
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/config.go`:
- Line 252: Update DefaultConfig where AllowedClaims is initialized so each
Config receives an independent copy of DefaultAllowedClaims rather than sharing
its backing array; preserve the existing default values while preventing later
in-place mutations from affecting other configurations.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_jwks_test.go`:
- Around line 74-94: Update the JWKS test around b.pathJwksRead and the
expectedKeySet comparison so expected data is not validated solely against
getPublicKeys. Add assertions for every returned key verifying Key is non-nil,
Use has the expected value, and KeyID matches the active policy versions, while
preserving the existing endpoint comparison.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_roles.go`:
- Around line 246-251: Remove the commented-out reserved-sub validation block
near the role claims handling in path_roles.go and retain only a brief comment
documenting that NVIDIA disables this restriction. Apply the same cleanup to the
corresponding block in path_roles_test.go, and record both modifications in
NOTICE as required; do not leave TODOs or disabled code comments.
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_sign_test.go`:
- Around line 30-151: Merge getSignedTokenWithClaims and
getSignedTokenWithoutClaims into one shared helper that accepts the request
operation and optional request data, preserving the existing claims and headers
destination behavior. Update both callers to supply their respective operation
and data configuration, then use the shared post-HandleRequest token-validation
flow. In the unique public-key failure path, report token.Headers[0].KeyID and
the matching key count instead of formatting the stale nil err value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a44dc0a9-00a3-4dc0-ba61-be2e80aaafa5
⛔ Files ignored due to path filters (15)
infra/openbao/plugins/vault-plugin-secrets-jwt/go.sumis excluded by!**/*.suminfra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/allowed_claims.jsonis excluded by!**/testdata/**infra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/claims.jsonis excluded by!**/testdata/**infra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/claims1.jsonis excluded by!**/testdata/**infra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/claims10.jsonis excluded by!**/testdata/**infra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/claims2.jsonis excluded by!**/testdata/**infra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/claims3.jsonis excluded by!**/testdata/**infra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/claims4.jsonis excluded by!**/testdata/**infra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/claims5.jsonis excluded by!**/testdata/**infra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/claims6.jsonis excluded by!**/testdata/**infra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/claims7.jsonis excluded by!**/testdata/**infra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/claims8.jsonis excluded by!**/testdata/**infra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/claims9.jsonis excluded by!**/testdata/**infra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/claims_foo.jsonis excluded by!**/testdata/**infra/openbao/plugins/vault-plugin-secrets-jwt/test/testdata/invalid_claims.jsonis excluded by!**/testdata/**
📒 Files selected for processing (32)
.github/workflows/openbao-jwt-plugin.ymlinfra/openbao/plugins/vault-plugin-secrets-jwt/AGENTS.mdinfra/openbao/plugins/vault-plugin-secrets-jwt/CLAUDE.mdinfra/openbao/plugins/vault-plugin-secrets-jwt/HEADERinfra/openbao/plugins/vault-plugin-secrets-jwt/LICENSEinfra/openbao/plugins/vault-plugin-secrets-jwt/NOTICEinfra/openbao/plugins/vault-plugin-secrets-jwt/README.mdinfra/openbao/plugins/vault-plugin-secrets-jwt/cmd/vault-plugin-secrets-jwt/main.goinfra/openbao/plugins/vault-plugin-secrets-jwt/go.modinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/backend.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/backend_test.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/config.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/friendlyid.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/friendlyid_test.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_config.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_config_test.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_jwks.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_jwks_test.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_roles.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_roles_test.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_sign.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/path_sign_test.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/policy_signer.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/token.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/util.goinfra/openbao/plugins/vault-plugin-secrets-jwt/test/Dockerfileinfra/openbao/plugins/vault-plugin-secrets-jwt/test/Stress-Dockerfileinfra/openbao/plugins/vault-plugin-secrets-jwt/test/config.hclinfra/openbao/plugins/vault-plugin-secrets-jwt/test/godoc.goinfra/openbao/plugins/vault-plugin-secrets-jwt/test/jwtverify/jwtverify.goinfra/openbao/plugins/vault-plugin-secrets-jwt/test/stress-test.shinfra/openbao/plugins/vault-plugin-secrets-jwt/test/test.sh
Brings across the rest of nvcf-openbao, mirroring infra/cassandra: Dockerfile, scripts, files/plugins, README, license-header tooling, upgrade/. Not imported: Dockerfile.internal, which bases on an internal mirror; .gitlab-ci.yml, which stays as-is in nvcf-internal; renovate, releaserc, CODEOWNERS and the OSRB report. The plugin is now compiled in a Dockerfile build stage rather than copied in prebuilt. Previously build-jwt-plugin.sh cloned a fork hosted outside this repository at a pinned revision, so the image could not be built from public source and the binaries had to arrive some other way: committed to git, or injected as a private overlay the way cassandra takes its exporter jar. Neither is needed. buildah is invoked with infra/openbao as the build context and the plugin source now sits at plugins/ inside it, so the image compiles the plugin from the same commit that produces it. That is a stronger position than cassandra, whose exporter jar is a third-party release artifact we cannot build and must therefore vendor. files/plugins/.gitignore keeps built binaries out of git; the directory still ships only .gitkeep. build-jwt-plugin.sh is kept and repointed at the in-repo source, since it is also the local developer path and what verify/smoke run against. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
check-license enforces that NOTICE lists every per-directory notice file, and the import added one it did not know about: ERROR: NOTICE is out of sync with repo notice / third-party license paths. + infra/openbao/plugins/vault-plugin-secrets-jwt/NOTICE Regenerated with ./tools/scripts/update-license. check-license now passes: 291 files with valid headers, MPL audit in sync. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
Contributor
Author
|
Thanks — worked through all ten. Two were mine and are fixed; eight are pre-existing upstream code and are deliberately out of scope for an import. Fixed
Not fixed: upstream Outfox code
I checked the two on NVIDIA-modified files rather than assuming: This PR imports third-party Apache-2.0 code with attribution intact. Fixing upstream defects inside the import would blur which lines are Outfox's and which are ours, and every such fix has to be added to the Two are worth that follow-up on substance: |
CodeRabbit, both valid: The workflow's licensing gate failed open. Inside `if`, a failing `go list` read as "dependency absent", and under pipefail `grep -q` can close the pipe and leave `go list` killed by SIGPIPE. It now captures the module list and fails the step if enumeration fails. NOTICE claimed unlisted files were unmodified upstream and that all other files retain the Outfox header. AGENTS.md and CLAUDE.md are NVIDIA-added, so both statements were false. The scope is now limited to upstream-originated files and the guidance files are inventoried. The remaining eight findings are pre-existing upstream code, including the two on NVIDIA-modified files: git blame puts path_roles.go:105 and path_roles_test.go:215 on upstream commits. Fixing upstream defects inside an import would blur which lines are Outfox's and which are ours. verify-jwt-plugin.sh asserted three things that only held when the plugin came from a frozen external revision: - the module path, updated for the rename - a required vcs.revision, dropped. Neither build path has git metadata: this script builds from a copy in a temp dir and the image build COPYs source into a layer. Requiring the stamp fails both; requiring a value pins a revision that no longer exists. - pinned sha256s, now recorded rather than asserted. Any source edit in this repository legitimately changes them, so equality would fail on every real change and teach people to update the constant without reading it. What still carries provenance is asserted: module path, target triple, toolchain floor, and the x/net, vault/api and vault/sdk versions. Verified end to end: build-jwt-plugin.sh produces both arches from the in-repo source and verify-jwt-plugin.sh passes on both. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
… the host MAC Two upstream defects CodeRabbit raised. I had declined all eight upstream findings as out of scope for an import, which was wrong for these two: we do not track upstream, so this code is ours to maintain, and both are security relevant in a plugin that signs tokens. plugin/config.go discarded the "unknown/unsupported signature algorithm" error and returned nil, so an unsupported algorithm reported success while the key was never rotated and the caller had no way to detect it. plugin/util.go generated token ids from uuid.NewUUID, which is a version 1 UUID: it encodes the host MAC address and the creation timestamp. That id is published as the token's jti, so every token holder received the signer's hardware address and issue time. Now uuid.NewRandom (v4), with a test that asserts the version and that consecutive ids do not share a trailing segment, which is what a node-derived id would show. Both recorded in NOTICE as required by Apache-2.0 section 4(b). The remaining six findings are upstream style and test-harness issues with no security or correctness impact, and are left to a follow-up so they can be reviewed as NVIDIA changes rather than buried in an import. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@infra/openbao/Dockerfile`:
- Line 35: Change plugin executable permissions from 775 to 755 consistently:
update the COPY directive in infra/openbao/Dockerfile at lines 35-35, the chmod
and install modes in infra/openbao/scripts/build-jwt-plugin.sh at lines 61-72,
and the COPY directive in infra/openbao/upgrade/Dockerfile.upgrade at lines
13-13.
In `@infra/openbao/files/plugins/PROVENANCE.md`:
- Around line 3-12: Remove the private NVIDIA GitLab source URL and internal
NVCF-10946 work-item reference from the provenance documentation. Retain only
publicly verifiable in-repository provenance, such as the build and verification
script references and publicly available commit or dependency details.
In `@infra/openbao/README.md`:
- Around line 7-39: The documentation describes an obsolete prebuilt-plugin
workflow. In infra/openbao/README.md lines 7-39, remove external cloning, binary
placement, architecture-specific build commands, and related prerequisites, and
document that the image builds the in-tree plugin module. In
infra/openbao/files/plugins/PROVENANCE.md lines 3-29, remove claims about
committed binaries and fixed hashes, and describe the repository source and
current verification process.
In `@infra/openbao/scripts/build-jwt-plugin.sh`:
- Around line 31-42: Update the work-directory setup and cleanup logic around
work_dir and cleanup so the exit trap removes only directories created by
mktemp. Track whether WORK_DIR was caller-provided, preserve it by default
regardless of KEEP_WORK_DIR, and retain the existing cleanup behavior for
script-created temporary directories unless explicit deletion is requested.
In `@infra/openbao/scripts/smoke-jwt-plugin-runtime.sh`:
- Around line 46-113: Update the smoke-test script to create a single private
temporary directory with mktemp -d before generating artifacts, and replace
every predictable /tmp/openbao-dev.hcl, /tmp/openbao.log, /tmp/bao-status.txt,
JWT, claims, and decoded-output path with files under that directory. Extend the
existing EXIT trap to remove the temporary directory while still terminating
server_pid, and preserve all current command behavior and checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 39dec38d-76c9-45d8-81a3-229d948aaf38
📒 Files selected for processing (15)
.github/workflows/openbao-jwt-plugin.ymlNOTICEinfra/openbao/.license-header.txtinfra/openbao/Dockerfileinfra/openbao/README.mdinfra/openbao/files/plugins/.gitignoreinfra/openbao/files/plugins/.gitkeepinfra/openbao/files/plugins/PROVENANCE.mdinfra/openbao/plugins/vault-plugin-secrets-jwt/NOTICEinfra/openbao/scripts/apply-license-header.shinfra/openbao/scripts/build-jwt-plugin.shinfra/openbao/scripts/check-license-header.shinfra/openbao/scripts/smoke-jwt-plugin-runtime.shinfra/openbao/scripts/verify-jwt-plugin.shinfra/openbao/upgrade/Dockerfile.upgrade
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/openbao-jwt-plugin.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/friendlyid_test.go`:
- Around line 136-157: Update the UUID version test to exercise
friendlyIdGenerator.id() rather than calling uuid.NewRandom() directly: create a
generator, generate each identifier, decode it with the test decoder defined
earlier, and assert the decoded UUID is version 4. Retain the existing
consecutive-ID suffix comparison as an additional check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7d9913ed-bb04-4608-ab55-5551c6d2baa5
📒 Files selected for processing (4)
infra/openbao/plugins/vault-plugin-secrets-jwt/NOTICEinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/config.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/friendlyid_test.goinfra/openbao/plugins/vault-plugin-secrets-jwt/plugin/util.go
🚧 Files skipped from review as they are similar to previous changes (3)
- infra/openbao/plugins/vault-plugin-secrets-jwt/NOTICE
- infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/util.go
- infra/openbao/plugins/vault-plugin-secrets-jwt/plugin/config.go
All six findings were on files this PR adds, and all were valid. files/plugins/PROVENANCE.md carried an internal GitLab fork URL and a private tracker id. I imported it wholesale without scanning it, which is exactly what the OSS hygiene rule exists to prevent. It was also stale: it described committed binaries and pinned hashes that no longer apply now that the plugin is compiled during the image build. Rewritten around the in-tree source. Dockerfile: the plugin is installed 0555 rather than 775. Group write on an executable the server exec's is not needed by anything. README.md still told readers to clone the upstream project and place binaries by hand. It now documents the in-tree build. build-jwt-plugin.sh deleted a caller-supplied WORK_DIR on exit. It now removes only a directory it created itself. smoke-jwt-plugin-runtime.sh wrote a dev root token, server logs and status output to fixed /tmp paths. It now uses a private 0700 directory removed on exit; predictable names in a shared /tmp are both a disclosure risk and a collision between concurrent runs. The UUID version test asserted on uuid.NewRandom() directly, which tested the uuid package rather than this code. It now decodes what friendlyIdGenerator.id() actually returns and asserts version 4 and the RFC4122 variant. Confirmed the test fails when the v1 call is reintroduced and passes when it is restored. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
check-dependency-docs regenerates dependencies.md and fails if the committed copy differs. The imported JWT plugin adds eleven Go modules to the repository-wide inventory, mostly hashicorp/go-secure-stdlib and crypto transitives pulled in by the vault SDK. Regenerated with `GOWORK=off go run -C ./tools/collect-dependencies .`, which is what CI runs. Also satisfies the OSRB report's requirement that dependency counts be regenerated once friendlyid-go is removed, which this branch does. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
OpenBao is approved for GitHub. Its image bakes in a JWT secrets plugin that is currently built by cloning a GitLab-only fork at a pinned revision, so a reproducible public build needs that source here.
What this is, and what it is not
The plugin is third-party: outfoxx/vault-plugin-secrets-jwt, Apache-2.0, Copyright 2021 Outfox, Inc. Authorship across its 74 commits is Kevin Wooten 64, Ian Fox 9, Brad Vernon 1. Exactly one commit is NVIDIA's.
So the usual "apply NVIDIA headers, copyright, NOTICE" pass would have been wrong here. Attribution is preserved instead of overwritten:
LICENSE(11,359 bytes) and upstreamHEADERretainedNOTICEenumerates every modification, as Apache-2.0 section 4(b) requiresAGENTS.mdtells the repository copyright stamper to stay outNOTICEwas written against the actual diff, not from the commit message. The commit says "allow settingaud"; the code disables the guard rejectingsubin a role's claims and registers aReadOperationon the signing path.friendlyid-go is removed, not imported
The image's plugin depended on
github.com/mariuszs/friendlyid-go. Upstream carries no license at all (GitHub API reportsNONE;LICENSE404s on both branches), so it is not redistributable. The openbao repo's ownopenbao-osrb-report.txtalready required its removal "with no source copied from friendlyid-go".plugin/friendlyid.gois an independent base62 implementation written from the definition. The plugin used exactly one function,Encode;Decodewas never called. Tests cover fixed width (including zero and max UUID), round-trip against a test-side decoder, collisions over 500 ids, alphabet containment, and the generator wiring.Verified: friendlyid-go is absent from
go.mod,go.sum, all sources, andgo list -m all.Placement
infra/openbao/plugins/vault-plugin-secrets-jwt.src/is first-party NVCF code and this is not, so placing it there would assert authorship we do not have. Nesting underinfra/openbaokeeps the source with the Dockerfile and build script that pin it, matching theinfra/cassandrashape. There is nothird_party/tree today and inventing one for a single consumer separates the fork from the script that pins it.The module path is renamed to the in-repo path. We do not track upstream, so resolving inside the monorepo is worth more than keeping diffs readable against Outfox. Apache-2.0 requires retaining the license, notices and a statement of changes; a rename does none of those harms.
Build strategy
GitHub. The module is deliberately not in
go.work.bazeland has noBUILD.bazel, so the Bazel matrix never sees it. Its graph is 278 modules includinghashicorp/vault/apiandhashicorp/vault/sdk, and the root module uses no hashicorp/vault at all. Joining the root graph would put all of that into minimal version selection for every service in the repository, to build one plugin binary that ships inside one image.The cost of that isolation is that nothing would otherwise compile or test this code, which matters now that NVIDIA owns modifications to it.
.github/workflows/openbao-jwt-plugin.ymlcloses that: plain go, scoped to the one directory, path-filtered. It also asserts friendlyid-go stays out of the module graph, since its return would be a licensing regression rather than a build failure and nothing else would catch it.GitLab. Unchanged by this PR. nvcf-internal continues to build the openbao image through the Dockerfile backend, and the openbao CI is preserved as-is. The follow-up that adds
infra/openbaorepointsbuild-jwt-plugin.shat this in-repo source instead of cloning the GitLab fork.Not imported on purpose
Upstream project machinery:
.github/workflows/above all, which would otherwise become live workflows in this repository, plus goreleaser config, the upstream Dockerfile, install script, Makefile and linter config. Source, tests and license material were retained in full.Testing
go build ./...andgo test ./...pass under the new module path, including the plugin's pre-existing suite.Follow-ups
infra/openbaoitself: publicDockerfileand scripts, withbuild-jwt-plugin.shrepointed at this sourceoverlays/openbaofor the plugin binaries, which stay out of the OSS snapshotSummary by CodeRabbit