ROX-33250: Add support for self-singed CA#121
Conversation
mtodor
force-pushed
the
mtodor/ROX-33250-add-self-signed-certs
branch
from
4b190eb to
2769128
Compare
❌ 2 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
E2E Test ResultsCommit: 1dac488 |
📝 WalkthroughWalkthroughAdds optional Central CA certificate support: Helm helpers and templates to supply or reference a CA secret, configmap/deployment changes to expose a CA file, a new CentralConfig CA path field, and client code to load, validate, and apply CA certs for TLS verification. ChangesCentral CA feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/client/client_test.go (1)
238-245: Assert the loaded CA is actually present in the pool.These success-path tests only prove that
loadCACertPoolreturned no error and a non-nil pool. They would still pass if the PEM contents were silently ignored, so they do not fully protect the new behavior.♻️ Example assertion pattern
pool, err := loadCACertPool(path) require.NoError(t, err) require.NotNil(t, pool) +expected := x509.NewCertPool() +require.True(t, expected.AppendCertsFromPEM(certPEM)) +assert.True(t, expected.Equal(pool))That same assertion shape should work for the mixed-PEM case too, since the private key block is expected to be ignored.
Also applies to: 560-582
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/client_test.go` around lines 238 - 245, The tests currently only check for non-nil pool; update TestLoadCACertPool_ValidCACert (and the mixed-PEM test) to assert the actual CA cert was added to the pool: parse the generated PEM from generateTestCACert (or re-read the file written by writeTestFile) into an x509.Certificate, then verify that the pool returned by loadCACertPool contains that certificate (e.g., by checking pool.Subjects() includes the cert.RawSubject or otherwise matching the cert's Raw bytes). Apply the same presence assertion in the mixed-PEM test where the private key block should be ignored and only the CA cert should be present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/client/client.go`:
- Around line 243-250: The tlsConfig() currently loads and validates
c.config.CACertPath even when c.config.InsecureSkipTLSVerify is true; update
tlsConfig() to check c.config.InsecureSkipTLSVerify first and skip calling
loadCACertPool or setting tlsCfg.RootCAs when InsecureSkipTLSVerify is true
(honor the config warning), while still setting tlsCfg.InsecureSkipVerify =
true; ensure loadCACertPool is only invoked when InsecureSkipTLSVerify is false
and c.config.CACertPath != "" to avoid failing connection setup due to
missing/invalid CA files.
---
Nitpick comments:
In `@internal/client/client_test.go`:
- Around line 238-245: The tests currently only check for non-nil pool; update
TestLoadCACertPool_ValidCACert (and the mixed-PEM test) to assert the actual CA
cert was added to the pool: parse the generated PEM from generateTestCACert (or
re-read the file written by writeTestFile) into an x509.Certificate, then verify
that the pool returned by loadCACertPool contains that certificate (e.g., by
checking pool.Subjects() includes the cert.RawSubject or otherwise matching the
cert's Raw bytes). Apply the same presence assertion in the mixed-PEM test where
the private key block should be ignored and only the CA cert should be present.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 02d8cbab-e2ee-4748-ba24-e2372f220670
📒 Files selected for processing (9)
charts/stackrox-mcp/templates/_helpers.tplcharts/stackrox-mcp/templates/central-ca-secret.yamlcharts/stackrox-mcp/templates/configmap.yamlcharts/stackrox-mcp/templates/deployment.yamlcharts/stackrox-mcp/values.yamlinternal/client/client.gointernal/client/client_test.gointernal/config/config.gointernal/config/config_test.go
mtodor
force-pushed
the
mtodor/ROX-33250-add-self-signed-certs
branch
from
2769128 to
814901f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/stackrox-mcp/templates/_helpers.tpl`:
- Around line 87-93: The template stackrox-mcp.centralCASecretName can produce
names >63 chars when appending "-central-ca" to include "stackrox-mcp.fullname";
truncate the fullname portion before adding the suffix so the total DNS label
length stays <=63. Update the else branch to take include
"stackrox-mcp.fullname" . and pipe it through a truncation (e.g., sprig's trunc)
to 52 characters (63 - len("-central-ca") = 52), then append "-central-ca" so
the resulting secret name always fits Kubernetes DNS label limits.
In `@internal/client/client_test.go`:
- Around line 545-558: The loop using conn.GetState and conn.WaitForStateChange
can exit due to timeout and still satisfy assert.NotEqual, hiding regressions;
modify the loop in the test to record whether connectivity.TransientFailure was
actually observed (e.g., set a boolean observedTransientFailure when state ==
connectivity.TransientFailure) and after the loop assert that
observedTransientFailure is true (or call t.Fatalf on timeout) instead of only
asserting conn.GetState() != connectivity.Ready; reference conn.GetState,
conn.WaitForStateChange, connectivity.TransientFailure, and connectivity.Ready
when making the change.
🪄 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 YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 42f20684-e724-47bc-9ba9-f8bd8f33a92a
📒 Files selected for processing (9)
charts/stackrox-mcp/templates/_helpers.tplcharts/stackrox-mcp/templates/central-ca-secret.yamlcharts/stackrox-mcp/templates/configmap.yamlcharts/stackrox-mcp/templates/deployment.yamlcharts/stackrox-mcp/values.yamlinternal/client/client.gointernal/client/client_test.gointernal/config/config.gointernal/config/config_test.go
| for { | ||
| state := conn.GetState() | ||
| if state == connectivity.TransientFailure { | ||
| break | ||
| } | ||
|
|
||
| if !conn.WaitForStateChange(ctx, state) { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| assert.NotEqual(t, connectivity.Ready, conn.GetState(), | ||
| "connection must not reach Ready state without CA cert for self-signed server") | ||
| } |
There was a problem hiding this comment.
Negative TLS test can pass without proving handshake rejection
Line 551 breaks on timeout and still passes with NotEqual(Ready), which can hide regressions (e.g., connection never actually attempted/failed). The test should assert it observed TransientFailure (or explicitly fail on timeout) to validate the rejection path.
Suggested fix
- for {
+ reachedTransientFailure := false
+ for {
state := conn.GetState()
if state == connectivity.TransientFailure {
+ reachedTransientFailure = true
break
}
if !conn.WaitForStateChange(ctx, state) {
- break
+ t.Fatalf("timeout waiting for TransientFailure; last state: %s", state)
}
}
- assert.NotEqual(t, connectivity.Ready, conn.GetState(),
- "connection must not reach Ready state without CA cert for self-signed server")
+ assert.True(t, reachedTransientFailure,
+ "expected TransientFailure without CA cert for self-signed server")
+ assert.NotEqual(t, connectivity.Ready, conn.GetState(),
+ "connection must not reach Ready state without CA cert for self-signed server")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/client/client_test.go` around lines 545 - 558, The loop using
conn.GetState and conn.WaitForStateChange can exit due to timeout and still
satisfy assert.NotEqual, hiding regressions; modify the loop in the test to
record whether connectivity.TransientFailure was actually observed (e.g., set a
boolean observedTransientFailure when state == connectivity.TransientFailure)
and after the loop assert that observedTransientFailure is true (or call
t.Fatalf on timeout) instead of only asserting conn.GetState() !=
connectivity.Ready; reference conn.GetState, conn.WaitForStateChange,
connectivity.TransientFailure, and connectivity.Ready when making the change.
mtodor
force-pushed
the
mtodor/ROX-33250-add-self-signed-certs
branch
from
814901f to
1dac488
Compare
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 `@charts/stackrox-mcp/templates/deployment.yaml`:
- Around line 129-133: The secret volume mount for "central-ca" must map a
configurable key in the existing secret to the path /central-ca/ca.crt; update
the deployment.yaml secret block (the template that uses include
"stackrox-mcp.centralCAEnabled" and include "stackrox-mcp.centralCASecretName")
to add an items mapping that uses a new/existing value like
centralCACert.existingSecretKey (defaulting to "ca.crt") and maps that key to
path "ca.crt" so the configmap's expected file (/central-ca/ca.crt) is always
present.
In `@internal/client/client.go`:
- Around line 243-248: Wrap the raw errors returned by loadCACertPool so
Connect() does not propagate filesystem/parsing errors directly: in the
tlsConfig construction path (the block that checks
c.config.InsecureSkipTLSVerify and calls loadCACertPool) replace the naked
return of err with a wrapped user-facing error using NewError (e.g.,
NewError(err, "failed to load CA certificates") or similar) so Connect() always
returns client.NewError-wrapped errors; update the tlsConfig()/Connect() error
return sites to use that wrapped 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 YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0a5167e0-64cc-48f3-b644-92dd9e356803
📒 Files selected for processing (9)
charts/stackrox-mcp/templates/_helpers.tplcharts/stackrox-mcp/templates/central-ca-secret.yamlcharts/stackrox-mcp/templates/configmap.yamlcharts/stackrox-mcp/templates/deployment.yamlcharts/stackrox-mcp/values.yamlinternal/client/client.gointernal/client/client_test.gointernal/config/config.gointernal/config/config_test.go
| {{- if include "stackrox-mcp.centralCAEnabled" . }} | ||
| - name: central-ca | ||
| secret: | ||
| secretName: {{ include "stackrox-mcp.centralCASecretName" . }} | ||
| defaultMode: 0440 |
There was a problem hiding this comment.
Make the existing CA secret key configurable.
configmap.yaml always points the client at /central-ca/ca.crt, but this volume mounts whatever keys already exist in the referenced secret. That means centralCACert.existingSecretName only works when the secret happens to store the bundle under ca.crt; otherwise the chart installs cleanly and fails TLS setup at runtime. Add a key field and remap it to ca.crt here, or explicitly document/enforce that contract.
🔧 Suggested change
- name: central-ca
secret:
secretName: {{ include "stackrox-mcp.centralCASecretName" . }}
+ items:
+ - key: {{ default "ca.crt" .Values.centralCACert.existingSecretKey | quote }}
+ path: ca.crt
defaultMode: 0440 centralCACert:
existingSecretName: ""
+ existingSecretKey: "ca.crt"
cert: ""📝 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.
| {{- if include "stackrox-mcp.centralCAEnabled" . }} | |
| - name: central-ca | |
| secret: | |
| secretName: {{ include "stackrox-mcp.centralCASecretName" . }} | |
| defaultMode: 0440 | |
| {{- if include "stackrox-mcp.centralCAEnabled" . }} | |
| - name: central-ca | |
| secret: | |
| secretName: {{ include "stackrox-mcp.centralCASecretName" . }} | |
| items: | |
| - key: {{ default "ca.crt" .Values.centralCACert.existingSecretKey | quote }} | |
| path: ca.crt | |
| defaultMode: 0440 |
🤖 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 `@charts/stackrox-mcp/templates/deployment.yaml` around lines 129 - 133, The
secret volume mount for "central-ca" must map a configurable key in the existing
secret to the path /central-ca/ca.crt; update the deployment.yaml secret block
(the template that uses include "stackrox-mcp.centralCAEnabled" and include
"stackrox-mcp.centralCASecretName") to add an items mapping that uses a
new/existing value like centralCACert.existingSecretKey (defaulting to "ca.crt")
and maps that key to path "ca.crt" so the configmap's expected file
(/central-ca/ca.crt) is always present.
There was a problem hiding this comment.
I would avoid complicating configuration because this can be set with: STACKROX_MCP__CENTRAL__CA_CERT_PATH.
There was a problem hiding this comment.
@mtodor, that's a fair point — since no items mapping is specified, Kubernetes mounts all keys from the referenced secret as files under /central-ca/, so a user with a different key name can set STACKROX_MCP__CENTRAL__CA_CERT_PATH=/central-ca/<their-key> and the env var will override the configmap value via Viper. The default case (key named ca.crt) works without any extra configuration.
It would be worth adding a note in the chart's values.yaml or README that when existingSecretName is used with a secret whose key is not ca.crt, users must also set the STACKROX_MCP__CENTRAL__CA_CERT_PATH environment variable to the correct path. That keeps the configuration simple while making the contract explicit.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
| // There is no reason to load certificates if we allow InsecureSkipTLSVerify. | ||
| if !c.config.InsecureSkipTLSVerify && c.config.CACertPath != "" { | ||
| certPool, err := loadCACertPool(c.config.CACertPath) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Wrap CA-load failures with NewError before they escape Connect().
This new path can return raw filesystem/parsing errors from loadCACertPool(), and Connect() currently forwards the tlsConfig() failure unchanged. That exposes internal mount paths and bypasses the client package’s normal user-facing error wrapper.
🔧 Suggested change
tlsConfig, err := c.tlsConfig()
if err != nil {
- return err
+ return NewError(err, "Connect")
}As per coding guidelines, "Proper error wrapping with client.NewError for user-facing errors".
🤖 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/client/client.go` around lines 243 - 248, Wrap the raw errors
returned by loadCACertPool so Connect() does not propagate filesystem/parsing
errors directly: in the tlsConfig construction path (the block that checks
c.config.InsecureSkipTLSVerify and calls loadCACertPool) replace the naked
return of err with a wrapped user-facing error using NewError (e.g.,
NewError(err, "failed to load CA certificates") or similar) so Connect() always
returns client.NewError-wrapped errors; update the tlsConfig()/Connect() error
return sites to use that wrapped error.
3 participants
Description
This PR is adding support for self-signed certificates. In some cases customers are using own certificates and this feature will support certificate validation. Otherwise it would be required to use insecure TLS.
Validation
AI-assisted development
In this case different environment was used and different organization of work.