feat: add tlsInsecureSkipVerify to ExternalModel spec#646
feat: add tlsInsecureSkipVerify to ExternalModel spec#646nerdalert wants to merge 1 commit intoopendatahub-io:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds an optional boolean spec field Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Issues identified
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
🧹 Nitpick comments (3)
maas-controller/api/maas/v1alpha1/externalmodel_types.go (2)
69-74: Consider adding a printcolumn to surface insecure TLS status inkubectl getoutput.Security-sensitive configurations benefit from visibility at the list level. This helps operators quickly identify resources with relaxed security settings during audits or troubleshooting.
🔧 Proposed printcolumn addition
//+kubebuilder:printcolumn:name="Endpoint",type="string",JSONPath=".spec.endpoint" +//+kubebuilder:printcolumn:name="InsecureTLS",type="boolean",JSONPath=".spec.tlsInsecureSkipVerify",priority=1 //+kubebuilder:printcolumn:name="Phase",type="string",JSONPath=".status.phase"Using
priority=1keeps it hidden from default output but visible with-o wide.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-controller/api/maas/v1alpha1/externalmodel_types.go` around lines 69 - 74, Add a kubebuilder printcolumn to surface the insecure TLS flag (e.g., "Insecure TLS") in the CRD print columns next to the existing Provider/Endpoint/Phase/Age tags; insert a line like a kubebuilder:printcolumn with name="Insecure TLS", type="boolean" (or "string"), JSONPath pointing to your spec field (for example ".spec.insecureSkipVerify" or ".spec.tls.insecureSkipVerify") and include priority=1 so it’s hidden by default; place this tag alongside the other kubebuilder annotations above the ExternalModel type definition in externalmodel_types.go.
44-56: Add explicit+kubebuilder:default=falsemarker for CRD schema clarity.While Go's zero value handles the default correctly, adding the explicit marker improves the generated OpenAPI schema and
kubectl explainoutput. This is especially important for security-sensitive fields where operators need unambiguous documentation.🔧 Proposed marker addition
// WARNING: Do not enable in production. This is intended for testing only. // // Default: false (certificates are verified). // +optional + // +kubebuilder:default=false TLSInsecureSkipVerify bool `json:"tlsInsecureSkipVerify,omitempty"`As per coding guidelines: "Kubebuilder validation markers on all user-facing fields."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-controller/api/maas/v1alpha1/externalmodel_types.go` around lines 44 - 56, The TLSInsecureSkipVerify boolean field lacks an explicit kubebuilder default marker; add the kubebuilder marker "// +kubebuilder:default=false" immediately above the TLSInsecureSkipVerify field declaration in externalmodel_types.go so the generated CRD/OpenAPI schema and kubectl explain show the explicit default for the TLSInsecureSkipVerify field.maas-controller/pkg/reconciler/externalmodel/reconciler.go (1)
279-286: Field propagation is correct; consider loggingtlsInsecureSkipVerifyfor operational visibility.The mapping from
extModel.Spec.TLSInsecureSkipVerifyis clean. However, the reconcile log at lines 113-117 doesn't capture whether insecure TLS is active. When operators troubleshoot connection issues or audit security posture, this visibility would help.🔧 Proposed logging enhancement
log.Info("Reconciling ExternalModel", "provider", spec.Provider, "endpoint", spec.Endpoint, "tls", spec.TLS, + "tlsInsecureSkipVerify", spec.TLSInsecureSkipVerify, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-controller/pkg/reconciler/externalmodel/reconciler.go` around lines 279 - 286, The ExternalModelSpec is correctly setting TLSInsecureSkipVerify into spec (TLSInsecureSkipVerify: extModel.Spec.TLSInsecureSkipVerify), but the reconcile log that reports the external model connection info doesn't include that field; update the reconcile logging call (the log/info call that currently prints Provider, Endpoint, PathPrefix, Port) to also log TLSInsecureSkipVerify by adding extModel.Spec.TLSInsecureSkipVerify or spec.TLSInsecureSkipVerify to the structured log entry so operators can see whether insecure TLS is active.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@maas-controller/api/maas/v1alpha1/externalmodel_types.go`:
- Around line 69-74: Add a kubebuilder printcolumn to surface the insecure TLS
flag (e.g., "Insecure TLS") in the CRD print columns next to the existing
Provider/Endpoint/Phase/Age tags; insert a line like a kubebuilder:printcolumn
with name="Insecure TLS", type="boolean" (or "string"), JSONPath pointing to
your spec field (for example ".spec.insecureSkipVerify" or
".spec.tls.insecureSkipVerify") and include priority=1 so it’s hidden by
default; place this tag alongside the other kubebuilder annotations above the
ExternalModel type definition in externalmodel_types.go.
- Around line 44-56: The TLSInsecureSkipVerify boolean field lacks an explicit
kubebuilder default marker; add the kubebuilder marker "//
+kubebuilder:default=false" immediately above the TLSInsecureSkipVerify field
declaration in externalmodel_types.go so the generated CRD/OpenAPI schema and
kubectl explain show the explicit default for the TLSInsecureSkipVerify field.
In `@maas-controller/pkg/reconciler/externalmodel/reconciler.go`:
- Around line 279-286: The ExternalModelSpec is correctly setting
TLSInsecureSkipVerify into spec (TLSInsecureSkipVerify:
extModel.Spec.TLSInsecureSkipVerify), but the reconcile log that reports the
external model connection info doesn't include that field; update the reconcile
logging call (the log/info call that currently prints Provider, Endpoint,
PathPrefix, Port) to also log TLSInsecureSkipVerify by adding
extModel.Spec.TLSInsecureSkipVerify or spec.TLSInsecureSkipVerify to the
structured log entry so operators can see whether insecure TLS is active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: fbfd0938-a373-49ad-9a98-c04f82bedb7e
📒 Files selected for processing (3)
deployment/base/maas-controller/crd/bases/maas.opendatahub.io_externalmodels.yamlmaas-controller/api/maas/v1alpha1/externalmodel_types.gomaas-controller/pkg/reconciler/externalmodel/reconciler.go
|
|
/retest |
|
@nerdalert: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
nerdalert
force-pushed
the
brent-insecure-tls-extmodel
branch
from
6cb4a67 to
c6e3181
Compare
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nerdalert 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nerdalert 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 |
Adds an optional spec.tlsInsecureSkipVerify field to the ExternalModel CRD. When true, the reconciler generates the DestinationRule with insecureSkipVerify: true, allowing connections to endpoints with self-signed certificates without manual patching that gets overwritten on reconciliation. Default is false. Closes opendatahub-io#627 Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
nerdalert
force-pushed
the
brent-insecure-tls-extmodel
branch
from
c6e3181 to
dcaf2ca
Compare
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)
maas-api/internal/models/discovery.go (1)
57-79:⚠️ Potential issue | 🟠 MajorRemove
InsecureSkipVerifyfrom the probe client (high, CWE-295).Any pod, proxy, or sidecar on the network path to these HTTPS probes can present an arbitrary certificate and spoof
/v1/modelsresponses. That lets an attacker make inaccessible backends look accessible during listing. Use the cluster trust bundle or an explicit CA pool instead of disabling verification.Remediation sketch
func NewManager(log *logger.Logger, accessCheckTimeoutSeconds int) (*Manager, error) { @@ httpClient: &http.Client{ Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec // cluster-internal only + TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, + // RootCAs: clusterCABundle, + }, MaxIdleConns: httpMaxIdleConns, MaxIdleConnsPerHost: maxDiscoveryConcurrency, IdleConnTimeout: httpIdleConnTimeout, }, },As per coding guidelines,
**/*.go:No InsecureSkipVerify in TLS configs (enables MITM attacks).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-api/internal/models/discovery.go` around lines 57 - 79, The TLS config for the probe HTTP client in NewManager currently sets InsecureSkipVerify: true which must be removed; update the TLSClientConfig used in the Transport so it does not disable verification and instead sets RootCAs to a proper CA pool (e.g. load x509.SystemCertPool() and augment it with the cluster trust bundle or an explicit CA file) or otherwise use the cluster trust bundle provided by your environment, ensuring the Manager.httpClient Transport uses tls.Config{RootCAs: caPool} (and no InsecureSkipVerify) so probe HTTPS certificates are properly validated.
🧹 Nitpick comments (2)
maas-api/internal/config/config.go (1)
49-53: Deduplicate the access-check default.
15is now defined here and again inmaas-api/internal/models/discovery.goasdefaultAccessCheckTimeout. If one side changes and the other does not, operators will get a timeout different from the configured/documented default. Move this into one shared constant and have bothLoad()andNewManager()consume it.Also applies to: 65-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-api/internal/config/config.go` around lines 49 - 53, The default access-check timeout value is duplicated (literal 15) between AccessCheckTimeoutSeconds in the config struct and defaultAccessCheckTimeout in models/discovery.go; create a single shared constant (e.g., DefaultAccessCheckTimeout) in the config package and replace the hard-coded 15 in both Load() (config loading/defaulting) and NewManager() (models discovery) to reference that constant so both consumers derive the same default value.maas-api/internal/config/config_test.go (1)
123-170: Add explicit failing cases forAccessCheckTimeoutSeconds.These updates only keep the happy-path table entries passing. The new
ACCESS_CHECK_TIMEOUT_SECONDS must be at least 1branch still has no direct test, so accepting0or-1would regress silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-api/internal/config/config_test.go` around lines 123 - 170, The test table in config_test.go lacks negative cases for AccessCheckTimeoutSeconds, so add explicit failing entries that set Config.AccessCheckTimeoutSeconds to 0 and to -1 and assert validation fails (matching the "ACCESS_CHECK_TIMEOUT_SECONDS must be at least 1" branch). Locate the table of test cases in the file (where Config values like DBConnectionURL, APIKeyMaxExpirationDays, and AccessCheckTimeoutSeconds are defined) and add two cases named e.g. "AccessCheckTimeoutSeconds zero invalid" and "AccessCheckTimeoutSeconds negative invalid" that expect an error from the same validation function used by other cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@deployment/base/maas-controller/crd/bases/maas.opendatahub.io_externalmodels.yaml`:
- Around line 104-115: Remove the tenant-facing spec.tlsInsecureSkipVerify
boolean from the ExternalModel CRD schema and any code that reads it (look for
the spec.tlsInsecureSkipVerify field in maas.opendatahub.io_externalmodels.yaml
and related controller logic that builds Istio DestinationRules); instead
enforce certificate verification by default and implement any necessary cluster-
or operator-level configuration for testing-only insecure TLS (e.g.,
operator-only flag or admin-only ConfigMap) so application-level CRs cannot
disable TLS validation; update schema, controller code that sets
trafficPolicy.tls.insecureSkipVerify, and related docs to reflect removal.
In `@maas-api/internal/handlers/models.go`:
- Around line 264-265: The code currently sets accessCheckedAt :=
time.Now().UTC() right before sending the response which records completion time
rather than the start of the authorization checks; change this to capture the
start timestamp (e.g., accessCheckStart := time.Now().UTC()) at the beginning of
the probe/authorization batch and use that value when writing the
"X-Access-Checked-At" header (replace uses of accessCheckedAt), and make the
same change for the other occurrence around lines handling h.maasModelRefLister
so the header reflects the batch start time instead of completion.
In `@maas-controller/pkg/reconciler/externalmodel/reconciler.go`:
- Around line 287-293: The reconciler currently propagates the user-controlled
extModel.Spec.TLSInsecureSkipVerify into the constructed config's
TLSInsecureSkipVerify field, allowing TLS cert validation to be disabled; update
the code in reconciler.go so TLSInsecureSkipVerify is never set from untrusted
user input (extModel.Spec.TLSInsecureSkipVerify). Instead, always set
TLSInsecureSkipVerify = false in the created spec, or enforce a safe server-side
policy/feature-gate that requires explicit operator approval before allowing
true; reference the TLSInsecureSkipVerify field and
extModel.Spec.TLSInsecureSkipVerify when making the change.
---
Outside diff comments:
In `@maas-api/internal/models/discovery.go`:
- Around line 57-79: The TLS config for the probe HTTP client in NewManager
currently sets InsecureSkipVerify: true which must be removed; update the
TLSClientConfig used in the Transport so it does not disable verification and
instead sets RootCAs to a proper CA pool (e.g. load x509.SystemCertPool() and
augment it with the cluster trust bundle or an explicit CA file) or otherwise
use the cluster trust bundle provided by your environment, ensuring the
Manager.httpClient Transport uses tls.Config{RootCAs: caPool} (and no
InsecureSkipVerify) so probe HTTPS certificates are properly validated.
---
Nitpick comments:
In `@maas-api/internal/config/config_test.go`:
- Around line 123-170: The test table in config_test.go lacks negative cases for
AccessCheckTimeoutSeconds, so add explicit failing entries that set
Config.AccessCheckTimeoutSeconds to 0 and to -1 and assert validation fails
(matching the "ACCESS_CHECK_TIMEOUT_SECONDS must be at least 1" branch). Locate
the table of test cases in the file (where Config values like DBConnectionURL,
APIKeyMaxExpirationDays, and AccessCheckTimeoutSeconds are defined) and add two
cases named e.g. "AccessCheckTimeoutSeconds zero invalid" and
"AccessCheckTimeoutSeconds negative invalid" that expect an error from the same
validation function used by other cases.
In `@maas-api/internal/config/config.go`:
- Around line 49-53: The default access-check timeout value is duplicated
(literal 15) between AccessCheckTimeoutSeconds in the config struct and
defaultAccessCheckTimeout in models/discovery.go; create a single shared
constant (e.g., DefaultAccessCheckTimeout) in the config package and replace the
hard-coded 15 in both Load() (config loading/defaulting) and NewManager()
(models discovery) to reference that constant so both consumers derive the same
default value.
🪄 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: Pro
Run ID: b6adf5b2-bcf8-49a5-9b5d-93ec96c23912
📒 Files selected for processing (9)
deployment/base/maas-controller/crd/bases/maas.opendatahub.io_externalmodels.yamlmaas-api/cmd/main.gomaas-api/internal/config/config.gomaas-api/internal/config/config_test.gomaas-api/internal/handlers/models.gomaas-api/internal/handlers/models_test.gomaas-api/internal/models/discovery.gomaas-controller/api/maas/v1alpha1/externalmodel_types.gomaas-controller/pkg/reconciler/externalmodel/reconciler.go
🚧 Files skipped from review as they are similar to previous changes (1)
- maas-controller/api/maas/v1alpha1/externalmodel_types.go
| tlsInsecureSkipVerify: | ||
| description: |- | ||
| TLSInsecureSkipVerify disables TLS certificate verification on the | ||
| DestinationRule created for this external model. When true, the generated | ||
| Istio DestinationRule includes insecureSkipVerify: true under | ||
| trafficPolicy.tls, allowing connections to endpoints with self-signed or | ||
| untrusted certificates (e.g., simulators, dev environments). | ||
|
|
||
| WARNING: Do not enable in production. This is intended for testing only. | ||
|
|
||
| Default: false (certificates are verified). | ||
| type: boolean |
There was a problem hiding this comment.
Remove spec.tlsInsecureSkipVerify from the CRD (Critical, CWE-295).
This exposes a tenant-facing switch for disabling TLS certificate validation; attackers with network position can MITM egress traffic when enabled.
Remediation patch
- tlsInsecureSkipVerify:
- description: |-
- TLSInsecureSkipVerify disables TLS certificate verification on the
- DestinationRule created for this external model. When true, the generated
- Istio DestinationRule includes insecureSkipVerify: true under
- trafficPolicy.tls, allowing connections to endpoints with self-signed or
- untrusted certificates (e.g., simulators, dev environments).
-
- WARNING: Do not enable in production. This is intended for testing only.
-
- Default: false (certificates are verified).
- type: booleanAs per coding guidelines, REVIEW PRIORITIES: 1. Security vulnerabilities (provide severity, exploit scenario, and remediation code).
📝 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.
| tlsInsecureSkipVerify: | |
| description: |- | |
| TLSInsecureSkipVerify disables TLS certificate verification on the | |
| DestinationRule created for this external model. When true, the generated | |
| Istio DestinationRule includes insecureSkipVerify: true under | |
| trafficPolicy.tls, allowing connections to endpoints with self-signed or | |
| untrusted certificates (e.g., simulators, dev environments). | |
| WARNING: Do not enable in production. This is intended for testing only. | |
| Default: false (certificates are verified). | |
| type: boolean |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@deployment/base/maas-controller/crd/bases/maas.opendatahub.io_externalmodels.yaml`
around lines 104 - 115, Remove the tenant-facing spec.tlsInsecureSkipVerify
boolean from the ExternalModel CRD schema and any code that reads it (look for
the spec.tlsInsecureSkipVerify field in maas.opendatahub.io_externalmodels.yaml
and related controller logic that builds Istio DestinationRules); instead
enforce certificate verification by default and implement any necessary cluster-
or operator-level configuration for testing-only insecure TLS (e.g.,
operator-only flag or admin-only ConfigMap) so application-level CRs cannot
disable TLS validation; update schema, controller code that sets
trafficPolicy.tls.insecureSkipVerify, and related docs to reflect removal.
| accessCheckedAt := time.Now().UTC() | ||
| if h.maasModelRefLister != nil { |
There was a problem hiding this comment.
X-Access-Checked-At currently reports completion time, not check time.
For a slow probe batch, the earliest authorization decision may already be several seconds old when this header is emitted. If clients use it to assess freshness, keep the initial timestamp and return the batch start time, or rename the header to reflect completion instead.
Minimal fix
modelList := []models.Model{}
accessCheckedAt := time.Now().UTC()
@@
- accessCheckedAt = time.Now().UTC()
h.logger.Debug("Access validation complete", "listed", len(list), "accessible", len(modelList), "subscriptions", len(subscriptionsToUse))
} else {Also applies to: 366-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@maas-api/internal/handlers/models.go` around lines 264 - 265, The code
currently sets accessCheckedAt := time.Now().UTC() right before sending the
response which records completion time rather than the start of the
authorization checks; change this to capture the start timestamp (e.g.,
accessCheckStart := time.Now().UTC()) at the beginning of the
probe/authorization batch and use that value when writing the
"X-Access-Checked-At" header (replace uses of accessCheckedAt), and make the
same change for the other occurrence around lines handling h.maasModelRefLister
so the header reflects the batch start time instead of completion.
| Provider: extModel.Spec.Provider, | ||
| Endpoint: extModel.Spec.Endpoint, | ||
| PathPrefix: ann[AnnPathPrefix], | ||
| TLS: true, | ||
| Port: 443, | ||
| TLSInsecureSkipVerify: extModel.Spec.TLSInsecureSkipVerify, | ||
| } |
There was a problem hiding this comment.
Block certificate-validation bypass in reconciled spec (Critical, CWE-295).
Line 292 propagates a user-controlled flag that disables upstream TLS cert verification; an on-path attacker can impersonate the external endpoint and intercept prompts/credentials.
Remediation patch
spec := ExternalModelSpec{
Provider: extModel.Spec.Provider,
Endpoint: extModel.Spec.Endpoint,
PathPrefix: ann[AnnPathPrefix],
TLS: true,
Port: 443,
- TLSInsecureSkipVerify: extModel.Spec.TLSInsecureSkipVerify,
+ TLSInsecureSkipVerify: false,
}
+
+if extModel.Spec.TLSInsecureSkipVerify {
+ return spec, fmt.Errorf("spec.tlsInsecureSkipVerify is not allowed")
+}As per coding guidelines, No InsecureSkipVerify in TLS configs (enables MITM attacks).
📝 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.
| Provider: extModel.Spec.Provider, | |
| Endpoint: extModel.Spec.Endpoint, | |
| PathPrefix: ann[AnnPathPrefix], | |
| TLS: true, | |
| Port: 443, | |
| TLSInsecureSkipVerify: extModel.Spec.TLSInsecureSkipVerify, | |
| } | |
| spec := ExternalModelSpec{ | |
| Provider: extModel.Spec.Provider, | |
| Endpoint: extModel.Spec.Endpoint, | |
| PathPrefix: ann[AnnPathPrefix], | |
| TLS: true, | |
| Port: 443, | |
| TLSInsecureSkipVerify: false, | |
| } | |
| if extModel.Spec.TLSInsecureSkipVerify { | |
| return spec, fmt.Errorf("spec.tlsInsecureSkipVerify is not allowed") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@maas-controller/pkg/reconciler/externalmodel/reconciler.go` around lines 287
- 293, The reconciler currently propagates the user-controlled
extModel.Spec.TLSInsecureSkipVerify into the constructed config's
TLSInsecureSkipVerify field, allowing TLS cert validation to be disabled; update
the code in reconciler.go so TLSInsecureSkipVerify is never set from untrusted
user input (extModel.Spec.TLSInsecureSkipVerify). Instead, always set
TLSInsecureSkipVerify = false in the created spec, or enforce a safe server-side
policy/feature-gate that requires explicit operator approval before allowing
true; reference the TLSInsecureSkipVerify field and
extModel.Spec.TLSInsecureSkipVerify when making the change.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
maas-api/internal/models/discovery.go (1)
68-77:⚠️ Potential issue | 🟠 MajorRemove
InsecureSkipVerifyfrom the probe client (CWE-295).These probes forward the caller's
Authorizationheader. Although filtering is restricted to cluster-internal models (ExternalModels are skipped), disabling certificate verification enables MITM attacks from compromised pods or network paths within the cluster to steal tokens or spoof access decisions. Use the cluster CA instead.The suggested fix requires loading the cluster's root CA bundle, not the system CA pool alone. For in-cluster services, use
x509.SystemCertPool()as a base, then append the cluster CA from the mounted service-account token or explicit bundle:rootCAs, err := x509.SystemCertPool() if err != nil { return nil, fmt.Errorf("load system cert pool: %w", err) } // Load cluster CA from /var/run/secrets/kubernetes.io/serviceaccount/ca.crt clusterCA, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/ca.crt") if err != nil { return nil, fmt.Errorf("load cluster CA: %w", err) } if !rootCAs.AppendCertsFromPEM(clusterCA) { return nil, errors.New("cluster CA certificate failed to parse") }Then set
TLSClientConfig: &tls.Config{RootCAs: rootCAs, MinVersion: tls.VersionTLS12}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-api/internal/models/discovery.go` around lines 68 - 77, Remove the insecure skip-verify flag on the probe HTTP client and instead build a proper RootCAs pool: in the code that constructs httpClient (the http.Client with Transport: &http.Transport{...} and TLSClientConfig: &tls.Config{...}), replace InsecureSkipVerify: true with TLS config that sets RootCAs to x509.SystemCertPool() with the cluster CA appended from /var/run/secrets/kubernetes.io/serviceaccount/ca.crt, return/propagate errors if loading the system pool or reading/parsing the cluster CA fails, and set MinVersion to tls.VersionTLS12; keep the other transport fields (MaxIdleConns, MaxIdleConnsPerHost, IdleConnTimeout) and ensure this change happens where the probe client is initialized so callers (e.g., access check logic using accessCheckTimeout and maxDiscoveryConcurrency) receive a properly verified client.
♻️ Duplicate comments (2)
deployment/base/maas-controller/crd/bases/maas.opendatahub.io_externalmodels.yaml (1)
104-115:⚠️ Potential issue | 🔴 CriticalRemove tenant-exposed
spec.tlsInsecureSkipVerifyfrom CRD (Critical, CWE-295).This schema field enables certificate-validation bypass from a namespaced CR.
Exploit scenario: a compromised tenant workload or malicious user can configure traffic to skip TLS verification and transparently MITM external model connections.Remediation patch
- tlsInsecureSkipVerify: - description: |- - TLSInsecureSkipVerify disables TLS certificate verification on the - DestinationRule created for this external model. When true, the generated - Istio DestinationRule includes insecureSkipVerify: true under - trafficPolicy.tls, allowing connections to endpoints with self-signed or - untrusted certificates (e.g., simulators, dev environments). - - WARNING: Do not enable in production. This is intended for testing only. - - Default: false (certificates are verified). - type: booleanAs per coding guidelines,
REVIEW PRIORITIES: 1. Security vulnerabilities (provide severity, exploit scenario, and remediation code).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployment/base/maas-controller/crd/bases/maas.opendatahub.io_externalmodels.yaml` around lines 104 - 115, The CRD exposes a tenant-configurable boolean tlsInsecureSkipVerify (spec.tlsInsecureSkipVerify) which allows disabling TLS certificate verification in the generated DestinationRule (trafficPolicy.tls.insecureSkipVerify) — remove this schema field so tenants cannot opt out of cert validation; update the CRD by deleting the tlsInsecureSkipVerify property and any references to it (and ensure code that reads spec.tlsInsecureSkipVerify is hardened/removed from the controller that creates DestinationRule objects so insecureSkipVerify is never set from CR input).maas-controller/pkg/reconciler/externalmodel/reconciler.go (1)
287-293:⚠️ Potential issue | 🔴 CriticalBlock tenant-controlled TLS verification bypass (Critical, CWE-295).
Line 292 propagates a user-controlled flag into transport policy, which allows
DestinationRule.trafficPolicy.tls.insecureSkipVerify=true.
Exploit scenario: any on-path attacker (cluster network, egress gateway path, DNS poisoning + rogue cert) can impersonate the upstream endpoint and intercept/alter prompts, responses, or credentials.Remediation patch
spec := ExternalModelSpec{ Provider: extModel.Spec.Provider, Endpoint: extModel.Spec.Endpoint, PathPrefix: ann[AnnPathPrefix], TLS: true, Port: 443, - TLSInsecureSkipVerify: extModel.Spec.TLSInsecureSkipVerify, + TLSInsecureSkipVerify: false, } + +if extModel.Spec.TLSInsecureSkipVerify { + return spec, fmt.Errorf("spec.tlsInsecureSkipVerify is not allowed") +}As per coding guidelines,
No InsecureSkipVerify in TLS configs (enables MITM attacks).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@maas-controller/pkg/reconciler/externalmodel/reconciler.go` around lines 287 - 293, The code currently propagates the tenant-controlled flag extModel.Spec.TLSInsecureSkipVerify into the built TLS config (the TLSInsecureSkipVerify field in the struct created in reconciler.go), enabling insecureSkipVerify; change this so that TLSInsecureSkipVerify is never set from tenant input (always set to false) and instead log or record any tenant attempts to enable it (or reject the resource) to an audit/event; update the code that builds the DestinationRule/transport policy in reconciler.go to ignore extModel.Spec.TLSInsecureSkipVerify and ensure TLSInsecureSkipVerify = false and add a validation/audit path to surface the denied override.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@maas-api/internal/models/discovery.go`:
- Around line 68-77: Remove the insecure skip-verify flag on the probe HTTP
client and instead build a proper RootCAs pool: in the code that constructs
httpClient (the http.Client with Transport: &http.Transport{...} and
TLSClientConfig: &tls.Config{...}), replace InsecureSkipVerify: true with TLS
config that sets RootCAs to x509.SystemCertPool() with the cluster CA appended
from /var/run/secrets/kubernetes.io/serviceaccount/ca.crt, return/propagate
errors if loading the system pool or reading/parsing the cluster CA fails, and
set MinVersion to tls.VersionTLS12; keep the other transport fields
(MaxIdleConns, MaxIdleConnsPerHost, IdleConnTimeout) and ensure this change
happens where the probe client is initialized so callers (e.g., access check
logic using accessCheckTimeout and maxDiscoveryConcurrency) receive a properly
verified client.
---
Duplicate comments:
In
`@deployment/base/maas-controller/crd/bases/maas.opendatahub.io_externalmodels.yaml`:
- Around line 104-115: The CRD exposes a tenant-configurable boolean
tlsInsecureSkipVerify (spec.tlsInsecureSkipVerify) which allows disabling TLS
certificate verification in the generated DestinationRule
(trafficPolicy.tls.insecureSkipVerify) — remove this schema field so tenants
cannot opt out of cert validation; update the CRD by deleting the
tlsInsecureSkipVerify property and any references to it (and ensure code that
reads spec.tlsInsecureSkipVerify is hardened/removed from the controller that
creates DestinationRule objects so insecureSkipVerify is never set from CR
input).
In `@maas-controller/pkg/reconciler/externalmodel/reconciler.go`:
- Around line 287-293: The code currently propagates the tenant-controlled flag
extModel.Spec.TLSInsecureSkipVerify into the built TLS config (the
TLSInsecureSkipVerify field in the struct created in reconciler.go), enabling
insecureSkipVerify; change this so that TLSInsecureSkipVerify is never set from
tenant input (always set to false) and instead log or record any tenant attempts
to enable it (or reject the resource) to an audit/event; update the code that
builds the DestinationRule/transport policy in reconciler.go to ignore
extModel.Spec.TLSInsecureSkipVerify and ensure TLSInsecureSkipVerify = false and
add a validation/audit path to surface the denied override.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8f55beb7-f24a-4442-b94a-9b80175228ce
📒 Files selected for processing (9)
deployment/base/maas-controller/crd/bases/maas.opendatahub.io_externalmodels.yamlmaas-api/cmd/main.gomaas-api/internal/config/config.gomaas-api/internal/config/config_test.gomaas-api/internal/handlers/models.gomaas-api/internal/handlers/models_test.gomaas-api/internal/models/discovery.gomaas-controller/api/maas/v1alpha1/externalmodel_types.gomaas-controller/pkg/reconciler/externalmodel/reconciler.go
🚧 Files skipped from review as they are similar to previous changes (6)
- maas-api/cmd/main.go
- maas-api/internal/config/config.go
- maas-api/internal/config/config_test.go
- maas-controller/api/maas/v1alpha1/externalmodel_types.go
- maas-api/internal/handlers/models_test.go
- maas-api/internal/handlers/models.go
|
PR needs rebase. DetailsInstructions 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 kubernetes-sigs/prow repository. |
3 participants
Adds an optional
spec.tlsInsecureSkipVerifyfield to the ExternalModel CRD. When true, the reconciler generates the DestinationRule withinsecureSkipVerify: true, allowing connections to endpoints with self-signed certificates without manual patching that gets overwritten on reconciliation. Default is false.Closes #627
Leaving in draft until I've validated on a live cluster. cc/ @noyitz
ty!
Summary by CodeRabbit
New Features
Behavior Changes