Support opaque (non-JWT) tokens for sandbox environments#1116
Open
geowa4 wants to merge 1 commit into
Open
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: geowa4 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (23)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (18)
Summary by CodeRabbit
WalkthroughAdds opaque-token mode: config flag and detection, CLI flag/env wiring, connection transport injecting Authorization header, conditional token persistence in commands, JWT-inspection gating in token output, tests, and README documentation. ChangesOpaque Token Feature
Estimated code review effort: 🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
geowa4
force-pushed
the
support-opaque-tokens
branch
from
4e38a63 to
f1a31ae
Compare
Author
|
Manual testing: ❯ bunx @earendil-works/gondolin bash --host-secret OCM_TOKEN@api.openshift.com=$(ocm token) --env OCM_URL='https://api.openshift.com' --env OCM_TOKEN_URL='https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token' --image gondolin-ocm:latest --mount-hostfs /Users/geowa4/Dev/repos/ocm-cli:/ocm-cli
(none):/# ocm config set access_token $OCM_TOKEN
(none):/# ocm config set token_url $OCM_TOKEN_URL
(none):/# ocm config set url $OCM_URL
(none):/# cat /tmp/.config/ocm/ocm.json
{
"access_token": "GONDOLIN_SECRET_54da441784b1fe3eef888aad85071e70f8ca4b1ea4617343",
"token_url": "https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token",
"url": "https://api.openshift.com"
}(none):/# OCM_OPAQUE_TOKEN=true ./ocm-cli/ocm-linux-arm64 whoami
{
...omitted...
"username":"geowa4.openshift"
}
(none):/# ./ocm-cli/ocm-linux-arm64 whoami
Error: Failed to create OCM connection: token contains an invalid number of segments
(none):/# ./ocm-cli/ocm-linux-arm64 --opaque-token whoami
{
...omitted...
"username":"geowa4.openshift"
}
(none):/# ./ocm-cli/ocm-linux-arm64 config set opaque_token true
(none):/# ./ocm-cli/ocm-linux-arm64 whoami
{
...omitted...
"username":"geowa4.openshift"
} |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/opaquetoken/flag.go (1)
34-35: ⚡ Quick winUse the shared env-key constant in the flag help text.
The help text duplicates
OCM_OPAQUE_TOKENas a raw string instead of reusingproperties.OpaqueTokenEnvKey, which risks drift if the key changes.Proposed diff
import ( + "fmt" "os" "strconv" "github.com/spf13/pflag" "github.com/openshift-online/ocm-cli/pkg/properties" ) @@ flags.BoolVar( &enabled, "opaque-token", false, "Treat the access token as an opaque (non-JWT) token. Can also be "+ - "enabled by setting the OCM_OPAQUE_TOKEN environment variable.", + fmt.Sprintf("enabled by setting the %s environment variable.", properties.OpaqueTokenEnvKey), ) }🤖 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 `@pkg/opaquetoken/flag.go` around lines 34 - 35, The flag help text currently embeds the literal "OCM_OPAQUE_TOKEN"; update it to reference the shared constant properties.OpaqueTokenEnvKey instead so the help message stays in sync with the env-key definition—locate the flag help string in pkg/opaquetoken/flag.go (the variable that defines the flag help) and replace the raw env var text with the properties.OpaqueTokenEnvKey symbol in the concatenated help string.pkg/config/config_test.go (1)
178-201: ⚡ Quick winAdd a refresh-token-only opaque-mode test case.
The new opaque coverage only exercises access-token paths. Please add a case asserting that opaque mode is not armed when only
RefreshTokenis present, so this regression path stays protected.Suggested test addition
+ It("Is not armed in opaque mode when only refresh token is set", func() { + config := &Config{ + RefreshToken: "opaque_refresh_only", + OpaqueToken: true, + URL: "http://my-server.example.com", + TokenURL: "http://my-sso.example.com", + } + armed, reason, err := config.Armed(true) + Expect(err).ToNot(HaveOccurred()) + Expect(armed).To(BeFalse()) + Expect(reason).To(Equal("credentials aren't set")) + })🤖 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 `@pkg/config/config_test.go` around lines 178 - 201, Add a new unit test that constructs a Config with OpaqueToken set (e.g., OpaqueToken: true), no AccessToken, and only RefreshToken populated (RefreshToken: "some-refresh-token"), then call config.Armed(true) and assert that armed is false (and check error/no-error per existing behavior); place the test alongside the existing cases and reference the Config type and Armed method to ensure opaque mode is not considered armed when only RefreshToken is present.
🤖 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 `@cmd/ocm/token/cmd.go`:
- Around line 113-120: The branch that handles opaque tokens (haveOpaqueToken
computed from opaquetoken.Enabled() || (cfg != nil && cfg.OpaqueToken)) can
dereference cfg when opaquetoken.Enabled() is true but cfg == nil; update the
opaque-token branch in cmd.go so that after detecting haveOpaqueToken you
explicitly check if cfg == nil and return a clear error (or handle missing
tokens) before accessing cfg.AccessToken and cfg.RefreshToken; ensure the
existing args.generate check remains and reference the symbols haveOpaqueToken,
opaquetoken.Enabled(), cfg, accessToken, refreshToken and args.generate when
making the change.
In `@pkg/config/config.go`:
- Around line 214-215: The current check sets refreshUsable when either
opaqueMode is true or IsEncryptedToken(c.RefreshToken) is true, which
erroneously treats a refresh token as usable in opaque mode; change the
condition in the logic that sets refreshUsable so that opaqueMode does NOT
enable it (e.g., only set refreshUsable when !opaqueMode &&
IsEncryptedToken(c.RefreshToken)), and verify any later use (the
armed-state/reporting logic that reads refreshUsable) relies on this corrected
flag so a service in opaqueMode won't be reported as "armed" based solely on
c.RefreshToken.
In `@README.md`:
- Around line 170-190: Replace the unlabeled code fences and prompt-style lines
in the README examples so markdownlint is satisfied: change the three
triple-backtick blocks containing the commands "ocm --opaque-token get
/api/clusters_mgmt/v1/clusters", the block with "export OCM_OPAQUE_TOKEN=true" /
"ocm get /api/clusters_mgmt/v1/clusters", and the block with "ocm login
--token=..." / "ocm config set access_token ..." / "ocm config set opaque_token
true" to use fenced code blocks labeled with bash (```bash) and remove leading
"$ " prompts from each command line so the examples are clean shell snippets.
---
Nitpick comments:
In `@pkg/config/config_test.go`:
- Around line 178-201: Add a new unit test that constructs a Config with
OpaqueToken set (e.g., OpaqueToken: true), no AccessToken, and only RefreshToken
populated (RefreshToken: "some-refresh-token"), then call config.Armed(true) and
assert that armed is false (and check error/no-error per existing behavior);
place the test alongside the existing cases and reference the Config type and
Armed method to ensure opaque mode is not considered armed when only
RefreshToken is present.
In `@pkg/opaquetoken/flag.go`:
- Around line 34-35: The flag help text currently embeds the literal
"OCM_OPAQUE_TOKEN"; update it to reference the shared constant
properties.OpaqueTokenEnvKey instead so the help message stays in sync with the
env-key definition—locate the flag help string in pkg/opaquetoken/flag.go (the
variable that defines the flag help) and replace the raw env var text with the
properties.OpaqueTokenEnvKey symbol in the concatenated help string.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9ef73458-6be2-4494-bbc5-d0c5c1d788a6
📒 Files selected for processing (23)
README.mdcmd/ocm/config/get/get.gocmd/ocm/config/set/set.gocmd/ocm/delete/cmd.gocmd/ocm/get/cmd.gocmd/ocm/login/cmd.gocmd/ocm/main.gocmd/ocm/patch/cmd.gocmd/ocm/post/cmd.gocmd/ocm/token/cmd.gopkg/arguments/arguments.gopkg/config/config.gopkg/config/config_test.gopkg/config/token.gopkg/ocm/connection-builder/connection.gopkg/opaquetoken/flag.gopkg/opaquetoken/flag_test.gopkg/opaquetoken/main_test.gopkg/properties/properties.gotests/get_test.gotests/login_test.gotests/logout_test.gotests/token_test.go
geowa4
force-pushed
the
support-opaque-tokens
branch
from
f1a31ae to
2a831d0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
README.md (1)
170-191:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe markdownlint violations flagged in the past review are still present.
The code fence language specifiers and shell prompt style issues identified in the previous review remain unaddressed. The past review comment provides a ready-to-apply fix.
🤖 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 `@README.md` around lines 170 - 191, Add language specifiers to the fenced code blocks and remove shell prompt characters so markdownlint errors are resolved: change the three backtick fences to use "bash" (e.g., ```bash) for both command blocks and remove the leading "$ " prompt from each command line inside those fences (leave commands themselves intact), ensuring the examples under the ocm get and the export/config examples are plain bash commands without prompt characters.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@README.md`:
- Around line 170-191: Add language specifiers to the fenced code blocks and
remove shell prompt characters so markdownlint errors are resolved: change the
three backtick fences to use "bash" (e.g., ```bash) for both command blocks and
remove the leading "$ " prompt from each command line inside those fences (leave
commands themselves intact), ensuring the examples under the ocm get and the
export/config examples are plain bash commands without prompt characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 26a44f7c-24b8-48db-a859-d696ba8bc6b4
📒 Files selected for processing (23)
README.mdcmd/ocm/config/get/get.gocmd/ocm/config/set/set.gocmd/ocm/delete/cmd.gocmd/ocm/get/cmd.gocmd/ocm/login/cmd.gocmd/ocm/main.gocmd/ocm/patch/cmd.gocmd/ocm/post/cmd.gocmd/ocm/token/cmd.gopkg/arguments/arguments.gopkg/config/config.gopkg/config/config_test.gopkg/config/token.gopkg/ocm/connection-builder/connection.gopkg/opaquetoken/flag.gopkg/opaquetoken/flag_test.gopkg/opaquetoken/main_test.gopkg/properties/properties.gotests/get_test.gotests/login_test.gotests/logout_test.gotests/token_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/opaquetoken/main_test.go
🚧 Files skipped from review as they are similar to previous changes (18)
- tests/logout_test.go
- tests/get_test.go
- cmd/ocm/delete/cmd.go
- cmd/ocm/main.go
- tests/login_test.go
- pkg/config/token.go
- pkg/arguments/arguments.go
- pkg/opaquetoken/flag.go
- cmd/ocm/config/set/set.go
- pkg/properties/properties.go
- cmd/ocm/get/cmd.go
- pkg/config/config.go
- cmd/ocm/post/cmd.go
- cmd/ocm/patch/cmd.go
- pkg/ocm/connection-builder/connection.go
- cmd/ocm/config/get/get.go
- cmd/ocm/token/cmd.go
- pkg/config/config_test.go
geowa4
force-pushed
the
support-opaque-tokens
branch
from
2a831d0 to
1e41da9
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/config.go (1)
214-221:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip refresh token validation in opaque mode to prevent spurious errors.
When
opaqueModeis true, the condition!opaqueMode && IsEncryptedToken(...)is always false, sotokenUsable()is called on the refresh token. If a stale or malformed refresh token exists in config, this will return an error and causeArmed()to fail—even though the opaque access token is valid.This is related to a past review concern about refresh token handling in opaque mode. The safest behavior is to skip refresh token validation entirely in opaque mode, since the feature relies solely on the access token.
Proposed fix
haveRefresh := c.RefreshToken != "" refreshUsable := false if haveRefresh { - if !opaqueMode && IsEncryptedToken(c.RefreshToken) { + if opaqueMode { + // In opaque mode, skip refresh token validation entirely. + // Authentication relies on the opaque access token. + refreshUsable = false + } else if IsEncryptedToken(c.RefreshToken) { refreshUsable = true } else { refreshUsable, err = tokenUsable(c.RefreshToken, 10*time.Second) if err != nil { return } } }🤖 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 `@pkg/config/config.go` around lines 214 - 221, The refresh-token validation currently runs even in opaqueMode (causing Armed() to fail on stale/malformed refresh tokens); change the conditional around opaqueMode/IsEncryptedToken/tokenUsable so that when opaqueMode is true you do not call tokenUsable() at all (e.g., short-circuit: if opaqueMode { set refreshUsable = false } else if IsEncryptedToken(c.RefreshToken) { refreshUsable = true } else { refreshUsable, err = tokenUsable(c.RefreshToken, 10*time.Second) ... }). Ensure you remove any early return on error in the opaque branch so tokenUsable is never invoked when opaqueMode is true.
🤖 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.
Outside diff comments:
In `@pkg/config/config.go`:
- Around line 214-221: The refresh-token validation currently runs even in
opaqueMode (causing Armed() to fail on stale/malformed refresh tokens); change
the conditional around opaqueMode/IsEncryptedToken/tokenUsable so that when
opaqueMode is true you do not call tokenUsable() at all (e.g., short-circuit: if
opaqueMode { set refreshUsable = false } else if
IsEncryptedToken(c.RefreshToken) { refreshUsable = true } else { refreshUsable,
err = tokenUsable(c.RefreshToken, 10*time.Second) ... }). Ensure you remove any
early return on error in the opaque branch so tokenUsable is never invoked when
opaqueMode is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 37327473-e968-40bc-bae8-14b20912d20c
📒 Files selected for processing (23)
README.mdcmd/ocm/config/get/get.gocmd/ocm/config/set/set.gocmd/ocm/delete/cmd.gocmd/ocm/get/cmd.gocmd/ocm/login/cmd.gocmd/ocm/main.gocmd/ocm/patch/cmd.gocmd/ocm/post/cmd.gocmd/ocm/token/cmd.gopkg/arguments/arguments.gopkg/config/config.gopkg/config/config_test.gopkg/config/token.gopkg/ocm/connection-builder/connection.gopkg/opaquetoken/flag.gopkg/opaquetoken/flag_test.gopkg/opaquetoken/main_test.gopkg/properties/properties.gotests/get_test.gotests/login_test.gotests/logout_test.gotests/token_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/properties/properties.go
- pkg/config/token.go
🚧 Files skipped from review as they are similar to previous changes (17)
- cmd/ocm/main.go
- cmd/ocm/config/get/get.go
- tests/login_test.go
- cmd/ocm/config/set/set.go
- cmd/ocm/patch/cmd.go
- tests/get_test.go
- cmd/ocm/delete/cmd.go
- cmd/ocm/post/cmd.go
- pkg/opaquetoken/flag_test.go
- pkg/opaquetoken/flag.go
- tests/token_test.go
- pkg/arguments/arguments.go
- cmd/ocm/get/cmd.go
- cmd/ocm/login/cmd.go
- cmd/ocm/token/cmd.go
- pkg/ocm/connection-builder/connection.go
- pkg/config/config_test.go
Add --opaque-token flag and OCM_OPAQUE_TOKEN env var for environments that use opaque tokens instead of JWTs. When enabled, the CLI sends the access token as-is via a custom HTTP transport, bypassing SDK token management and JWT parsing. - Add opaquetoken package for flag/env var handling - Use unauthenticated SDK connection with custom transport wrapper - Skip token refresh in API commands (GET/POST/PATCH/DELETE) - Add opaque_token to config get/set commands - Reject --opaque-token during login with guidance message - Add README documentation for per-invocation and persistent modes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
geowa4
force-pushed
the
support-opaque-tokens
branch
from
1e41da9 to
7bcad6b
Compare
6 tasks
rcampos2029
added
the
priority/important-soon
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.
Summary
--opaque-tokenglobal flag andOCM_OPAQUE_TOKENenvironment variable for environments that use opaque (non-JWT) tokensopaque_tokentoocm config getandocm config setcommands--opaque-tokenduringocm loginwith a guidance message showing the correct workflowTest plan
make test— 232 specs, 0 failures)opaquetoken.Enabled()flag and env var handlingconfig.IsJWTToken()helper--opaque-tokenopaque_tokenconfigocm tokenwith opaque tokens (display,--generaterejection, JWT-only option rejection)🤖 Generated with Claude Code