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
113 changes: 110 additions & 3 deletions internal/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@
import (
"context"
"crypto/tls"
"crypto/x509"
"encoding/pem"
"fmt"
"log/slog"
"os"
"sync"
"testing"
"time"
Expand All @@ -23,6 +27,7 @@
const (
minConnectTimeout = 5 * time.Second
backoffJitter = 0.2
maxCACertFileSize = 1 << 20 // 1MB
)

// Client provides gRPC connection to StackRox Central API.
Expand Down Expand Up @@ -65,7 +70,8 @@

tlsConfig, err := c.tlsConfig()
if err != nil {
return err
slog.Error("TLS configuration failed", "error", err)
return errors.New("TLS configuration error: verify CA certificate configuration. Check server logs for details.")

Check failure on line 74 in internal/client/client.go

View workflow job for this annotation

GitHub Actions / Code Style Checks

error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
}

var conn *grpc.ClientConn
Expand Down Expand Up @@ -229,11 +235,112 @@
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 thread
mtodor marked this conversation as resolved.

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