NO-ISSUE: Handle CA files and directories separately in settings#693
Open
jhernand wants to merge 2 commits into
Open
NO-ISSUE: Handle CA files and directories separately in settings#693jhernand wants to merge 2 commits into
jhernand wants to merge 2 commits into
Conversation
|
@jhernand: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (5)
Walkthrough
ChangesCA Certificate Pool Refactor
Sequence Diagram(s)sequenceDiagram
rect rgba(200, 100, 50, 0.5)
note over loginCmd: Login --ca-file persistence
loginCmd->>loginCmd: filepath.Abs + Clean
loginCmd->>loginCmd: os.Stat(absPath)
alt is directory
loginCmd->>config: AddCaFile{Name: absPath}
else is regular file
loginCmd->>loginCmd: os.ReadFile(absPath)
loginCmd->>config: AddCaFile{Name: absPath, Content: bytes}
end
end
rect rgba(50, 100, 200, 0.5)
note over Settings: CA pool construction (createCaPool)
Settings->>Settings: iterate CaFiles
alt relative path
Settings->>Settings: warn, skip
else absolute + Content
Settings->>Settings: os.ReadFile (fallback to Content)
Settings->>CertPoolBuilder: AddCertificates(pemBytes)
else absolute, no Content (directory)
Settings->>Settings: os.Stat confirm directory
Settings->>CertPoolBuilder: AddFiles(dirPath)
end
Settings->>CertPoolBuilder: Build()
CertPoolBuilder->>x509CertPool: AppendCertsFromPEM / AddCert
CertPoolBuilder-->>Settings: *x509.CertPool
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmd/cli/login/login_cmd.go`:
- Around line 368-380: The CA file reading logic in the loop that processes
c.args.caFiles does not handle directory inputs properly. When os.ReadFile is
called on a directory path, it fails with a "is a directory" error. To fix this,
before calling os.ReadFile on the caPath, check if the path is a directory using
filepath.IsDir or os.Stat. If it is a directory, iterate through all files in
that directory and read each file individually, adding them to the configuration
via cfg.AddCaFile for each file found. If it is a regular file, proceed with the
current os.ReadFile logic. This will allow the command to accept both individual
CA files and directories containing multiple CA files without hard-failing
login.
In `@internal/config/config_settings.go`:
- Around line 571-580: The absolute-path CA selection logic in the block where
filepath.IsAbs(caFile.Name) is true is inverted, causing unreadable files to be
added to caFiles and readable files to be skipped. When os.Stat() returns an
error (indicating the file is missing or unreadable), the code incorrectly
appends the path to caFiles and continues, bypassing the Content fallback. Fix
this by inverting the condition: only append caFile.Name to caFiles when there
is NO error from os.Stat(), meaning the file is readable and should be used.
When os.Stat() returns an error, remove the continue statement and let the code
proceed to check caFile.Content as a fallback mechanism instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b0b744ac-e163-47cd-9532-7ea0adb4ab81
📒 Files selected for processing (4)
internal/cmd/cli/login/login_cmd.gointernal/config/config_settings.gointernal/network/cert_pool.gointernal/network/cert_pool_test.go
jhernand
force-pushed
the
store_all_ca_files_on_login
branch
from
b80ecf4 to
4ee71ae
Compare
jhernand
commented
Jun 14, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/network/cert_pool.go (2)
279-279:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTypo in log message: "Igoring" → "Ignoring".
- "Igoring file because it doesn't have a valid extension", + "Ignoring file because it doesn't have a valid extension",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/network/cert_pool.go` at line 279, Fix the typo in the log message string at the cert_pool.go file where "Igoring" is misspelled. Change "Igoring" to "Ignoring" in the log message that states the file is being ignored because it doesn't have a valid extension.
176-186:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRisk: Misleading error message due to wrapping nil error.
Severity: Low | Impact: Debugging difficulty
AppendCertsFromPEMreturns abool, not an error. Wrappingerr(which isnilat this point) with%wproduces output like"failed to add certificate to pool: %!w(<nil>)". This obscures the actual failure reason—invalid or empty PEM data.🔧 Proposed fix
case string: ok := pool.AppendCertsFromPEM([]byte(cert)) if !ok { - err = fmt.Errorf("failed to add certificate to pool: %w", err) + err = errors.New("failed to add certificate to pool: PEM data contains no valid certificates") return } case []byte: ok := pool.AppendCertsFromPEM(cert) if !ok { - err = fmt.Errorf("failed to add certificate to pool: %w", err) + err = errors.New("failed to add certificate to pool: PEM data contains no valid certificates") return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/network/cert_pool.go` around lines 176 - 186, The error handling for AppendCertsFromPEM returns only a boolean success/failure indicator, not an error. When AppendCertsFromPEM returns false, the code attempts to wrap a nil error using %w format specifier, producing misleading output like "failed to add certificate to pool: %!w(<nil>)". Fix both occurrences (the string case and []byte case) by removing the %w format verb and the nil err variable from the fmt.Errorf call, and instead use a plain error message that describes the actual failure reason (such as invalid or empty PEM data) without attempting to wrap a non-existent underlying error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/config_settings.go`:
- Around line 571-586: The filepath accessibility check using os.Stat only
verifies the file exists but does not guarantee read access, causing the code to
skip the inline PEM fallback even when the file cannot actually be read. Instead
of just checking if os.Stat succeeds, verify that the CA file is actually
readable by attempting to open it or by validating read permissions. Only add
the file to caFiles and continue if you can confirm it is readable; otherwise,
allow the code to fall through to the fallback logic on lines 584-586 where
caFile.Content is used as an alternative when an absolute path file is not
accessible.
In `@internal/network/cert_pool_test.go`:
- Around line 736-738: The tls.Config struct in the TLSClientConfig
initialization is missing the MinVersion field specification. Add the MinVersion
field to the tls.Config struct and set it to tls.VersionTLS13 to demonstrate
cryptographic best practices and ensure the test code sets a proper example for
secure TLS configuration. This change applies to the Transport configuration
around line 736-738 in the cert_pool_test.go file where the tls.Config is being
instantiated.
---
Outside diff comments:
In `@internal/network/cert_pool.go`:
- Line 279: Fix the typo in the log message string at the cert_pool.go file
where "Igoring" is misspelled. Change "Igoring" to "Ignoring" in the log message
that states the file is being ignored because it doesn't have a valid extension.
- Around line 176-186: The error handling for AppendCertsFromPEM returns only a
boolean success/failure indicator, not an error. When AppendCertsFromPEM returns
false, the code attempts to wrap a nil error using %w format specifier,
producing misleading output like "failed to add certificate to pool:
%!w(<nil>)". Fix both occurrences (the string case and []byte case) by removing
the %w format verb and the nil err variable from the fmt.Errorf call, and
instead use a plain error message that describes the actual failure reason (such
as invalid or empty PEM data) without attempting to wrap a non-existent
underlying error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1c4c384c-7d1e-4bf1-b861-c7137b31a7a1
📒 Files selected for processing (15)
internal/cmd/cli/login/login_cmd.gointernal/cmd/service/dev/watch/dev_watch_cmd.gointernal/cmd/service/probe/grpcserver/probe_grpc_server_cmd.gointernal/cmd/service/start/consoleproxy/start_console_proxy_cmd.gointernal/cmd/service/start/controller/start_controller_cmd.gointernal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/config/config_settings.gointernal/network/cert_pool.gointernal/network/cert_pool_test.gointernal/network/grpc_client.gointernal/oauth/oauth_discovery_tool.gointernal/oauth/oauth_token_source.gointernal/oauth/oauth_token_source_test.goit/it_tool.go
…der` The `CertPoolBuilder` previously only supported loading CA certificates from files on disk. This adds `AddCertificate` and `AddCertificates` methods that accept certificates directly as PEM-encoded strings, byte slices, or parsed `*x509.Certificate` objects. This is useful for callers that already have certificates in memory and don't need to write them to a temporary file first. The `Build` method performs a type switch on each value: strings and byte slices are appended via `AppendCertsFromPEM`, X.509 objects are added with `AddCert`, and any other type produces a descriptive error. Assisted-by: Cursor Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
The login command now distinguishes between regular files and directories when storing CA entries. Regular files are stored with both the absolute path and the PEM content, while directories are stored with only the path. All paths are resolved to absolute form before being saved, so the configuration works regardless of the working directory at login time. On the loading side, `createCaPool` classifies entries into three categories. Entries with a relative path are ignored with a warning, since the tool may be invoked from a different working directory. Entries with an absolute path and stored content represent files whose content was captured at configuration time; these are re-read from disk so that rotated certificates are picked up automatically, falling back to the stored content when the file is no longer accessible. Entries with an absolute path but no stored content represent directories; these are passed to the certificate pool builder so their contents are scanned on every invocation, allowing the user to add or remove certificate files inside the directory. Assisted-by: Cursor Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
jhernand
force-pushed
the
store_all_ca_files_on_login
branch
from
4ee71ae to
5b650f5
Compare
jhernand
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmd/cli/login/login_cmd.go`:
- Around line 562-572: The inferFlow method checks for OAuth flow ambiguity
between credentials and password hints without first checking if token-based
authentication flags are set. When --token or --token-script flags are provided,
the method should skip OAuth flow inference entirely. Add an early return check
at the beginning of the inferFlow method (before the credentialsHint and
passwordHint logic) to detect if c.flags.Changed("token") or
c.flags.Changed("token-script") is true, and return nil immediately if either is
set. This prevents the ambiguity error from blocking valid token-based login
commands.
In `@internal/config/config_settings.go`:
- Around line 581-588: The code unconditionally skips CA entries with relative
paths, but it should first check whether the entry has valid stored PEM content
before discarding it. Modify the branch that checks `!filepath.IsAbs(caPath)` to
verify if the CA entry contains valid Content (the stored PEM data). Only skip
the entry and continue if no Content exists; if Content is present, proceed to
use it in the CA pool rebuild instead of silently dropping it. This prevents
breaking previously saved configs that rely on stored PEM content for relative
path entries.
In `@internal/network/cert_pool.go`:
- Around line 167-177: The error messages in both the string and []byte cases of
the certificate parsing are wrapping a nil err variable with the %w verb, which
produces misleading error messages. In both locations where AppendCertsFromPEM
returns false (lines around the error handling for the string case and the
[]byte case), remove the %w verb and the nil err variable from the fmt.Errorf
call. Instead, create a plain error message that describes the PEM parsing
failure without attempting to wrap a nil error value. This will ensure the error
message accurately reflects the actual problem of PEM certificate parsing
failure rather than displaying a wrapped nil error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6239cec0-c7f4-4548-8192-253ad0988a7c
📒 Files selected for processing (5)
internal/cmd/cli/login/login_cmd.gointernal/config/config_settings.gointernal/config/config_settings_test.gointernal/network/cert_pool.gointernal/network/cert_pool_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmd/cli/login/login_cmd.go`:
- Around line 562-572: The inferFlow method checks for OAuth flow ambiguity
between credentials and password hints without first checking if token-based
authentication flags are set. When --token or --token-script flags are provided,
the method should skip OAuth flow inference entirely. Add an early return check
at the beginning of the inferFlow method (before the credentialsHint and
passwordHint logic) to detect if c.flags.Changed("token") or
c.flags.Changed("token-script") is true, and return nil immediately if either is
set. This prevents the ambiguity error from blocking valid token-based login
commands.
In `@internal/config/config_settings.go`:
- Around line 581-588: The code unconditionally skips CA entries with relative
paths, but it should first check whether the entry has valid stored PEM content
before discarding it. Modify the branch that checks `!filepath.IsAbs(caPath)` to
verify if the CA entry contains valid Content (the stored PEM data). Only skip
the entry and continue if no Content exists; if Content is present, proceed to
use it in the CA pool rebuild instead of silently dropping it. This prevents
breaking previously saved configs that rely on stored PEM content for relative
path entries.
In `@internal/network/cert_pool.go`:
- Around line 167-177: The error messages in both the string and []byte cases of
the certificate parsing are wrapping a nil err variable with the %w verb, which
produces misleading error messages. In both locations where AppendCertsFromPEM
returns false (lines around the error handling for the string case and the
[]byte case), remove the %w verb and the nil err variable from the fmt.Errorf
call. Instead, create a plain error message that describes the PEM parsing
failure without attempting to wrap a nil error value. This will ensure the error
message accurately reflects the actual problem of PEM certificate parsing
failure rather than displaying a wrapped nil error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6239cec0-c7f4-4548-8192-253ad0988a7c
📒 Files selected for processing (5)
internal/cmd/cli/login/login_cmd.gointernal/config/config_settings.gointernal/config/config_settings_test.gointernal/network/cert_pool.gointernal/network/cert_pool_test.go
🛑 Comments failed to post (3)
internal/cmd/cli/login/login_cmd.go (1)
562-572:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkip OAuth flow inference when
--tokenor--token-scriptis set.
inferFlowcan return the ambiguity error even when OAuth is not the selected auth path. That blocks otherwise valid token-based login commands.Risk severity: Minor. Impact: avoidable login failure for mixed-flag invocations.
Suggested fix
func (c *runnerContext) inferFlow(ctx context.Context) error { + if c.args.token != "" || c.args.tokenScript != "" { + return nil + } if c.flags.Changed("flow") || c.flags.Changed("oauth-flow") { return nil }📝 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.func (c *runnerContext) inferFlow(ctx context.Context) error { if c.args.token != "" || c.args.tokenScript != "" { return nil } if c.flags.Changed("flow") || c.flags.Changed("oauth-flow") { return nil } credentialsHint := c.flags.Changed("client-secret") || c.flags.Changed("oauth-client-secret") passwordHint := c.flags.Changed("user") || c.flags.Changed("oauth-user") || c.flags.Changed("password") || c.flags.Changed("oauth-password") if credentialsHint && passwordHint { c.console.Render(ctx, "ambiguous_flow.txt", nil) return exit.Error(1) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/cli/login/login_cmd.go` around lines 562 - 572, The inferFlow method checks for OAuth flow ambiguity between credentials and password hints without first checking if token-based authentication flags are set. When --token or --token-script flags are provided, the method should skip OAuth flow inference entirely. Add an early return check at the beginning of the inferFlow method (before the credentialsHint and passwordHint logic) to detect if c.flags.Changed("token") or c.flags.Changed("token-script") is true, and return nil immediately if either is set. This prevents the ambiguity error from blocking valid token-based login commands.internal/config/config_settings.go (1)
581-588:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t discard relative CA entries when stored PEM content exists.
This branch drops all relative paths unconditionally, including entries that still carry valid
Content. That can break previously saved configs by silently removing trusted roots during CA pool rebuild.Risk severity: Major. Impact: TLS trust regression / connection failures after upgrade for existing configs.
Suggested fix
if !filepath.IsAbs(caPath) { - c.logger.WarnContext( - ctx, - "Ignoring CA entry with relative path", - slog.String("file", caPath), - ) + if caContent != "" { + c.logger.WarnContext( + ctx, + "CA entry has relative path, using stored content", + slog.String("file", caPath), + ) + caCerts = append(caCerts, caContent) + continue + } + c.logger.WarnContext( + ctx, + "Ignoring CA entry with relative path and no stored content", + slog.String("file", caPath), + ) continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/config_settings.go` around lines 581 - 588, The code unconditionally skips CA entries with relative paths, but it should first check whether the entry has valid stored PEM content before discarding it. Modify the branch that checks `!filepath.IsAbs(caPath)` to verify if the CA entry contains valid Content (the stored PEM data). Only skip the entry and continue if no Content exists; if Content is present, proceed to use it in the CA pool rebuild instead of silently dropping it. This prevents breaking previously saved configs that rely on stored PEM content for relative path entries.internal/network/cert_pool.go (1)
167-177:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a concrete parse error instead of wrapping
errhere.
erris nil on this path, so Line [170] / Line [176] produce a misleading wrapped error instead of the real PEM-parse failure.Risk severity: Minor. Impact: poor diagnostics and brittle downstream error handling.
Suggested fix
case string: ok := pool.AppendCertsFromPEM([]byte(cert)) if !ok { - err = fmt.Errorf("failed to add certificate to pool: %w", err) + err = errors.New("failed to add certificate to pool: invalid PEM content") return } case []byte: ok := pool.AppendCertsFromPEM(cert) if !ok { - err = fmt.Errorf("failed to add certificate to pool: %w", err) + err = errors.New("failed to add certificate to pool: invalid PEM content") return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/network/cert_pool.go` around lines 167 - 177, The error messages in both the string and []byte cases of the certificate parsing are wrapping a nil err variable with the %w verb, which produces misleading error messages. In both locations where AppendCertsFromPEM returns false (lines around the error handling for the string case and the []byte case), remove the %w verb and the nil err variable from the fmt.Errorf call. Instead, create a plain error message that describes the PEM parsing failure without attempting to wrap a nil error value. This will ensure the error message accurately reflects the actual problem of PEM certificate parsing failure rather than displaying a wrapped nil error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The CLI login command and the configuration loader now treat CA files and
directories as distinct categories.
When the user passes
--ca-filearguments to the login command, regular filesare stored with both their absolute path and their PEM content. Directories are
stored with only their absolute path. All paths are resolved to absolute form
before being saved, so the configuration works regardless of the working
directory at login time.
When the configuration is loaded,
createCaPoolclassifies each stored CA entryinto one of three categories:
from a different working directory.
so that rotated certificates are picked up automatically. If the file is no
longer accessible, the stored content is used as a fallback.
certificate pool builder so their contents are scanned on every invocation,
allowing the user to add or remove certificate files inside the directory
without re-running login.
This also adds
AddCertificateandAddCertificatesmethods toCertPoolBuilderso that certificates can be provided directly as PEM-encodedstrings, byte slices, or parsed
*x509.Certificateobjects, without writingtemporary files.
Test plan
AddCertificate/AddCertificatescovering strings,byte slices,
*x509.Certificateobjects, mixed types, and error cases.createCaPoolcovering all three entry categories:relative paths ignored, absolute files with content (readable and
non-readable), absolute directories, non-existent paths, and regular files
without content.
Summary by CodeRabbit
New Features
Tests