Conversation
📝 WalkthroughWalkthroughAdds a new Comment |
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
Nice solution - the env var means we can skip a command step in CI and the fingerprint inclusion is 😚 👌
…print verification - Add MIREN_CLUSTER env var parsing in LoadCluster with address;sha1:fingerprint format - Add cluster export-address command that outputs address;sha1:<fingerprint> for use with MIREN_CLUSTER, honoring the -C flag instead of a positional argument - Extract checkFingerprint helper from setupClusterFromAddress for testability - Move extractTLSCertificate to accept context.Context and setupClusterFromAddress into config.go alongside the env var logic it supports - Allow cluster add in manual mode (--cluster + --address) without requiring an identity - Add tests for fingerprint verification, LoadCluster env var paths, and export-address
evanphx
force-pushed
the
mir-739-fix-ability-to-target-a-cluster-in-oidc-auth-mode
branch
from
99b5ce8 to
c6fc9cf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/commands/config.go`:
- Around line 124-131: The checkFingerprint function fails to strip uppercase
"SHA1:" prefixes because strings.TrimPrefix is case-sensitive; update
checkFingerprint to remove the "sha1:" prefix case-insensitively (e.g., check
strings.HasPrefix(strings.ToLower(expected), "sha1:") and then slice off the
prefix from the original expected string, or lowercase a copy and TrimPrefix on
that copy) before the case-insensitive comparison with strings.EqualFold; change
the existing strings.TrimPrefix(expected, "sha1:") usage in checkFingerprint
accordingly.
- Around line 39-42: Replace direct equality checks against
clientconfig.ErrNoConfig with the idiomatic errors.Is(...) pattern: where the
code currently does `if err == clientconfig.ErrNoConfig &&
os.Getenv("MIREN_CLUSTER") != "" { ... }` (and the similar check around lines
47-50), change to `if errors.Is(err, clientconfig.ErrNoConfig) &&
os.Getenv("MIREN_CLUSTER") != "" { ... }`; import the standard "errors" package
if not already imported and keep the existing behavior of creating a new config
via clientconfig.NewConfig() and returning c.cfg, nil.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
cli/commands/cluster_add.gocli/commands/cluster_export.gocli/commands/cluster_export_test.gocli/commands/commands.gocli/commands/config.gocli/commands/config_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/commands/config_test.go
- cli/commands/cluster_export.go
- cli/commands/cluster_export_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cli/commands/config.go (1)
124-131:⚠️ Potential issue | 🟡 Minor
checkFingerprintdoes not handle uppercaseSHA1:prefix.
strings.TrimPrefix(expected, "sha1:")is case-sensitive, so an expected value likeSHA1:abcdef...will fail comparison even though the fingerprint matching is otherwise case-insensitive.🔧 Proposed fix for case-insensitive prefix handling
func checkFingerprint(expected, actual string) error { if expected == "" { return nil } - expected = strings.TrimPrefix(expected, "sha1:") + if strings.HasPrefix(strings.ToLower(expected), "sha1:") { + expected = expected[len("sha1:"):] + } if !strings.EqualFold(expected, actual) { return fmt.Errorf("TLS certificate fingerprint mismatch: got %s, expected %s", actual, expected) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/commands/config.go` around lines 124 - 131, The checkFingerprint function fails to strip a case-insensitive "sha1:" prefix because strings.TrimPrefix("sha1:") is case-sensitive; modify checkFingerprint to remove the prefix regardless of case (e.g., normalize expected with strings.ToLower or detect prefix with strings.HasPrefix(strings.ToLower(expected), "sha1:") and then strip the first len("sha1:") characters) before comparing with strings.EqualFold, keeping the rest of the logic in checkFingerprint unchanged.
🧹 Nitpick comments (1)
cli/commands/config.go (1)
140-141: Consider adding a timeout for the TLS probe.The context has no deadline, so
extractTLSCertificatecould block indefinitely if the target address is reachable but the TLS handshake hangs. For CLI responsiveness, a reasonable timeout (e.g., 10-30 seconds) would improve user experience.⏱️ Proposed fix to add timeout
- ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel()Note: This requires adding
"time"to the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/commands/config.go` around lines 140 - 141, The current ctx created with context.WithCancel in the TLS probe can hang indefinitely; replace it with a context with deadline (e.g., use context.WithTimeout instead of context.WithCancel) when calling extractTLSCertificate and set a reasonable timeout (10–30s, e.g., 20s), keep the defer cancel(), and add the "time" import; update references to ctx and cancel in this scope so extractTLSCertificate receives the timed context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cli/commands/config.go`:
- Around line 124-131: The checkFingerprint function fails to strip a
case-insensitive "sha1:" prefix because strings.TrimPrefix("sha1:") is
case-sensitive; modify checkFingerprint to remove the prefix regardless of case
(e.g., normalize expected with strings.ToLower or detect prefix with
strings.HasPrefix(strings.ToLower(expected), "sha1:") and then strip the first
len("sha1:") characters) before comparing with strings.EqualFold, keeping the
rest of the logic in checkFingerprint unchanged.
---
Nitpick comments:
In `@cli/commands/config.go`:
- Around line 140-141: The current ctx created with context.WithCancel in the
TLS probe can hang indefinitely; replace it with a context with deadline (e.g.,
use context.WithTimeout instead of context.WithCancel) when calling
extractTLSCertificate and set a reasonable timeout (10–30s, e.g., 20s), keep the
defer cancel(), and add the "time" import; update references to ctx and cancel
in this scope so extractTLSCertificate receives the timed context.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cli/commands/config.go
evanphx
deleted the
mir-739-fix-ability-to-target-a-cluster-in-oidc-auth-mode
branch
2 participants
Summary
LoadClusternow parsesMIREN_CLUSTERwith optionaladdress;sha1:fingerprintformat for targeting clusters without a config file, with TLS certificate fingerprint verification (TOFU protection)cluster export-addresscommand: Exports a cluster's address and CA cert fingerprint in the format consumed byMIREN_CLUSTER. Honors the-Cflag (not a positional arg) and falls back to the active clustercluster addmanual mode: When both--clusterand--addressare provided, identity is no longer required — supports OIDC and environment-based authcheckFingerprinthelper: Extracted fromsetupClusterFromAddressfor direct unit testing of the security-critical fingerprint comparison logicTestCheckFingerprint(prefix stripping, case insensitivity, mismatch errors),TestLoadClusterEnvVar(known names, address fallthrough, fingerprint splitting),TestClusterExportAddress(active cluster,-Cflag,-Cpriority, error cases)Test plan
go test ./cli/commands/... -run 'TestCheckFingerprint|TestLoadClusterEnvVar|TestClusterExportAddress' -v— all 15 tests passgo test ./cli/commands/...suite passesmiren cluster export-addressoutputs correct fingerprint for active clustermiren cluster export-address -C <name>targets the right clusterMIREN_CLUSTER=<exported-value> miren app listconnects to the expected cluster