Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions charts/stackrox-mcp/templates/_helpers.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,21 @@ TLS Secret name - returns existingSecretName if set, otherwise generates name
{{- include "stackrox-mcp.fullname" . }}-tls
{{- end }}
{{- end }}

{{/*
Central CA Secret name - returns existingSecretName if set, otherwise generates name
*/}}
{{- define "stackrox-mcp.centralCASecretName" -}}
{{- if .Values.centralCACert.existingSecretName }}
{{- .Values.centralCACert.existingSecretName }}
{{- else }}
{{- include "stackrox-mcp.fullname" . }}-central-ca
{{- end }}
{{- end }}
Comment thread
mtodor marked this conversation as resolved.

{{/*
Central CA enabled - returns "true" if either cert or existingSecretName is set
*/}}
{{- define "stackrox-mcp.centralCAEnabled" -}}
{{- if or .Values.centralCACert.cert .Values.centralCACert.existingSecretName }}true{{- end }}
{{- end }}
14 changes: 14 additions & 0 deletions charts/stackrox-mcp/templates/central-ca-secret.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{{- if and .Values.centralCACert.cert .Values.centralCACert.existingSecretName }}
{{- fail "centralCACert: cannot set both 'cert' and 'existingSecretName' — use one or the other" }}
{{- end }}
{{- if and .Values.centralCACert.cert (not .Values.centralCACert.existingSecretName) }}
apiVersion: v1
kind: Secret
metadata:
name: {{ include "stackrox-mcp.fullname" . }}-central-ca
labels:
{{- include "stackrox-mcp.labels" . | nindent 4 }}
type: Opaque
data:
ca.crt: {{ .Values.centralCACert.cert | b64enc }}
{{- end }}
3 changes: 3 additions & 0 deletions charts/stackrox-mcp/templates/configmap.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ data:
auth_type: "passthrough"
insecure_skip_tls_verify: {{ .Values.config.central.insecureSkipTLSVerify }}
force_http1: {{ .Values.config.central.forceHTTP1 }}
{{- if include "stackrox-mcp.centralCAEnabled" . }}
ca_cert_path: "/central-ca/ca.crt"
{{- end }}
request_timeout: {{ .Values.config.central.requestTimeout | quote }}
max_retries: {{ .Values.config.central.maxRetries }}
initial_backoff: {{ .Values.config.central.initialBackoff | quote }}
Expand Down
11 changes: 11 additions & 0 deletions charts/stackrox-mcp/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,11 @@ spec:
mountPath: /certs
readOnly: true
{{- end }}
{{- if include "stackrox-mcp.centralCAEnabled" . }}
- name: central-ca
mountPath: /central-ca
readOnly: true
{{- end }}
volumes:
- name: config
configMap:
Expand All @@ -121,6 +126,12 @@ spec:
secretName: {{ include "stackrox-mcp.tlsSecretName" . }}
defaultMode: 0440
{{- end }}
{{- if include "stackrox-mcp.centralCAEnabled" . }}
- name: central-ca
secret:
secretName: {{ include "stackrox-mcp.centralCASecretName" . }}
defaultMode: 0440
Comment on lines +129 to +133
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
{{- 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid complicating configuration because this can be set with: STACKROX_MCP__CENTRAL__CA_CERT_PATH.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

{{- end }}
{{- with .Values.nodeSelector }}
nodeSelector:
{{- toYaml . | nindent 8 }}
Expand Down
6 changes: 6 additions & 0 deletions charts/stackrox-mcp/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ tlsSecret:
# Server TLS Private Key (PEM format)
key: ""

# CA certificate for verifying Central's TLS certificate (e.g., self-signed)
# Only one of cert or existingSecretName should be set.
centralCACert:
existingSecretName: ""
cert: ""

# Resource limits and requests
resources:
limits:
Expand Down
110 changes: 108 additions & 2 deletions internal/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ package client
import (
"context"
"crypto/tls"
"crypto/x509"
"encoding/pem"
"fmt"
"log/slog"
"os"
"sync"
"testing"
"time"
Expand All @@ -23,6 +27,7 @@ import (
const (
minConnectTimeout = 5 * time.Second
backoffJitter = 0.2
maxCACertFileSize = 1 << 20 // 1MB
)

// Client provides gRPC connection to StackRox Central API.
Expand Down Expand Up @@ -229,11 +234,112 @@ func (c *Client) tlsConfig() (*tls.Config, error) {
return nil, errors.Wrap(err, "failed to get central URL hostname")
}

return &tls.Config{
tlsCfg := &tls.Config{
InsecureSkipVerify: c.config.InsecureSkipTLSVerify, //nolint:gosec
MinVersion: tls.VersionTLS12,
ServerName: hostname,
}, nil
}

// 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
}
Comment on lines +243 to +248
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.


tlsCfg.RootCAs = certPool
}

return tlsCfg, nil
}

func loadCACertPool(caCertPath string) (*x509.CertPool, error) {
// File size guard
fileInfo, err := os.Stat(caCertPath)
if err != nil {
return nil, errors.Wrapf(err, "failed to access CA certificate at %s", caCertPath)
}

if !fileInfo.Mode().IsRegular() {
return nil, errors.Errorf("CA certificate path %s is not a regular file", caCertPath)
}

if fileInfo.Size() == 0 {
return nil, errors.Errorf("CA certificate file %s is empty", caCertPath)
}

if fileInfo.Size() > maxCACertFileSize {
return nil, errors.Errorf(
"CA certificate file %s is too large (%d bytes, max %d)",
caCertPath, fileInfo.Size(),
maxCACertFileSize,
)
}
Comment thread
mtodor marked this conversation as resolved.

caCert, err := os.ReadFile(caCertPath) //nolint:gosec
if err != nil {
return nil, errors.Wrapf(err, "failed to read CA certificate from %s", caCertPath)
}

// Get system cert pool, warn on fallback
certPool, err := x509.SystemCertPool()
if err != nil {
slog.Warn("Failed to load system CA pool, using custom CA only", "error", err)

certPool = x509.NewCertPool()
}

if !certPool.AppendCertsFromPEM(caCert) {
return nil, errors.Errorf("failed to parse CA certificate from %s: no valid PEM data found", caCertPath)
}

showCertInfo(caCert)

return certPool, nil
}

// showCertInfo parses and logs certificate metadata.
func showCertInfo(caCert []byte) {
block, _ := pem.Decode(caCert)
if block == nil {
slog.Warn("Unable to decode CA certificate")

return
}

cert, err := x509.ParseCertificate(block.Bytes)
if err != nil {
slog.Warn("Failed to parse CA certificate", "error", err)

return
}

slog.Info("Loaded CA certificate",
"subject", cert.Subject.CommonName,
"issuer", cert.Issuer.CommonName,
"notAfter", cert.NotAfter,
"isCA", cert.IsCA,
)

if !cert.IsCA {
slog.Warn("Provided certificate does not have the CA basic constraint set — TLS verification may fail",
"subject", cert.Subject.CommonName,
)
}

if time.Now().After(cert.NotAfter) {
slog.Warn("CA certificate is expired — TLS verification will fail",
"subject", cert.Subject.CommonName,
"expiredAt", cert.NotAfter,
)
}

if time.Now().Before(cert.NotBefore) {
slog.Warn("CA certificate is not yet valid",
"subject", cert.Subject.CommonName,
"validFrom", cert.NotBefore,
)
}
}

func (c *Client) connectHTTP1(
Expand Down
Loading
Loading