Add OIDC Authentication to caib#49
Conversation
📝 WalkthroughWalkthroughAdds OIDC PKCE authentication to the CLI, server-side support for internal HS256 and external JWT/OIDC authenticators sourced from OperatorConfig, client TLS CA/insecure options, operator secret management and CRD/schema additions, many vendor dependencies, Docker CA copies, and unit/e2e tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CAIB CLI
participant Browser as Browser
participant OIDC as OIDC Provider
participant API as Build API Server
participant K8s as Kubernetes
CLI->>CLI: check token cache
alt cached token valid
CLI-->>API: request with Bearer token
else
CLI->>API: GET /v1/auth/config
API-->>CLI: OIDC config or empty
alt OIDC configured
CLI->>OIDC: GET /.well-known/openid-configuration
OIDC-->>CLI: discovery endpoints
CLI->>CLI: generate PKCE verifier/challenge
CLI->>Browser: open auth URL
Browser->>OIDC: user authenticates
OIDC->>Browser: redirect to localhost (code)
Browser->>CLI: deliver code to local callback
CLI->>OIDC: POST token (code + verifier)
OIDC-->>CLI: id/access token
CLI->>CLI: save token to cache
CLI-->>API: request with Bearer token
else
CLI-->>API: proceed without OIDC
end
end
API->>K8s: validate token via authenticators (JWKS/HS256/TokenReview)
K8s-->>API: validation result
API-->>CLI: response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@cmd/caib/auth/oidc.go`:
- Around line 357-361: The generateRandomString function currently ignores the
error from rand.Read; change its signature to return (string, error) so it
checks the error from crypto/rand.Read(b) and returns an explicit error on
failure, and update all callers (e.g., where generateRandomString is used for
PKCE code verifier and state) to handle the error (propagate or fail the
request) rather than assuming a value was produced.
In `@cmd/caib/auth/wrapper.go`:
- Around line 60-71: In CreateClientWithReauth, add a nil guard for the
authToken pointer before dereferencing it and handle errors from
GetTokenWithReauth instead of ignoring them: check if authToken == nil and
return a clear error (or allocate a local token string) before using *authToken;
when calling GetTokenWithReauth, capture and propagate the error (or return it)
rather than proceeding silently so OIDC failures don't result in an
unauthenticated client; ensure the final buildapiclient.New call (with
buildapiclient.WithAuthToken(strings.TrimSpace(*authToken))) only runs with a
validated token value or after correctly returning an error.
- Around line 28-35: The code currently swallows errors from
GetOIDCConfigFromLocalConfig() and GetOIDCConfigFromAPI() and returns "", false,
nil which hides real failures; change the fallback branch so that when
GetOIDCConfigFromAPI(serverURL) returns an error you propagate that error (e.g.,
return "", false, err) or wrap it with context instead of returning nil; locate
the wrapper function around this diff and modify the error path after
GetOIDCConfigFromAPI to return/forward the error (or a wrapped error) so callers
can distinguish “OIDC not configured” from “failed to fetch OIDC config”.
In `@cmd/caib/main.go`:
- Around line 82-112: The current createBuildAPIClient logic falls back to
loadTokenFromKubeconfig when auth.GetTokenWithReauth returns a non-nil error,
which masks real OIDC failures; change the flow so that when
auth.GetTokenWithReauth returns a non-nil error you return that error (or
propagate it) instead of silently loading kubeconfig, and only attempt
kubeconfig fallback when err == nil and the returned token is empty; update the
branch handling inside createBuildAPIClient around the auth.GetTokenWithReauth
call to set *authToken on success, try kubeconfig only for the explicit "no OIDC
configured" case (err == nil && token == ""), and propagate any non-nil err from
auth.GetTokenWithReauth.
In `@go.mod`:
- Line 10: Update the vulnerable dependency github.com/coreos/go-oidc (currently
v2.3.0+incompatible in go.mod) to a patched release (prefer v2.4.0+ or v3.15.0+)
to remediate CVE-2025-27144; modify go.mod to replace the existing
github.com/coreos/go-oidc entry with the chosen safe version and run the module
update command (e.g., go get github.com/coreos/go-oidc@v2.4.0 or `@v3.15.0`) and
go mod tidy to ensure transitive updates are applied.
In `@internal/buildapi/client/client.go`:
- Around line 60-68: The current code creates a zero-value http.Transport which
destroys proxy, HTTP/2, pooling and other defaults; instead, in WithInsecureTLS
and WithCACertificate alter the existing transport by cloning
http.DefaultTransport (http.DefaultTransport.Clone()) when
c.httpClient.Transport is nil, then ensure TLSClientConfig is non-nil and set
the desired fields (InsecureSkipVerify or RootCAs) on that cloned transport and
assign it to c.httpClient.Transport; also handle the case where
c.httpClient.Transport is already an *http.Transport by setting TLSClientConfig
if nil and modifying it there (use transport := http.DefaultTransport.Clone() to
seed defaults when nil).
In `@internal/buildapi/internal_jwt.go`:
- Around line 37-64: The function validateInternalJWT currently accepts tokens
with an empty Subject and may dereference cfg without a nil-check; update
validateInternalJWT to explicitly reject tokens where claims.Subject == ""
(return "", false) before returning, and add a defensive nil check for cfg at
the start (return "", false if cfg == nil) so you don't dereference cfg. Locate
and modify validateInternalJWT and its uses of cfg, claims.Subject, and
cfg.audience/cfg.issuer to implement these checks.
In `@vendor/github.com/coreos/go-oidc/test`:
- Around line 12-13: Remove the deprecated invocation "go test -v -i -race
github.com/coreos/go-oidc/..." and keep the existing "go test -v -race
github.com/coreos/go-oidc/..." command instead; simply delete the -i variant so
the script/Makefile only runs the supported race-enabled test command.
In `@vendor/gopkg.in/go-jose/go-jose.v2/cipher/cbc_hmac.go`:
- Around line 35-61: The function NewCBCHMAC can leave the local variable hash
nil for unsupported key lengths which later causes hmac.New to panic; update
NewCBCHMAC to validate the provided key length up front (compute keySize :=
len(key)/2 and ensure len(key) is even and keySize is one of 16, 24 or 32), and
if not, return a clear error (e.g., invalid key size) before calling
newBlockCipher or using hash; keep the existing switch (hash = ...) for the
supported sizes and return the cbcAEAD only when validation succeeds.
🟡 Minor comments (15)
vendor/github.com/coreos/go-oidc/CONTRIBUTING.md-49-66 (1)
49-66: Add language identifiers to fenced code blocks.markdownlint’s fenced-code-language rule will flag these blocks. Adding a language (e.g.,
text) keeps lint clean.🛠️ Suggested fix
-``` +```text scripts: add the test-cluster command this uses tmux to setup a test cluster that you can easily kill and start for debugging. Fixes `#38` -``` +``` -``` +```text <subsystem>: <what changed> <BLANK LINE> <why this change was made> <BLANK LINE> <footer> -``` +```vendor/github.com/coreos/go-oidc/code-of-conduct.md-39-41 (1)
39-41: Fix markdownlint MD034 (bare URLs).Wrap bare URLs to satisfy linting.
✅ Proposed fix
-This Code of Conduct is adapted from the Contributor Covenant -(http://contributor-covenant.org), version 1.2.0, available at -http://contributor-covenant.org/version/1/2/0/ +This Code of Conduct is adapted from the Contributor Covenant +(<http://contributor-covenant.org>), version 1.2.0, available at +<http://contributor-covenant.org/version/1/2/0/>vendor/github.com/pquerna/cachecontrol/README.md-17-38 (1)
17-38: Replace hard tabs in code blocks to satisfy markdownlint.Lines 21–37 and 49–91 contain hard tabs (MD010). Consider converting indentation to spaces or excluding vendored docs from linting.
Also applies to: 45-92
vendor/github.com/pquerna/cachecontrol/README.md-97-103 (1)
97-103: Fix spelling/capitalization in the Credits section.Line 97 should use “GitHub”, and Line 101 has a typo.
✏️ Proposed wording fix
-Please [open issues in Github](https://github.com/pquerna/cachecontrol/issues) for ideas, bugs, and general thoughts. Pull requests are of course preferred :) +Please [open issues in GitHub](https://github.com/pquerna/cachecontrol/issues) for ideas, bugs, and general thoughts. Pull requests are of course preferred :) @@ -`cachecontrol` has recieved significant contributions from: +`cachecontrol` has received significant contributions from:internal/buildapi/client_tokens.go-114-116 (1)
114-116: Reduce secret-name collision risk by avoiding heavy truncation.Line 116 limits the hash to 12 hex chars (~48 bits), which increases collision probability as user count grows and could overwrite another user’s token secret. Consider using the full hash (or a longer prefix).
🛠️ Proposed fix
func hashName(value string) string { sum := sha1.Sum([]byte(value)) - return hex.EncodeToString(sum[:])[:12] + return hex.EncodeToString(sum[:]) }vendor/github.com/pquerna/cachecontrol/README.md-41-44 (1)
41-44: Hyphenate compound adjectives for readability.Line 43–44 uses “high level”, “lower level”, and “high performance”; these should be hyphenated.
✏️ Proposed wording tweak
-`cachecontrol` is divided into two packages: `cachecontrol` with a high level API, and a lower level `cacheobject` package. Use [Object](https://godoc.org/github.com/pquerna/cachecontrol/cacheobject#Object) in a high performance use case where you have previously parsed headers containing dates or would like to avoid memory allocations. +`cachecontrol` is divided into two packages: `cachecontrol` with a high-level API, and a lower-level `cacheobject` package. Use [Object](https://godoc.org/github.com/pquerna/cachecontrol/cacheobject#Object) in a high-performance use case where you have previously parsed headers containing dates or would like to avoid memory allocations.vendor/github.com/golang-jwt/jwt/v5/MIGRATION_GUIDE.md-25-39 (1)
25-39: Address markdownlint warnings in this guide.Lint flags list indentation, fenced code blocks without language, hard tabs, and a bare URL; please normalize formatting to avoid doc‑lint failures.
vendor/github.com/golang-jwt/jwt/v5/MIGRATION_GUIDE.md-95-99 (1)
95-99: Fix typo that changes meaning.“once could easily disable” → “one could easily disable”.
✏️ Proposed fix
-Previously, users could override the `Valid` method in a custom claim, for -example to extend the validation with application-specific claims. However, this -was always very dangerous, since once could easily disable the standard +Previously, users could override the `Valid` method in a custom claim, for +example to extend the validation with application-specific claims. However, this +was always very dangerous, since one could easily disable the standardcmd/caib/auth/wrapper.go-11-20 (1)
11-20: Fragile string-based error detection.Matching error substrings like
"401","403","unauthorized", or"forbidden"is brittle—these patterns could appear in unrelated error messages (e.g., "resource ID 4013 not found") or fail to match if error formatting changes.Consider defining typed errors in the
buildapiclientpackage and usingerrors.Is()orerrors.As()for reliable detection.Suggested approach
// In internal/buildapi/client/errors.go var ( ErrUnauthorized = errors.New("unauthorized") ErrForbidden = errors.New("forbidden") ) // In wrapper.go func IsAuthError(err error) bool { if err == nil { return false } return errors.Is(err, buildapiclient.ErrUnauthorized) || errors.Is(err, buildapiclient.ErrForbidden) }vendor/github.com/golang-jwt/jwt/v5/types.go-86-88 (1)
86-88: Typo in error message: "NumericData" should be "NumericDate".Minor typo in the error message that could cause confusion during debugging.
Suggested fix
if err = json.Unmarshal(b, &number); err != nil { - return fmt.Errorf("could not parse NumericData: %w", err) + return fmt.Errorf("could not parse NumericDate: %w", err) }cmd/caib/auth/config.go-78-80 (1)
78-80: Handleos.UserHomeDir()error to avoid writing to unexpected paths.If the home directory cannot be determined (e.g., in containerized environments without
HOMEset),homeDirwill be an empty string, resulting in paths like/.caib/config.jsonwhich could fail or cause unintended behavior.Proposed fix
func GetOIDCConfigFromLocalConfig() (*OIDCConfig, error) { - homeDir, _ := os.UserHomeDir() + homeDir, err := os.UserHomeDir() + if err != nil { + return nil, fmt.Errorf("failed to get home directory: %w", err) + } configPath := filepath.Join(homeDir, tokenCacheDir, "config.json")cmd/caib/auth/config.go-114-116 (1)
114-116: Same issue: handleos.UserHomeDir()error.Apply the same fix here to prevent writing to unexpected filesystem locations.
Proposed fix
func SaveOIDCConfig(config *OIDCConfig) error { - homeDir, _ := os.UserHomeDir() + homeDir, err := os.UserHomeDir() + if err != nil { + return fmt.Errorf("failed to get home directory: %w", err) + } configPath := filepath.Join(homeDir, tokenCacheDir, "config.json")cmd/caib/auth/oidc.go-52-53 (1)
52-53: Handle error fromos.UserHomeDir().The error from
os.UserHomeDir()is silently ignored. If this fails (e.g., in containerized environments without a home directory),cachePathwill be constructed with an empty base path, leading to unexpected file locations.Proposed fix
- homeDir, _ := os.UserHomeDir() - cachePath := filepath.Join(homeDir, tokenCacheDir, tokenCacheFile) + homeDir, err := os.UserHomeDir() + if err != nil { + // Fallback to current directory or temp directory + homeDir = os.TempDir() + } + cachePath := filepath.Join(homeDir, tokenCacheDir, tokenCacheFile)internal/buildapi/server.go-1648-1677 (1)
1648-1677: Missing context cancellation check in polling loop.The
findReadyArtifactPodfunction doesn't checkctx.Done()in its polling loop. If the context is cancelled, the function will continue polling until the deadline is reached.Proposed fix
func findReadyArtifactPod(ctx context.Context, k8sClient client.Client, namespace, buildName string, deadline time.Time) (*corev1.Pod, error) { for { + select { + case <-ctx.Done(): + return nil, ctx.Err() + default: + } + podList := &corev1.PodList{} if err := k8sClient.List(ctx, podList,cmd/caib/auth/oidc.go-125-133 (1)
125-133: TOCTOU race condition in port acquisition.There's a time-of-check to time-of-use race between closing the listener (line 131) and starting the server (line 195). Another process could bind to the same port in between. Consider keeping the listener open and passing it to the server.
Regarding the static analysis warning: the
net.Listen("tcp", ":0")is only used to find an available port, and the actual callback server binds tolocalhost(line 133), so this is safe for the OAuth callback use case.Proposed fix to eliminate race condition
// Find available port for callback listener, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { return "", fmt.Errorf("failed to find available port: %w", err) } port := listener.Addr().(*net.TCPAddr).Port - listener.Close() redirectURI := fmt.Sprintf("http://localhost:%d/callback", port) // ... build authURL ... // Create callback server - server := &http.Server{ - Addr: fmt.Sprintf(":%d", port), + server := &http.Server{ Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // ... handler code ... }), } go func() { - if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed { + if err := server.Serve(listener); err != nil && err != http.ErrServerClosed { errChan <- fmt.Errorf("callback server error: %w", err) } }()
🧹 Nitpick comments (21)
vendor/gopkg.in/go-jose/go-jose.v2/.travis.yml (1)
1-45: Consider excluding CI configuration files from vendored dependencies.This is the upstream go-jose library's Travis CI configuration, which serves no purpose in your repository. Vendoring typically should only include the Go source files needed for compilation. Files like
.travis.yml,.gitcookies.sh.enc, and test files add unnecessary bloat.Consider using a
.gitignorepattern or Go module pruning to exclude these files, or verify your vendoring tool's configuration to avoid committing upstream CI artifacts.Example exclusions for vendor cleanup
You can add patterns to exclude unnecessary vendored files:
vendor/**/.travis.yml vendor/**/.github/ vendor/**/*_test.goOr if using
go mod vendor, these files shouldn't typically be included by default. Verify if this file was manually added or if there's a vendoring configuration issue.vendor/github.com/coreos/go-oidc/jose.go (1)
1-1: Add modern//go:builddirective for Go 1.22+ tooling compatibility.Using only the legacy
// +buildtag can trigger warnings in newer toolchains and linters. Add the//go:buildline above it. As per coding guidelines, Go 1.22+ is in use.♻️ Proposed fix
-// +build !golint +//go:build !golint +// +build !golintinternal/buildapi/client/client.go (1)
76-85: Surface CA load/parse failures to aid debugging.Silently ignoring read/parse failures makes misconfigurations hard to diagnose. Consider logging a warning when the CA file is missing or invalid.
✅ Proposed fix
caCert, err := os.ReadFile(caCertPath) if err != nil { - // If file doesn't exist, skip (will use system CAs) + fmt.Fprintf(os.Stderr, "Warning: failed to read CA cert %s: %v\n", caCertPath, err) return } caCertPool := x509.NewCertPool() if !caCertPool.AppendCertsFromPEM(caCert) { - // If parsing fails, skip (will use system CAs) + fmt.Fprintf(os.Stderr, "Warning: failed to parse CA cert %s\n", caCertPath) return }internal/controller/operatorconfig/controller.go (2)
391-409: Unusedownerparameter should be removed.The
ownerparameter inensureBuildAPIInternalJWTSecretis not used anywhere in the method body. Either remove it or use it to set an owner reference on the created secret.♻️ If owner reference is not needed, remove the parameter
-func (r *OperatorConfigReconciler) ensureBuildAPIInternalJWTSecret(ctx context.Context, owner *automotivev1alpha1.OperatorConfig) error { +func (r *OperatorConfigReconciler) ensureBuildAPIInternalJWTSecret(ctx context.Context) error {And update the call site at line 232:
- if err := r.ensureBuildAPIInternalJWTSecret(ctx, owner); err != nil { + if err := r.ensureBuildAPIInternalJWTSecret(ctx); err != nil {
231-241: Remove unusedownerparameter and clarify inconsistent owner reference handling.The
ensureBuildAPIInternalJWTSecretmethod receives anownerparameter but never uses it. Additionally, neitherensureBuildAPIInternalJWTSecretnorensureBuildAPIAuthConfigMapsets owner references on the created resources.This is inconsistent with how other resources (e.g., Tekton tasks and pipelines) are handled—they explicitly call
controllerutil.SetControllerReferencebefore creation, ensuring automatic garbage collection when theOperatorConfigis deleted.Consider either:
- Removing the unused
ownerparameter fromensureBuildAPIInternalJWTSecret- Adding
SetControllerReferenceto both methods if owner references are desired for cleanup- Documenting if manual cleanup via
cleanupBuildAPIis intentionalcmd/caib/main.go (1)
119-129: Emit a warning whenCAIB_INSECURE_TLSdisables verification.Helps prevent accidental insecure usage in CLI runs.
💡 Suggested tweak
- if strings.EqualFold(os.Getenv("CAIB_INSECURE_TLS"), "true") || os.Getenv("CAIB_INSECURE_TLS") == "1" { + if strings.EqualFold(os.Getenv("CAIB_INSECURE_TLS"), "true") || os.Getenv("CAIB_INSECURE_TLS") == "1" { + fmt.Fprintln(os.Stderr, "Warning: CAIB_INSECURE_TLS is set; TLS certificate verification is disabled") opts = append(opts, buildapiclient.WithInsecureTLS()) }internal/controller/operatorconfig/resources.go (1)
21-24: Consider pinning the default operator image instead oflatest.A moving tag can introduce non-reproducible rollouts. A versioned tag or digest (with OPERATOR_IMAGE override preserved) is typically safer.
vendor/github.com/golang-jwt/jwt/v5/map_claims.go (1)
58-61: Ignored error onjson.Number.Float64()conversion.The error from
exp.Float64()is discarded. If the conversion fails,vdefaults to0, which would result in anilreturn (no expiration time). While this is vendored third-party library code and likely intentional (valid JSON numbers from the decoder shouldn't fail), this silent fallback could mask malformed tokens.cmd/caib/auth/config.go (2)
16-21: Set TLS MinVersion to prevent protocol downgrade attacks.The TLS configuration is missing
MinVersion, which could allow negotiation down to older, less secure TLS versions. This is particularly relevant sinceInsecureSkipVerifycan be enabled via environment variable.🔒 Proposed fix
client := &http.Client{ Timeout: 10 * time.Second, Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: insecureTLS}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: insecureTLS, + MinVersion: tls.VersionTLS12, + }, }, }
29-37: Silent fallback on HTTP errors appears intentional but consider logging.Returning
nil, nilon HTTP errors and non-200 status allows graceful fallback to other auth methods, which aligns with the described behavior. However, this could make debugging difficult when OIDC configuration is expected but failing.Consider adding debug logging to help troubleshoot connectivity issues without breaking the fallback behavior.
internal/buildapi/auth_config.go (1)
131-133: Consider logging if scheme registration fails.Ignoring errors from
AddToSchemeis common in Kubernetes code since registration is typically idempotent, but silently swallowing errors could mask unexpected issues during initialization.Optional: Add error logging
scheme := runtime.NewScheme() - _ = apiserver.AddToScheme(scheme) - _ = apiserverv1beta1.AddToScheme(scheme) + if err := apiserver.AddToScheme(scheme); err != nil { + // Log but don't fail - registration errors are typically non-fatal + log.Printf("warning: failed to add apiserver scheme: %v", err) + } + if err := apiserverv1beta1.AddToScheme(scheme); err != nil { + log.Printf("warning: failed to add apiserverv1beta1 scheme: %v", err) + }vendor/github.com/coreos/go-oidc/verify.go (4)
10-10: Deprecatedioutilpackage usage.The
io/ioutilpackage is deprecated since Go 1.16. Per coding guidelines requiring Go 1.22+, this should useio.ReadAllinstead.Suggested fix
- "io/ioutil" + "io"And on line 157:
- body, err := ioutil.ReadAll(resp.Body) + body, err := io.ReadAll(resp.Body)
24-24: Typo in documentation: "publc" should be "public".-// KeySet is a set of publc JSON Web Keys that can be used to validate the signature +// KeySet is a set of public JSON Web Keys that can be used to validate the signature
63-63: Typo in example: "newStatisKeySet" should be "newStaticKeySet".-// keySet := newStatisKeySet(publicKeys...) +// keySet := newStaticKeySet(publicKeys...)
324-324: Typo in comment: "square" appears to be incorrect.The comment mentions "square" but likely should refer to "keyset" or "KeySet.VerifySignature".
- // Ensure that the payload returned by the square actually matches the payload parsed earlier. + // Ensure that the payload returned by the KeySet actually matches the payload parsed earlier.vendor/github.com/golang-jwt/jwt/v5/token.go (1)
39-56: Unusedoptsparameter in constructors.The
opts ...TokenOptionparameter is accepted but not applied inNewandNewWithClaims. The documentation notes this is intentional ("currently unused"), but this could be confusing to API consumers.Consider either applying the options or documenting why they exist for future use:
func NewWithClaims(method SigningMethod, claims Claims, opts ...TokenOption) *Token { t := &Token{ Header: map[string]interface{}{ "typ": "JWT", "alg": method.Alg(), }, Claims: claims, Method: method, } for _, opt := range opts { opt(t) } return t }vendor/github.com/openshift/api/config/v1/types_scheduling.go (1)
91-94: Typo in comment: "LowNodeUtililization" should be "LowNodeUtilization".The comment has an extra 'l' in "Utililization".
- // LowNodeUtililization is the default, and defines a scheduling profile which prefers to + // LowNodeUtilization is the default, and defines a scheduling profile which prefers tovendor/github.com/openshift/api/config/v1/types_dns.go (2)
127-130: Consider addingomitemptyto AWS field json tag.The
AWSfield is marked as+optionalbut lacksomitemptyin the json tag. This could result in"aws": nullappearing in serialized JSON when the platform is not AWS.// aws contains DNS configuration specific to the Amazon Web Services cloud provider. // +optional - AWS *AWSDNSSpec `json:"aws"` + AWS *AWSDNSSpec `json:"aws,omitempty"`
93-95: Incomplete comment inDNSStatus.The comment on line 94 appears to be a placeholder or incomplete note:
// dnsSuffix (service-ca amongst others). Consider either completing the documentation or removing it.cmd/caib/auth/oidc.go (1)
152-152: UnusedstateChanchannel.The
stateChanis created (line 152) and written to (line 189) but never read from. The state validation already happens synchronously in the handler at line 181. This channel can be removed.Proposed cleanup
codeChan := make(chan string, 1) - stateChan := make(chan string, 1) errChan := make(chan error, 1) // ... in handler ... codeChan <- code - stateChan <- returnedState w.WriteHeader(http.StatusOK)Also applies to: 189-189
internal/buildapi/server.go (1)
1688-1837: Consider extracting the inline response struct definition.The
OIDCConfigResponsestruct is defined inline with deeply nested anonymous structs. While functional, extracting this to a named type would improve readability and maintainability.Proposed refactor
// OIDCConfigResponse represents the OIDC configuration exposed to clients type OIDCConfigResponse struct { ClientID string `json:"clientId,omitempty"` JWT []OIDCJWTConfig `json:"jwt"` } type OIDCJWTConfig struct { Issuer OIDCIssuerConfig `json:"issuer"` ClaimMappings OIDCClaimMappingsConfig `json:"claimMappings"` } type OIDCIssuerConfig struct { URL string `json:"url"` Audiences []string `json:"audiences,omitempty"` } type OIDCClaimMappingsConfig struct { Username OIDCUsernameMapping `json:"username"` } type OIDCUsernameMapping struct { Claim string `json:"claim"` Prefix string `json:"prefix,omitempty"` }
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
bkhizgiy
changed the title
bkhizgiy
force-pushed
the
oidc
branch
3 times, most recently
from
bd819f3 to
08de563
Compare
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
…onection Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Fix all issues with AI agents
In `@cmd/caib/auth/config_test.go`:
- Around line 75-88: The test description is misleading: Update the It()
description in the test using GetOIDCConfigFromAPI to reflect that an empty
"jwt" array returns (nil, nil) rather than an error; change the text (currently
"should return error when API returns empty JWT array") to something like
"should return nil config and no error when API returns empty JWT array" so it
matches the assertions and the behavior implemented in GetOIDCConfigFromAPI in
config.go.
In `@cmd/caib/auth/config.go`:
- Around line 78-82: The code constructs an OIDCConfig using
jwtConfig.Issuer.URL without validating it; add a check before returning to
ensure jwtConfig.Issuer.URL is non-empty (trim whitespace) and return a
descriptive error instead of creating the OIDCConfig when it's empty or invalid;
reference the OIDCConfig construction and jwtConfig.Issuer.URL (and IssuerURL
field) to locate where to insert the validation and error return.
In `@cmd/caib/auth/oidc.go`:
- Around line 149-159: The callback listener currently calls net.Listen("tcp",
":0") which binds to all interfaces; change it to bind only to localhost by
using net.Listen("tcp", "127.0.0.1:0") (or "localhost:0") when creating listener
so the temporary port (port variable) and redirectURI remain local, keep the
existing listener.Close() handling and error checks around listener, and ensure
redirectURI := fmt.Sprintf("http://localhost:%d/callback", port) continues to
use the resolved port.
In `@cmd/caib/auth/wrapper.go`:
- Around line 29-39: The code opens an if err != nil after calling
GetOIDCConfigFromLocalConfig and then reassigns config, err by calling
GetOIDCConfigFromAPI but never closes the first if block; close the first if
block (the fallback-to-API branch) before starting the next if err != nil check
so the error handling for GetOIDCConfigFromAPI is nested/structured correctly;
specifically, ensure the block that starts after "if err != nil { // Fallback to
Build API" is properly terminated before the subsequent "if err != nil {" and
that final nil-check "if config == nil {" remains at the top level following
both fallbacks, preserving variables config and err from GetOIDCConfigFromAPI.
In `@internal/buildapi/server.go`:
- Around line 1726-1785: refreshAuthConfigIfNeeded currently holds authConfigMu
for the entire function causing network ops to block auth; change it to take a
read lock to check time/lastAuthConfigCheck and return if not needed, then
release the lock and perform network calls (getKubernetesClient,
loadAuthenticationConfigurationFromOperatorConfig) without holding authConfigMu,
and finally re-acquire authConfigMu as a write lock to update fields
(a.authConfig, a.externalJWT, a.internalPrefix, a.oidcClientID and
a.lastAuthConfigCheck) using the results; ensure you still set
a.lastAuthConfigCheck under the write lock to avoid races and refer to the
existing symbols refreshAuthConfigIfNeeded, authConfigMu, lastAuthConfigCheck,
getKubernetesClient, loadAuthenticationConfigurationFromOperatorConfig,
authConfig, externalJWT, internalPrefix, oidcClientID when making the edits.
In `@test/e2e/e2e_test.go`:
- Around line 337-359: The OIDC suite's BeforeAll/AfterAll creates and deletes
the shared namespace (variable namespace) which duplicates the controller suite
lifecycle and can cause flakes; either remove the namespace creation/deletion
from the Describe("OIDC Authentication", Ordered...) block (BeforeAll/AfterAll)
so the global BeforeSuite/AfterSuite manages it, or change this suite to use a
dedicated namespace name (e.g., oidcNamespace) and update all references
accordingly; modify the BeforeAll to create the dedicated namespace (or delete
the creation if relying on shared setup) and the AfterAll to delete only that
dedicated namespace (or remove deletion if using shared teardown) and ensure the
utils.Run calls and Expect checks remain.
In `@vendor/github.com/coreos/go-oidc/code-of-conduct.md`:
- Around line 15-23: The markdown lists in code-of-conduct.md trigger
markdownlint MD007/MD005 due to inconsistent indentation/marker usage; fix by
normalizing all list items to a consistent style (use a single space after each
bullet marker, consistent nesting with either 2 or 4 spaces per indent level and
no tabs) or alternatively update the linting config to ignore vendor/ files;
locate and normalize the unordered list lines (the bullet lines under "Examples
of unacceptable behavior by participants include:") or add vendor/ exclusion to
your markdownlint rule set.
In `@vendor/github.com/golang-jwt/jwt/v5/MIGRATION_GUIDE.md`:
- Around line 24-195: Fix the markdownlint issues reported across the
MIGRATION_GUIDE.md: normalize unordered list indentation to 0-space (MD007) for
the top bullet lists, add missing blank lines around lists (MD032), replace hard
tabs with spaces inside all Go code blocks (MD010), convert the five specified
fenced code blocks to indented code block style (MD046) or consistently use
fenced blocks with language tags as appropriate, shorten or wrap long lines to
<=80 chars (MD013) in the paragraphs around the validator/claims examples,
remove or demote extra H1 headings so only one top-level title remains (MD025),
add a language identifier to the final shell code block (MD040) and convert the
bare URL to an angle-bracketed link (MD034); target edits around the documented
sections for claims/validator examples, the code blocks showing Go examples, the
final "go get" shell block, and the older releases URL line.
In `@vendor/github.com/golang-jwt/jwt/v5/README.md`:
- Around line 143-147: The extensions table contains bare URLs in the Repo
column (rows for GCP, AWS, JWKS); wrap each bare URL in markdown link syntax so
they become clickable links (e.g., [Repo Name](https://...) or reuse the
existing URL as the link text) to satisfy markdownlint MD034 and maintain
consistent formatting—update the three entries referencing
https://github.com/someone1/gcp-jwt-go,
https://github.com/matelang/jwt-go-aws-kms, and
https://github.com/MicahParks/keyfunc accordingly.
- Around line 38-44: The "Supported Go versions" Markdown header is currently
H3; change it to H2 by replacing "### Supported Go versions" with "## Supported
Go versions" so it aligns with the surrounding top-level sections and removes
the MD001 markdownlint warning; locate the header by its exact text "Supported
Go versions" in the README.md and update the header level accordingly.
In `@vendor/github.com/golang-jwt/jwt/v5/rsa_pss.go`:
- Around line 12-18: The vendored file contains doc typos in the
SigningMethodRSAPSS type (duplicated phrase and "VerifyPPS" typo) inside the
vendor directory; do not edit it directly — instead open an upstream issue or PR
against golang-jwt/jwt to correct the comments in SigningMethodRSAPSS (and its
fields Options / VerifyOptions), or update to a release that already fixes it
and then regenerate vendored code by updating the dependency (e.g., bump the
module version in go.mod via go get to the fixed release) and run go mod vendor
to refresh vendor/, ensuring the corrected comments land without manual edits to
vendor.
In `@vendor/github.com/golang-jwt/jwt/v5/VERSION_HISTORY.md`:
- Around line 98-100: Fix the spelling typo in the VERSION_HISTORY.md entry:
replace the misspelled word "compatibilty" with "compatibility" in the paragraph
that mentions "Backwards compatibilty has been maintained for passing `[]byte`
to the RSA signing methods"; ensure the corrected word appears where the
sentence references backwards compatibility for RSA signing methods and any
other occurrences of "compatibilty" in the same file are updated as well.
In `@vendor/github.com/openshift/api/config/v1/types_cluster_image_policy.go`:
- Around line 35-36: Fix the exported doc comment typos so they start with the
exact type name: change the comment for ClusterImagePolicySpec (currently
"CLusterImagePolicySpec...") to begin with "ClusterImagePolicySpec" and correct
the misspelling "Polices" to "Policies" in the doc comment referring to the
ClusterImagePolicy type; ensure both comments exactly begin with their
respective type names to satisfy Go doc comment conventions.
In `@vendor/github.com/openshift/api/config/v1/types_dns.go`:
- Around line 26-31: The Status field is marked +optional but its JSON tag lacks
omitempty, causing empty DNSStatus values to be serialized; update the struct
field declaration for Status (type DNSStatus, field name Status) to include
`omitempty` in its json tag so optional empty status objects are omitted (mirror
the pattern used for Platform and other optional fields).
- Around line 61-67: The Platform field in the DNS struct is declared as a
non-pointer DNSPlatformSpec with `json:"platform,omitempty"`, so omitempty is
ineffective and cannot represent absence; change Platform's type to a pointer
(*DNSPlatformSpec) so nil means omitted, update any constructors,
unmarshalling/usage sites that assume a value (references to Platform) to handle
nil safely, and adjust any related validation or defaulting logic that expects a
non-nil DNSPlatformSpec (search for usages of Platform and DNSPlatformSpec to
update).
In `@vendor/github.com/openshift/api/config/v1/types_ingress.go`:
- Around line 97-100: The LoadBalancer field is a non-pointer struct so
`omitempty` won't omit it and will serialize as `{}` when unset; change the
field declaration from `LoadBalancer LoadBalancer` to a pointer `LoadBalancer
*LoadBalancer` so it can be nil and omitted by `json:"loadBalancer,omitempty"`,
and update any code that constructs or reads this field (e.g., places that
create or dereference `LoadBalancer`, validations, deep-copy or defaulting
logic) to handle the nil pointer appropriately.
In `@vendor/github.com/pquerna/cachecontrol/README.md`:
- Around line 101-103: Update the README.md line containing the contributor
acknowledgement by correcting the misspelled word "recieved" to "received"
(i.e., change the phrase "`cachecontrol` has recieved significant contributions
from:" to "`cachecontrol` has received significant contributions from:"). Ensure
only the spelling is corrected and surrounding formatting/links (e.g., "[Paul
Querna](https://github.com/pquerna)") remain unchanged.
- Around line 48-82: The example's main function uses time.Now().UTC() but the
imports block doesn't include the time package; update the import block in the
example (the import list at the top of the snippet used by main) to add "time"
so the call to time.Now().UTC() in Object.NowUTC compiles; locate the import
block that currently imports "fmt", "io/ioutil", and "net/http" and add the
"time" entry.
In `@vendor/gopkg.in/go-jose/go-jose.v2/CHANGELOG.md`:
- Around line 74-79: The CHANGELOG entry contains a bare URL that violates
MD034; update the line containing
"https://i.blackhat.com/BH-US-23/Presentations/US-23-Tervoort-Three-New-Attacks-Against-JSON-Web-Tokens.pdf"
by wrapping it as a proper Markdown link (e.g., [BlackHat
presentation](https://i.blackhat.com/...)) or enclosing it in angle brackets
<https://i.blackhat.com/...> so the URL is not bare, leaving the rest of the
paragraph unchanged.
- Around line 5-36: Several unordered list items in the CHANGELOG.md start with
a leading space before the hyphen (e.g., " - An attacker...", " - Revert...", "
- Minimum supported Go version..."), which violates markdownlint rules
MD007/MD005; remove the extra leading space so list markers start at column 0
(i.e., change " - ..." to "- ...") and ensure consistent indentation for nested
lists throughout the file to satisfy markdownlint.
🧹 Nitpick comments (15)
cmd/caib/auth/wrapper_test.go (1)
34-42: Consider adding tests for the re-authentication flow.The current tests verify client creation succeeds, but don't exercise the actual re-auth behavior when an authentication error is encountered. Consider adding a test that:
- Creates a client with an invalid/expired token
- Makes a request that returns a 401
- Verifies the re-auth flow is triggered
This would provide confidence that the "reauth" part of
CreateClientWithReauthworks as intended.internal/buildapi/client/client.go (1)
74-99: UseSystemCertPool()and append the custom CA to preserve system roots.Creating a new empty pool loses the system's trusted CA certificates. Instead, load the system pool and append the custom CA to it, falling back to an empty pool only if the system pool is unavailable.
♻️ Suggested change
- caCertPool := x509.NewCertPool() + caCertPool, err := x509.SystemCertPool() + if err != nil || caCertPool == nil { + caCertPool = x509.NewCertPool() + } if !caCertPool.AppendCertsFromPEM(caCert) { // If parsing fails, skip (will use system CAs) return }vendor/github.com/pquerna/cachecontrol/README.md (1)
23-33: Preferio.ReadAllover deprecatedioutil.ReadAll.
Go 1.22+ deprecatesioutil. Consider updating both examples.♻️ Example update (apply similarly in both snippets)
- "io/ioutil" + "io" ... - _, _ = ioutil.ReadAll(res.Body) + _, _ = io.ReadAll(res.Body)Also applies to: 51-61
vendor/github.com/coreos/go-oidc/oidc.go (1)
13-13: Replace deprecatedioutil.ReadAllwithio.ReadAll(Go 1.20+).This keeps the code idiomatic for Go 1.22+ and avoids deprecated APIs.
♻️ Proposed update
@@ -import ( +import ( "context" "crypto/sha256" "crypto/sha512" "encoding/base64" "encoding/json" "errors" "fmt" "hash" - "io/ioutil" + "io" "mime" "net/http" "strconv" "strings" "time" @@ - body, err := ioutil.ReadAll(resp.Body) + body, err := io.ReadAll(resp.Body) @@ - body, err := ioutil.ReadAll(resp.Body) + body, err := io.ReadAll(resp.Body)Also applies to: 126-128, 236-238
vendor/github.com/coreos/go-oidc/.travis.yml (1)
3-5: Update CI Go versions to align with repo standard (Go 1.22+).If this CI file is actually used, the Go 1.14/1.15 matrix is far below the project’s Go version target. If it’s unused in this repo, consider dropping it to avoid confusion.
♻️ Example update (adjust to actual repo Go version)
go: - - "1.14" - - "1.15" + - "1.22"internal/controller/operatorconfig/resources_test.go (1)
31-37: Make the placeholder test explicitly pending/skip.A hardcoded
trueassertion adds no signal and can mask missing coverage. Consider marking it pending until real assertions are added.📝 Suggested change
It("placeholder test", func() { - Expect(true).To(BeTrue()) + Skip("TODO: add resource generation assertions") })internal/controller/operatorconfig/resources.go (1)
369-423: Use full-entropy bytes for the HS256 signing key.Mapping random bytes into a 64‑char charset reduces entropy. Encoding raw random bytes preserves the full 256‑bit strength expected for HS256 keys.
♻️ Suggested change
-func generateRandomToken(length int) (string, error) { - const charset = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_" - bytes := make([]byte, length) - if _, err := rand.Read(bytes); err != nil { - return "", err - } - for i := range bytes { - bytes[i] = charset[int(bytes[i])%len(charset)] - } - return string(bytes), nil -} +func generateRandomToken(lengthBytes int) (string, error) { + b := make([]byte, lengthBytes) + if _, err := rand.Read(b); err != nil { + return "", err + } + return base64.RawURLEncoding.EncodeToString(b), nil +}test/e2e/e2e_test.go (1)
372-379: Add timeouts and explicit TLS minimum version to HTTP clients.Without a timeout, these tests can hang indefinitely, and an explicit MinVersion avoids legacy TLS defaults.
♻️ Suggested change
- client := &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, - }, - } + client := &http.Client{ + Timeout: 15 * time.Second, + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS13, + }, + }, + }- client := &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, - }, - } + client := &http.Client{ + Timeout: 15 * time.Second, + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS13, + }, + }, + }Also applies to: 427-432
cmd/caib/auth/config_test.go (1)
145-150: Minor: HOME environment variable restoration could be more robust.If
HOMEwas originally unset (empty), this logic won't restore it to unset after the test. Consider usingos.UnsetenvwhenoriginalHomeis empty.Suggested improvement
AfterEach(func() { - if originalHome != "" { - _ = os.Setenv("HOME", originalHome) + if originalHome == "" { + _ = os.Unsetenv("HOME") + } else { + _ = os.Setenv("HOME", originalHome) } _ = os.RemoveAll(tempDir) })cmd/caib/auth/wrapper.go (1)
12-21: Consider case-insensitive matching for auth error detection.The current string matching is case-sensitive, so
"Unauthorized"won't match"unauthorized". This could miss some error messages.Suggested improvement
func IsAuthError(err error) bool { if err == nil { return false } - errStr := err.Error() + errStr := strings.ToLower(err.Error()) return strings.Contains(errStr, "401") || strings.Contains(errStr, "403") || strings.Contains(errStr, "unauthorized") || strings.Contains(errStr, "forbidden") }internal/buildapi/auth_config.go (2)
39-41: Silently ignoringAddToSchemeerrors may hide initialization issues.While these registrations are unlikely to fail in practice, discarding the errors makes debugging harder if they do. Consider at least logging at debug level.
🔧 Proposed fix
scheme := runtime.NewScheme() - _ = apiserver.AddToScheme(scheme) - _ = apiserverv1beta1.AddToScheme(scheme) + if err := apiserver.AddToScheme(scheme); err != nil { + return nil, fmt.Errorf("failed to add apiserver scheme: %w", err) + } + if err := apiserverv1beta1.AddToScheme(scheme); err != nil { + return nil, fmt.Errorf("failed to add apiserverv1beta1 scheme: %w", err) + }
49-64: Consider adding a clarifying comment for the CA field mutation.Line 54 clears
CertificateAuthorityafter loading it into theCAContentProvider. This is correct (the CA is now handled byoidcCAContent), but the intent could be clearer.📝 Suggested comment
if _, err := os.Stat(jwtAuthenticator.Issuer.CertificateAuthority); err == nil { oidcCAContent, oidcCAError = dynamiccertificates.NewDynamicCAContentFromFile( "oidc-authenticator", jwtAuthenticator.Issuer.CertificateAuthority, ) + // Clear inline CA since it's now handled by the dynamic CA provider jwtAuthenticator.Issuer.CertificateAuthority = ""cmd/caib/auth/oidc.go (3)
52-75: Constructor silently returns nil on errors, making debugging difficult.When
issuerURLorclientIDis empty, or whenos.UserHomeDir()fails, the function returnsnilwithout any error indication. Callers need to check for nil and have no way to know why initialization failed.Consider returning an error to improve debuggability:
♻️ Proposed signature change
-func NewOIDCAuth(issuerURL, clientID string, scopes []string) *OIDCAuth { +func NewOIDCAuth(issuerURL, clientID string, scopes []string) (*OIDCAuth, error) { if issuerURL == "" || clientID == "" { - return nil + return nil, fmt.Errorf("issuerURL and clientID are required") } // ... homeDir, err := os.UserHomeDir() if err != nil { - return nil + return nil, fmt.Errorf("failed to get user home directory: %w", err) }
106-124: Token validation only checks expiration, not signature - verify this is intentional.
IsTokenValidusesParseUnverifiedand only checks theexpclaim. This means the cached token's signature is never validated locally. While the server will validate the signature, a corrupted or tampered cached token will result in a failed API call rather than triggering re-authentication.If this is intentional (to avoid needing JWKS locally), consider adding a comment to clarify the design decision.
427-438: Windowsstartcommand may fail with URLs containing special characters.On Windows, URLs with special characters (like
&in query parameters) can cause issues withcmd /c start. Consider wrapping the URL in quotes or using a different approach.♻️ Proposed fix for Windows
switch runtime.GOOS { case "windows": cmd = "cmd" - args = []string{"/c", "start"} + args = []string{"/c", "start", ""} case "darwin":Adding an empty string as the window title before the URL helps with special characters in URLs on Windows.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/buildapi/client_tokens.go`:
- Around line 98-117: The signClientToken method dereferences a.internalJWT
(accessing a.internalJWT.issuer, .audience, .key) without a nil check; add an
explicit nil guard at the start of func signClientToken(username string) to
return a descriptive error if a.internalJWT == nil (e.g., "internal JWT not
configured") and avoid proceeding, and then continue using
a.internalJWT.issuer/audience/key only after that check so you never risk a nil
pointer dereference.
🧹 Nitpick comments (5)
internal/controller/operatorconfig/resources.go (1)
369-411: Consider token rotation strategy for the 1-year expiry.The JWT token has a 365-day expiry (line 377). While this may be intentional for an internal service token, long-lived tokens present operational challenges:
- If the signing key is compromised, it remains valid for up to a year.
- No automatic rotation mechanism is in place.
For internal service-to-service authentication, this might be acceptable. However, consider documenting this decision and potentially adding a reconciliation check that regenerates the secret when it approaches expiry.
The clock skew handling (lines 383-384) setting
IssuedAtandNotBeforeto 1 minute in the past is good practice.cmd/caib/main.go (1)
151-218: Consider simplifying retry logic to reduce complexity.The
executeWithReauthfunction has multiple kubeconfig fallback paths (lines 179-189 and 207-214) which could be consolidated. While the current implementation is defensive, it adds complexity.However, given this is WIP and the logic handles edge cases like server OIDC being broken, the current approach is acceptable.
cmd/caib/auth/wrapper.go (1)
11-21: Consider using typed errors or status codes for auth error detection.
IsAuthErrorrelies on string matching which is fragile - it may match unrelated errors containing "401" or "unauthorized" in different contexts. Consider defining typed errors or extracting HTTP status codes where available.♻️ Alternative approach using typed errors
// Define a typed error in the client package type HTTPError struct { StatusCode int Message string } func (e *HTTPError) Error() string { return fmt.Sprintf("HTTP %d: %s", e.StatusCode, e.Message) } // Then check type rather than string content func IsAuthError(err error) bool { var httpErr *HTTPError if errors.As(err, &httpErr) { return httpErr.StatusCode == 401 || httpErr.StatusCode == 403 } // Fallback to string matching for backwards compatibility errStr := err.Error() return strings.Contains(errStr, "401") || strings.Contains(errStr, "403") }internal/buildapi/server.go (1)
1887-1986: Consider extracting inline struct definitions for readability.The
handleGetAuthConfigfunction has deeply nested inline struct definitions (lines 1892-1906, 1947-1981). While functional, extracting these to named types would improve readability and maintainability.♻️ Example extraction
type OIDCJWTConfigResponse struct { Issuer struct { URL string `json:"url"` Audiences []string `json:"audiences,omitempty"` } `json:"issuer"` ClaimMappings struct { Username struct { Claim string `json:"claim"` Prefix string `json:"prefix,omitempty"` } `json:"username"` } `json:"claimMappings"` } type OIDCConfigResponse struct { ClientID string `json:"clientId,omitempty"` JWT []OIDCJWTConfigResponse `json:"jwt"` }cmd/caib/auth/oidc.go (1)
233-265: Consider consolidating server shutdown logic.The server shutdown code is duplicated in all four exit paths (lines 235-237, 251-253, 256-258, 261-263). This could be simplified using a defer pattern.
♻️ Proposed consolidation
+ // Ensure server is shut down in all cases + defer func() { + shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + _ = server.Shutdown(shutdownCtx) + cancel() + }() + // Wait for callback select { case code := <-codeChan: - shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - _ = server.Shutdown(shutdownCtx) - cancel() // Exchange code for token token, err := a.exchangeCodeForToken(ctx, discovery.TokenEndpoint, code, redirectURI, codeVerifier) // ... rest of success path case err := <-errChan: - shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - _ = server.Shutdown(shutdownCtx) - cancel() return "", err // ... other cases without explicit shutdown
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/operatorconfig/resources.go (1)
344-367:⚠️ Potential issue | 🟡 MinorSecurity: Fallback to static secret on random generation failure.
If
rand.Readfails (Line 347), the code falls back to a static 32-byte string. While this is extremely unlikely in practice, using a predictable cookie secret would compromise session security.Consider failing loudly instead of silently degrading security:
🛡️ Proposed fix
func (r *OperatorConfigReconciler) buildOAuthSecret(name string) *corev1.Secret { // Generate a random 32-byte cookie secret for AES-256 cookieSecret := make([]byte, 32) if _, err := rand.Read(cookieSecret); err != nil { - // Fallback to a static secret if random generation fails - // This should never happen in practice - cookieSecret = []byte("fallback-secret-change-me-32bit") + // This should never happen in practice, but if it does, panic rather than + // use a predictable secret that would compromise security + panic(fmt.Sprintf("crypto/rand.Read failed: %v", err)) }
🤖 Fix all issues with AI agents
In `@cmd/caib/main.go`:
- Around line 203-215: The kubeconfig fallback after an OIDC re-auth failure can
silently change the identity; in the block handling IsAuthError(err) where
loadTokenFromKubeconfig() is used, remove the automatic kubeconfig fallback and
instead return the original auth error (or at minimum log a clear warning before
returning). Update the code around IsAuthError(err), loadTokenFromKubeconfig(),
createBuildAPIClient(...) and the call to fn(client) so that if OIDC re-auth
produced a token but the server still returns 401, you do not proceed to
loadTokenFromKubeconfig() and re-authenticate as another identity—return err (or
log.Warnf with context about the 401 and the OIDC token) and stop execution.
In `@test/e2e/e2e_test.go`:
- Around line 488-500: The test under Context("Internal JWT Validation")
currently contains It("should reject tokens with empty subject") but only checks
the Build API pod is Running; update the test so the name matches behavior OR
implement the actual token-rejection assertion: either rename the It description
to something like "should have Build API pod running" to reflect the pod status
check, or add steps to generate/send an internal JWT with an empty subject to
the Build API (e.g., create a JWT, call the Build API endpoint, and Expect a
4xx/unauthorized response) within the same It block; locate the test by
Context("Internal JWT Validation") and It("should reject tokens with empty
subject") to apply the change.
- Around line 452-453: Replace the brittle time.Sleep(10 * time.Second) in the
test (the By("waiting for operator to reconcile and Build API to reload
configuration") block) with a Gomega Eventually poll that asserts the operator
has reconciled and the Build API config/state reflects the expected changes;
implement a small helper or inline function that queries the Build resource or
relevant operator status (the same API used elsewhere in the file) and returns
the condition/value you expect, then call Eventually(<checkFunc>,
30*time.Second, 1*time.Second).Should(<expected matcher>) to wait reliably
instead of sleeping.
🧹 Nitpick comments (9)
internal/controller/operatorconfig/resources.go (1)
369-411: Consider documenting the 1-year token expiry rationale.The internal JWT is created with a 365-day expiry (Line 377). While this may be intentional for internal service-to-service authentication where the secret can be rotated by deleting the Secret resource, it's worth documenting this design decision. If the signing key is compromised, all tokens remain valid for up to a year.
For enhanced security posture, consider either:
- Adding a comment explaining why 1-year is appropriate for this use case
- Making the expiry configurable via OperatorConfig
test/e2e/e2e_test.go (1)
410-414: Set TLS MinVersion in test HTTP clients.The static analyzer flags missing
MinVersionin TLS configs. WhileInsecureSkipVerify: trueis acceptable for testing against self-signed certs, explicitly settingMinVersionis a good practice even in tests.🔒 Proposed fix
client := &http.Client{ Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, }, }Also applies to: 464-468
cmd/caib/auth/oidc.go (1)
282-288: Consider reusing http.Transport or using http.DefaultTransport.Creating a new
http.Transport{}for each request loses connection pooling benefits. For the discovery and token exchange calls, consider usinghttp.DefaultTransportor a shared transport instance.♻️ Proposed fix
- transport := &http.Transport{} - // Use default TLS settings - client := &http.Client{ Timeout: 30 * time.Second, - Transport: transport, + // Use default transport for connection pooling and standard TLS settings }cmd/caib/auth/config.go (1)
16-21: Consider reusing http.Transport.Similar to the feedback on
oidc.go, creating a new emptyhttp.Transport{}for each API call loses connection pooling benefits. The default transport would work fine here.♻️ Proposed fix
- transport := &http.Transport{} - client := &http.Client{ Timeout: 30 * time.Second, - Transport: transport, }internal/buildapi/auth_config.go (1)
39-42: Consider logging errors fromAddToScheme.The errors from
AddToSchemeare silently ignored. While these are unlikely to fail in practice, logging them at debug level would help diagnose issues in edge cases.📝 Proposed fix
scheme := runtime.NewScheme() - _ = apiserver.AddToScheme(scheme) - _ = apiserverv1beta1.AddToScheme(scheme) + if err := apiserver.AddToScheme(scheme); err != nil { + return nil, fmt.Errorf("failed to add apiserver to scheme: %w", err) + } + if err := apiserverv1beta1.AddToScheme(scheme); err != nil { + return nil, fmt.Errorf("failed to add apiserverv1beta1 to scheme: %w", err) + }internal/buildapi/server.go (4)
129-131: Consider adding a timeout for initial auth config load.The initial call to
loadAuthenticationConfigurationFromOperatorConfigon line 131 usescontext.Background()without a timeout. If the Kubernetes API is slow or the OIDC provider is unreachable during startup, this could delay server readiness. Consider using a context with timeout similar to the refresh logic (line 1745).🔧 Proposed fix
logger.Info("attempting to load authentication config from OperatorConfig", "namespace", namespace) k8sClient, err := a.getCatalogClient() if err == nil { - cfg, authn, prefix, err := loadAuthenticationConfigurationFromOperatorConfig(context.Background(), k8sClient, namespace) + initCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + cfg, authn, prefix, err := loadAuthenticationConfigurationFromOperatorConfig(initCtx, k8sClient, namespace) + cancel()
1679-1700: Consider consolidating duplicate client creation logic.
getKubernetesClient()duplicates much of the logic ingetCatalogClient()(lines 325-350). Both create REST config identically. Consider extracting the common REST config creation into a shared helper and/or parameterizing the scheme setup.
1787-1790: Consider moving periodic refresh to a background goroutine.
refreshAuthConfigIfNeeded()is called on every authenticated request (line 1789). While it has a 60-second check, acquiring the mutex on every request adds overhead. Consider running the refresh in a background goroutine started byStart()instead.
1892-1906: Consider extracting the response struct to a named type.The inline struct definition is verbose and the same structure is repeated in the append operation (lines 1947-1980). Extracting this to a named type would improve readability and make the code easier to maintain.
♻️ Proposed refactor
// Define at package level or near handleGetAuthConfig type OIDCConfigResponse struct { ClientID string `json:"clientId,omitempty"` JWT []OIDCJWTConfig `json:"jwt"` } type OIDCJWTConfig struct { Issuer OIDCIssuer `json:"issuer"` ClaimMappings OIDCClaimMappings `json:"claimMappings"` } type OIDCIssuer struct { URL string `json:"url"` Audiences []string `json:"audiences,omitempty"` } type OIDCClaimMappings struct { Username OIDCUsernameClaim `json:"username"` } type OIDCUsernameClaim struct { Claim string `json:"claim"` Prefix string `json:"prefix,omitempty"` }
| // Still 401 after OIDC re-auth (e.g. server OIDC broken, or wrong client/audience) - try kubeconfig fallback | ||
| if !auth.IsAuthError(err) { | ||
| return err | ||
| } | ||
| if tok, kerr := loadTokenFromKubeconfig(); kerr == nil && strings.TrimSpace(tok) != "" { | ||
| *authToken = tok | ||
| client, err = createBuildAPIClient(serverURL, authToken) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| fmt.Println("Attempting kubeconfig fallback...") | ||
| return fn(client) | ||
| } |
There was a problem hiding this comment.
Consider whether kubeconfig fallback after OIDC re-auth is appropriate.
After OIDC re-authentication succeeds (Line 178 sets new token), if the retry still returns 401 (Lines 203-206), the code falls back to kubeconfig (Lines 207-214). This could authenticate with a different identity than intended.
If OIDC was configured and re-auth succeeded but the server still rejects the token (e.g., wrong audience/issuer configuration), falling back to kubeconfig might bypass the intended authentication mechanism.
Consider returning the error instead of falling back, or at minimum adding a warning:
🔧 Proposed fix
// Still 401 after OIDC re-auth (e.g. server OIDC broken, or wrong client/audience) - try kubeconfig fallback
if !auth.IsAuthError(err) {
return err
}
+ // OIDC was configured and re-auth succeeded, but server still rejected the token
+ // This likely indicates misconfiguration rather than an expired token
+ fmt.Println("Warning: OIDC token was rejected after re-authentication. This may indicate OIDC misconfiguration.")
if tok, kerr := loadTokenFromKubeconfig(); kerr == nil && strings.TrimSpace(tok) != "" {
*authToken = tok📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Still 401 after OIDC re-auth (e.g. server OIDC broken, or wrong client/audience) - try kubeconfig fallback | |
| if !auth.IsAuthError(err) { | |
| return err | |
| } | |
| if tok, kerr := loadTokenFromKubeconfig(); kerr == nil && strings.TrimSpace(tok) != "" { | |
| *authToken = tok | |
| client, err = createBuildAPIClient(serverURL, authToken) | |
| if err != nil { | |
| return err | |
| } | |
| fmt.Println("Attempting kubeconfig fallback...") | |
| return fn(client) | |
| } | |
| // Still 401 after OIDC re-auth (e.g. server OIDC broken, or wrong client/audience) - try kubeconfig fallback | |
| if !auth.IsAuthError(err) { | |
| return err | |
| } | |
| // OIDC was configured and re-auth succeeded, but server still rejected the token | |
| // This likely indicates misconfiguration rather than an expired token | |
| fmt.Println("Warning: OIDC token was rejected after re-authentication. This may indicate OIDC misconfiguration.") | |
| if tok, kerr := loadTokenFromKubeconfig(); kerr == nil && strings.TrimSpace(tok) != "" { | |
| *authToken = tok | |
| client, err = createBuildAPIClient(serverURL, authToken) | |
| if err != nil { | |
| return err | |
| } | |
| fmt.Println("Attempting kubeconfig fallback...") | |
| return fn(client) | |
| } |
🤖 Prompt for AI Agents
In `@cmd/caib/main.go` around lines 203 - 215, The kubeconfig fallback after an
OIDC re-auth failure can silently change the identity; in the block handling
IsAuthError(err) where loadTokenFromKubeconfig() is used, remove the automatic
kubeconfig fallback and instead return the original auth error (or at minimum
log a clear warning before returning). Update the code around IsAuthError(err),
loadTokenFromKubeconfig(), createBuildAPIClient(...) and the call to fn(client)
so that if OIDC re-auth produced a token but the server still returns 401, you
do not proceed to loadTokenFromKubeconfig() and re-authenticate as another
identity—return err (or log.Warnf with context about the 401 and the OIDC token)
and stop execution.
There was a problem hiding this comment.
why shouldn't we have a fallback?
There was a problem hiding this comment.
@bkhizgiy Good question. A fallback is helpful for UX, but the specific case that’s risky is “OIDC re-auth succeeded and we still get 401.” In that situation, silently switching to kubeconfig can:
- Change the caller’s identity without intent: many kubeconfigs are highly-privileged (e.g., cluster-admin). The command may “succeed,” but under a different principal than the user just authenticated with OIDC.
- Mask real misconfigurations: audience/issuer/clientID mistakes get hidden because we continue with a different auth path. That makes issues harder to diagnose and can violate org policy that mandates IdP-backed logins.
- Complicate audit and forensics: actions get recorded under a different subject than the one the user just logged in as.
When OIDC isn’t configured at all, falling back to kubeconfig is reasonable (we already do that). The contentious fallback is only after we obtained an OIDC token and the server still rejects it.
Balanced approach:
- Default: if OIDC re-auth returned a token but the retry is 401, surface the error with a clear message (likely misconfiguration), and do not auto-fallback.
- Opt-in: allow fallback via an explicit signal (e.g., env var or flag) for environments that want it.
Example minimal gating (env-based) around the final fallback branch in executeWithReauth:
// Before attempting kubeconfig fallback:
allowFallback := os.Getenv("CAIB_AUTH_FALLBACK") == "kubeconfig" || os.Getenv("CAIB_AUTH_FALLBACK") == "1"
if !allowFallback {
return fmt.Errorf("OIDC token was rejected after re-authentication; refusing automatic kubeconfig fallback. Set CAIB_AUTH_FALLBACK=kubeconfig to allow fallback explicitly")
}
// then proceed with kubeconfig fallback as today...If you’d prefer a CLI switch (e.g., --auth-fallback=kubeconfig) instead of an env var, I can wire that up.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/buildapi/server.go`:
- Around line 116-159: The initial OperatorConfig load uses context.Background()
which can hang; replace it with a bounded context (e.g., ctx, cancel :=
context.WithTimeout(context.Background(), 5*time.Second); defer cancel()) and
pass that ctx into loadAuthenticationConfigurationFromOperatorConfig instead of
context.Background(); ensure you import time, call defer cancel() immediately
after creating ctx, and adjust the error logging around the load (in the block
that calls getCatalogClient and
loadAuthenticationConfigurationFromOperatorConfig) to surface
context.DeadlineExceeded/timeout errors appropriately so startup won’t hang
indefinitely.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cmd/caib/auth/config.go`:
- Around line 15-21: GetOIDCConfigFromAPI currently creates an http.Transport
with no TLSClientConfig, which breaks trust of custom CA/self-signed certs on
macOS/Windows; update the transport to set Transport.TLSClientConfig using a
SystemCertPool augmented with any extra CAs (e.g., load from SSL_CERT_FILE or
default system pool) and support the CAIB_INSECURE_TLS env var to set
InsecureSkipVerify for dev; modify the GetOIDCConfigFromAPI function to build
TLS config (use x509.SystemCertPool fallback, append parsed certs if provided,
and set InsecureSkipVerify when CAIB_INSECURE_TLS=true) and assign it to the
transport used by the http.Client.
In `@cmd/caib/auth/oidc.go`:
- Around line 423-438: The Windows branch in openBrowser uses cmd "/c" "start"
and then appends the URL directly, which lets start treat URLs with spaces or
special chars (like &) as a window title; update the args construction for
runtime.GOOS == "windows" in openBrowser to pass an explicit empty-title
argument before the URL (e.g., "/c", "start", "" , url) or otherwise ensure the
URL is quoted/passed as a separate argument so exec.Command(cmd, args...)
receives the URL as its own argument and not a title.
In `@cmd/caib/main.go`:
- Around line 108-113: The kubeconfig attempt is shadowing the earlier OIDC
error so the original error context is lost; change the loadTokenFromKubeconfig
call to use distinct variable names (e.g., tokKube, errKube) instead of reusing
err, and when returning the failure from this branch include both errors (wrap
the original OIDC error and the kubeconfig error) in the fmt.Errorf message so
callers see both contexts; locate the block that references authToken and
loadTokenFromKubeconfig and update the variable names and the returned
fmt.Errorf to wrap both errors.
🧹 Nitpick comments (2)
test/e2e/e2e_test.go (1)
446-450: Consider addingMinVersionto TLS config for completeness.While
InsecureSkipVerify: trueis acceptable for e2e tests with self-signed certificates, setting a minimum TLS version is still good practice.♻️ Optional improvement
client := &http.Client{ Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, }, }cmd/caib/auth/oidc.go (1)
51-75: Constructor returns nil on errors without explanation.
NewOIDCAuthreturnsnilwhenissuerURL/clientIDare empty or when the home directory lookup fails. Callers must check for nil, but there's no indication of why it failed. Consider returning(*OIDCAuth, error)for better error handling.♻️ Suggested improvement
-func NewOIDCAuth(issuerURL, clientID string, scopes []string) *OIDCAuth { +func NewOIDCAuth(issuerURL, clientID string, scopes []string) (*OIDCAuth, error) { if issuerURL == "" || clientID == "" { - return nil + return nil, fmt.Errorf("issuerURL and clientID are required") } if len(scopes) == 0 { scopes = []string{"openid", "profile", "email"} } homeDir, err := os.UserHomeDir() if err != nil { - return nil + return nil, fmt.Errorf("failed to determine home directory: %w", err) } cachePath := filepath.Join(homeDir, tokenCacheDir, tokenCacheFile) return &OIDCAuth{ config: OIDCConfig{ IssuerURL: issuerURL, ClientID: clientID, Scopes: scopes, }, cachePath: cachePath, - } + }, nil }
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@cmd/caib/auth/oidc.go`:
- Around line 355-368: In loadTokenCache, validate the cached token's issuer
before using it by comparing cache.Issuer to the current configuration
(a.config.IssuerURL); if they do not match, return an error (or clear the cache
and return a specific error) so an old token from a different OIDC provider
isn't accepted. Update loadTokenCache to perform this check after unmarshalling
into TokenCache and before setting a.tokenCache, referencing the loadTokenCache
function, TokenCache struct, cache.Issuer, and a.config.IssuerURL.
- Around line 312-318: The code currently prefers tokenResp.IDToken then
tokenResp.AccessToken and returns an empty string without error; update the
logic in the token exchange routine (the block that assigns token :=
tokenResp.IDToken / token = tokenResp.AccessToken and the subsequent return
token, nil) to validate that token is non-empty and, if both are empty, return a
clear error (e.g., fmt.Errorf or wrapped error) describing "no token in token
response" including any available tokenResp metadata; ensure callers receive
this error instead of an empty token.
- Around line 149-159: The current port allocation does a net.Listen to get a
free port then closes the listener and later starts an HTTP server, creating a
TOCTOU race and uses "127.0.0.1" vs "localhost" inconsistently; instead keep and
reuse the open listener (do not call listener.Close()), build redirectURI from
the listener's actual address (e.g., use listener.Addr().(*net.TCPAddr).IP and
.Port or listener.Addr().String with normalized host) and start the callback
HTTP server by calling http.Serve(listener, handler) (or
server.Serve(listener)), ensuring the same listener variable is passed to the
server to eliminate the race and make host/port consistent for redirectURI and
binding.
In `@cmd/caib/main.go`:
- Around line 140-146: The TLS configuration block in createBuildAPIClient
ignores the CAIB_INSECURE_TLS env var; detect CAIB_INSECURE_TLS (e.g., value "1"
or "true") and when set append the corresponding client option (e.g.,
buildapiclient.WithInsecureTLS(true) or the project’s equivalent) to opts so the
client is created with insecure TLS; modify the same function
(createBuildAPIClient) to check os.Getenv("CAIB_INSECURE_TLS") and conditionally
append the insecure TLS option alongside the existing WithCACertificate
handling.
In `@internal/controller/operatorconfig/resources.go`:
- Around line 369-410: The current buildInternalJWTSecret always creates a new
signed token but ensureBuildAPIInternalJWTSecret never rotates or updates an
existing Secret, so tokens will expire after 365 days; modify
ensureBuildAPIInternalJWTSecret to read the existing Secret (look for keys
"expires-at" and "signing-key"), parse the expires-at timestamp, and if the
token is expired or will expire within 30 days regenerate by calling
buildInternalJWTSecret (or its logic) and update the Secret in-cluster (use the
reconciler's client Update/Status or a create-or-update pattern), otherwise
leave the Secret unchanged; ensure you handle parse errors by treating them as
needing rotation and propagate/record errors from the update call.
🧹 Nitpick comments (2)
cmd/caib/auth/oidc.go (2)
52-64: Constructor silently returns nil on errors.The constructor returns
nilwithout an error when validation fails oros.UserHomeDir()errors. Callers cannot distinguish between configuration issues (empty URL/clientID) and environment issues (home directory failure). This can lead to confusing nil pointer errors downstream.Consider returning
(*OIDCAuth, error)to provide actionable feedback:♻️ Proposed refactor to return errors
-func NewOIDCAuth(issuerURL, clientID string, scopes []string) *OIDCAuth { +func NewOIDCAuth(issuerURL, clientID string, scopes []string) (*OIDCAuth, error) { if issuerURL == "" || clientID == "" { - return nil + return nil, fmt.Errorf("issuerURL and clientID are required") } if len(scopes) == 0 { scopes = []string{"openid", "profile", "email"} } homeDir, err := os.UserHomeDir() if err != nil { - return nil + return nil, fmt.Errorf("failed to get user home directory: %w", err) } cachePath := filepath.Join(homeDir, tokenCacheDir, tokenCacheFile) return &OIDCAuth{ config: OIDCConfig{ IssuerURL: issuerURL, ClientID: clientID, Scopes: scopes, }, cachePath: cachePath, - } + }, nil }
233-265: Consider extracting shutdown logic to reduce duplication.The server shutdown pattern is repeated four times in the select statement. While functionally correct, this could be simplified using a deferred cleanup function.
♻️ Optional refactor to reduce duplication
+ shutdownServer := func() { + shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + _ = server.Shutdown(shutdownCtx) + cancel() + } + defer shutdownServer() + // Wait for callback select { case code := <-codeChan: - shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - _ = server.Shutdown(shutdownCtx) - cancel() // Exchange code for token token, err := a.exchangeCodeForToken(ctx, discovery.TokenEndpoint, code, redirectURI, codeVerifier) // ... rest of success handling ... case err := <-errChan: - shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - _ = server.Shutdown(shutdownCtx) - cancel() return "", err case <-ctx.Done(): - shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - _ = server.Shutdown(shutdownCtx) - cancel() return "", ctx.Err() case <-time.After(5 * time.Minute): - shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - _ = server.Shutdown(shutdownCtx) - cancel() return "", fmt.Errorf("authentication timeout") }
bkhizgiy
changed the title
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cmd/caib/auth/oidc.go`:
- Around line 51-75: Change NewOIDCAuth to return (*OIDCAuth, error) instead of
silently returning nil: validate issuerURL and clientID and return a descriptive
error if empty (e.g., "issuerURL required" / "clientID required"), call
os.UserHomeDir and return the wrapped error if it fails, and only construct and
return the &OIDCAuth{...} with nil error when successful; update references to
OIDCConfig, cachePath, tokenCacheDir and tokenCacheFile accordingly and adjust
all callers to handle the (auth, err) return signature.
- Around line 344-350: After decoding into the DiscoveryDocument, validate that
discovery.AuthorizationEndpoint and discovery.TokenEndpoint are non-empty and
return a clear error if either is empty (e.g., fmt.Errorf("discovery document
missing %s endpoint", "authorization" or "token")), instead of returning
&discovery, nil; update the function that decodes the document (the block that
defines var discovery DiscoveryDocument and returns &discovery) to perform these
checks and return an error early when endpoints are missing.
In `@internal/controller/operatorconfig/controller.go`:
- Around line 392-427: The reconcile must schedule a future requeue based on the
JWT expiry; change ensureBuildAPIInternalJWTSecret to return (time.Duration,
error) (or an explicit timeUntilCheck) so it computes time.Until(expiresAt) -
internalJWTExpiryThreshold (clamped ≥0) when the secret exists, and return that
duration to the caller; then update Reconcile to use that duration as
ctrl.Result{RequeueAfter: duration} when non-zero (or fall back to a sensible
default like 24h) and propagate errors unchanged. Update callers of
ensureBuildAPIInternalJWTSecret (e.g., in Reconcile) to handle the new return
value and ensure regenerateInternalJWTSecret still returns a zero duration on
immediate regeneration.
🧹 Nitpick comments (8)
internal/controller/operatorconfig/controller.go (1)
390-390: Consider moving constant to the constants block.
internalJWTExpiryThresholdis declared at package level but placed after function definitions. For consistency and discoverability, group it with the other constants at lines 26-32.♻️ Suggested refactor
Move the constant declaration:
const ( operatorNamespace = "automotive-dev-operator-system" finalizerName = "operatorconfig.automotive.sdv.cloud.redhat.com/finalizer" buildAPIName = "ado-build-api" phaseFailed = "Failed" internalJWTSecretName = "ado-build-api-internal-jwt" + internalJWTExpiryThreshold = 30 * 24 * time.Hour // Regenerate when within 30 days of expiry )And remove it from line 390.
test/e2e/e2e_test.go (2)
343-366: Replace customcontains()withstrings.Contains().The standard library's
strings.Contains()is more efficient, idiomatic, and already handles all edge cases correctly. The custom implementation adds unnecessary complexity.♻️ Proposed fix
Remove the custom functions and update the usages at lines 160 and 166:
-func contains(s, substr string) bool { - if len(s) == 0 || len(substr) == 0 { - return false - } - if s == substr { - return true - } - if len(s) < len(substr) { - return false - } - // Check prefix, suffix, or middle - return s[:len(substr)] == substr || - s[len(s)-len(substr):] == substr || - containsMiddle(s, substr) -} - -func containsMiddle(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -}Then update line 160:
- if !contains(tasks, "build-automotive-image") { + if !strings.Contains(tasks, "build-automotive-image") {And line 166:
- if !contains(tasks, "push-artifact-registry") { + if !strings.Contains(tasks, "push-artifact-registry") {
504-508: Consider addingMinVersionto TLS config.While
InsecureSkipVerify: trueis acceptable for e2e tests against self-signed certificates, adding an explicitMinVersionis good practice for security hygiene.🛡️ Proposed fix
client := &http.Client{ Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, + }, }, }Apply the same change to the second occurrence at line 555.
cmd/caib/auth/config.go (2)
22-24: Set minimum TLS version for security hardening.The static analysis correctly identifies that
MinVersionis missing from the TLS configuration. While Go defaults to TLS 1.2 for clients, explicitly setting TLS 1.2 or 1.3 as the minimum provides defense-in-depth and documents the security intent.🔒 Proposed fix to add MinVersion
transport := &http.Transport{ - TLSClientConfig: &tls.Config{RootCAs: pool}, + TLSClientConfig: &tls.Config{ + RootCAs: pool, + MinVersion: tls.VersionTLS12, + }, }
53-67: API response struct includes unused fields.The
apiResponsestruct definesAudiences(line 58) andClaimMappings(lines 60-65) but these fields are never used. If they're for future use, consider adding a comment; otherwise, remove them to reduce cognitive load.♻️ Proposed simplification
var apiResponse struct { ClientID string `json:"clientId"` JWT []struct { Issuer struct { - URL string `json:"url"` - Audiences []string `json:"audiences,omitempty"` + URL string `json:"url"` } `json:"issuer"` - ClaimMappings struct { - Username struct { - Claim string `json:"claim"` - Prefix string `json:"prefix,omitempty"` - } `json:"username"` - } `json:"claimMappings"` } `json:"jwt"` }cmd/caib/auth/oidc.go (3)
154-155: Inconsistency between listener address and redirect URI.The listener binds to
127.0.0.1butredirectURIuseslocalhost. While typically equivalent, they can resolve differently in some configurations (e.g., IPv6-only, custom hosts files). Consider using consistent addressing.♻️ Proposed fix for consistency
port := listener.Addr().(*net.TCPAddr).Port - redirectURI := fmt.Sprintf("http://localhost:%d/callback", port) + redirectURI := fmt.Sprintf("http://127.0.0.1:%d/callback", port)
228-260: Repeated shutdown logic could be consolidated.The same shutdown pattern appears four times. While this is acceptable for a select block, you could use a helper or defer pattern to reduce duplication.
♻️ Optional: Extract shutdown helper
shutdownServer := func() { shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() _ = server.Shutdown(shutdownCtx) }Then replace each shutdown block with
shutdownServer().
277-283: Consider consistent TLS configuration for token endpoint.The HTTP transport uses empty TLS settings, relying on Go defaults. For consistency with
GetOIDCConfigFromAPI(which uses system cert pool), consider applying the same TLS configuration, especially if the IdP uses custom CA certificates.
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
3 participants
Adds OIDC authentication support to
caibCLI. Users can authenticate with identity providers of theire choise instead of managing kubeconfig tokens.When
caibruns without a token, it fetches OIDC config from the Build API, opens a browser for login, and caches the token locally. Tokens are automatically refreshed when expired. Falls back to kubeconfig if OIDC isn't configured.Configure idp via the
ado-build-api-authenticationConfigMap.Run
caibcommands normally - OIDC login triggers automatically. For testing with self-signed certs, useCAIB_INSECURE_TLS=1.Summary by CodeRabbit
New Features
Security / TLS
Infrastructure
Tests
✏️ Tip: You can customize this high-level summary in your review settings.