ci: add tls compliance scan#2564
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: guidonguido 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds OpenShift TLS profile discovery, dynamic TLS configuration for operators, rendered workloads, and proxies, profile-change-triggered manager shutdown, TLS-focused linting, RBAC access, dependency updates, and tests. ChangesTLS security integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant TLSHelpers
participant APIServer
participant SecurityProfileWatcher
participant Manager
Operator->>TLSHelpers: resolve TLS profile and TLS config
TLSHelpers->>APIServer: read APIServer TLS settings
APIServer-->>TLSHelpers: return profile data
TLSHelpers-->>Operator: provide TLS configuration
Operator->>Manager: start metrics and webhook servers with TLS
SecurityProfileWatcher->>APIServer: watch TLS profile
APIServer-->>SecurityProfileWatcher: report profile change
SecurityProfileWatcher->>Manager: cancel context
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
guidonguido
force-pushed
the
ci/tls-profile-semgrep
branch
from
0dd825a to
a5e414c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
operators/pkg/util/client.go (1)
132-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth logging and returning the same error.
Both
log.Error(...)+return nil, errsites here handle the error twice — once via log, once via return — leaving the caller to potentially log it again. Only the scheme-registration error at line 139-141 follows the intended pattern (wrap-and-return, no log).As per coding guidelines, "Wrap errors with
fmt.Errorf(\"...: %w\", err)and handle each error once—either log it or return it, but not both."♻️ Proposed fix
config, err := clientcmd.BuildConfigFromFlags("", "") if err != nil { - log.Error(err, ConfigErrorMessage) - return nil, err + return nil, fmt.Errorf("%s: %w", ConfigErrorMessage, err) } ... ocpConfigClient, err = client.New(config, client.Options{Scheme: scheme}) if err != nil { - log.Error(err, "Failed to create OCP config controller-runtime client") - return nil, err + return nil, fmt.Errorf("failed to create OCP config controller-runtime client: %w", err) }Also applies to: 143-147
🤖 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 `@operators/pkg/util/client.go` around lines 132 - 136, Remove the redundant logging from the error paths in the client configuration and related setup code, including the sites around client-go scheme registration. Wrap each underlying error with fmt.Errorf using %w and return it directly, matching the existing scheme-registration pattern; ensure each error is handled only once.Source: Coding guidelines
operators/multiclusterobservability/pkg/rendering/renderer_proxy.go (1)
86-87: 🔒 Security & Privacy | 🔵 TrivialResidual TLS-profile gap on the oauth-proxy container — tracked, but worth flagging.
The TODO correctly documents that oauth-proxy doesn't support these flags, so nothing to fix in this diff. But this means the cluster TLS security profile enforcement this PR is adding doesn't cover the oauth-proxy sidecar's TLS termination at all — worth a tracked follow-up (or confirming oauth-proxy has some other TLS config knob, e.g. via
--tls-cert/env, before calling this closed).🤖 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 `@operators/multiclusterobservability/pkg/rendering/renderer_proxy.go` around lines 86 - 87, Track a follow-up for the oauth-proxy TLS termination gap in the renderer proxy configuration. Confirm whether oauth-proxy supports an alternative TLS security configuration mechanism, such as certificate or environment-based settings; otherwise document that util.SetTLSSecurityConfiguration is intentionally not applied to the oauth-proxy container.operators/pkg/util/tls.go (1)
37-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDouble error handling: logged and returned.
Same pattern as flagged in
client.go:log.Error(...)followed byreturn nil, errat both sites. Per coding guidelines, an error should be logged or returned, not both.As per coding guidelines, "Wrap errors with
fmt.Errorf(\"...: %w\", err)and handle each error once—either log it or return it, but not both."Also applies to: 103-104
🤖 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 `@operators/pkg/util/tls.go` around lines 37 - 41, Remove the log.Error calls for failures from tlsClientFunc and the other indicated site, returning wrapped errors instead (for example via fmt.Errorf with %w) so each error is handled only once; update both error paths in the relevant TLS client helper.Source: Coding guidelines
operators/pkg/util/tls_integration_test.go (1)
100-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLocal
tlsProfileSpecandtlsConfigshadow package-level cached vars.The local variables shadow the package-level
tlsProfileSpecandtlsConfigthatGetOrCreateTLSProfileSpec/GetOrCreateTLSConfigread and write. This is functionally correct but could confuse a future reader who assumes the local and package-level vars are the same. Consider renaming toinitialSpec/initialTLSConfigfor clarity.🤖 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 `@operators/pkg/util/tls_integration_test.go` around lines 100 - 103, Rename the local variables in the test from tlsProfileSpec and tlsConfig to initialSpec and initialTLSConfig to distinguish them from the package-level cached variables used by GetOrCreateTLSProfileSpec and GetOrCreateTLSConfig, updating all references in that scope.operators/multiclusterobservability/pkg/rendering/renderer_alertmanager.go (1)
142-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCommented-out oauth-proxy line would not compile if uncommented.
SetTLSSecurityConfigurationreturns([]string, error), but the commented-out line only assigns one return value. A future developer uncommenting this would hit a compile error and need to reconstruct the full error-handling pattern. Consider either showing the correct full pattern in the comment or removing the line entirely—the TODO already explains the rationale.♻️ Proposed fix: show correct pattern or remove
Option A — show the correct pattern:
- // TODO(guidonguido): oauth-proxy upstream doesn't support --tls-cipher-suites/--tls-min-version flags - // oauthProxyContainer.Args = util.SetTLSSecurityConfiguration(ctx, oauthProxyContainer.Args, "--tls-cipher-suites=", "--tls-min-version=") + // TODO(guidonguido): oauth-proxy upstream doesn't support --tls-cipher-suites/--tls-min-version flags + // args, err := util.SetTLSSecurityConfiguration(ctx, oauthProxyContainer.Args, "--tls-cipher-suites=", "--tls-min-version=") + // if err != nil { + // return nil, err + // } + // oauthProxyContainer.Args = argsOption B — remove the line and keep only the TODO:
// TODO(guidonguido): oauth-proxy upstream doesn't support --tls-cipher-suites/--tls-min-version flags - // oauthProxyContainer.Args = util.SetTLSSecurityConfiguration(ctx, oauthProxyContainer.Args, "--tls-cipher-suites=", "--tls-min-version=")🤖 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 `@operators/multiclusterobservability/pkg/rendering/renderer_alertmanager.go` around lines 142 - 143, Update the TODO near SetTLSSecurityConfiguration to avoid retaining a commented-out call with an invalid single-return assignment: either remove the commented line, or document the complete error-handling pattern that assigns both returned values and handles the error before updating oauthProxyContainer.Args.
🤖 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 @.github/workflows/lint.yaml:
- Around line 41-45: Pin the Semgrep package to an explicit version in the
“Install Semgrep” step and replace the default-branch clone in “Clone
argus-observe-rules” with a checkout of a specific commit SHA, ensuring both the
scanner and ruleset remain reproducible.
In `@collectors/metrics/pkg/forwarder/forwarder.go`:
- Around line 139-144: Remove the InsecureSkipVerify fallback from both
forwarder client TLS configuration blocks around the transport setup, including
the corresponding paths near the second referenced location. Use system
certificate roots by leaving RootCAs unset or configure a provided trusted CA
bundle, while retaining an appropriate minimum TLS version and updating related
comments or suppression annotations.
In `@operators/pkg/util/client.go`:
- Around line 31-35: Protect the memoized ocpConfigClient initialization in the
relevant client getter with sync.Once or equivalent synchronization so
concurrent TLS rendering workers cannot race. Preserve the initialized client
for subsequent calls, and wrap initialization errors with context while
returning them directly instead of logging and repeatedly returning the same
failure.
In `@operators/pkg/util/tls_integration_test.go`:
- Around line 119-153: Protect the shared changes slice with a sync.Mutex in the
test. Lock around the append in OnProfileChange, and lock when reading its
length during PollUntilContextTimeout and when accessing post-poll entries such
as changes[0]; remove the obsolete TODO acknowledging the race.
In `@operators/pkg/util/tls.go`:
- Around line 96-105: Remove the UNIT_TEST environment-variable bypass from
SetTLSSecurityConfiguration so GetOrCreateTLSProfileSpec errors always log and
return the error. Provide an exported test-injection hook or setter around the
existing tlsClientFunc seam, update renderer_test.go and
renderer_alertmanager_test.go to use it, and eliminate their reliance on
UNIT_TEST for simulating failures.
- Around line 20-27: Protect the memoized package variables tlsProfileSpec and
tlsConfig with the same synchronization approach used to fix client.go’s
ocpConfigClient. Update the functions accessing these values, including the code
around lines 33–35 and 63–71, to use sync.Once or an appropriate mutex so
concurrent calls cannot race during initialization; preserve tlsClientFunc’s
test injection behavior.
- Around line 78-94: The TLS cache assignment in GetOrCreateTLSConfig is
shadowing the package-level tlsConfig variable, so memoization never persists.
Replace the short declaration with an assignment and separately declare
unsupportedCiphers as needed, ensuring the returned configuration is stored in
the shared tlsConfig cache.
---
Nitpick comments:
In `@operators/multiclusterobservability/pkg/rendering/renderer_alertmanager.go`:
- Around line 142-143: Update the TODO near SetTLSSecurityConfiguration to avoid
retaining a commented-out call with an invalid single-return assignment: either
remove the commented line, or document the complete error-handling pattern that
assigns both returned values and handles the error before updating
oauthProxyContainer.Args.
In `@operators/multiclusterobservability/pkg/rendering/renderer_proxy.go`:
- Around line 86-87: Track a follow-up for the oauth-proxy TLS termination gap
in the renderer proxy configuration. Confirm whether oauth-proxy supports an
alternative TLS security configuration mechanism, such as certificate or
environment-based settings; otherwise document that
util.SetTLSSecurityConfiguration is intentionally not applied to the oauth-proxy
container.
In `@operators/pkg/util/client.go`:
- Around line 132-136: Remove the redundant logging from the error paths in the
client configuration and related setup code, including the sites around
client-go scheme registration. Wrap each underlying error with fmt.Errorf using
%w and return it directly, matching the existing scheme-registration pattern;
ensure each error is handled only once.
In `@operators/pkg/util/tls_integration_test.go`:
- Around line 100-103: Rename the local variables in the test from
tlsProfileSpec and tlsConfig to initialSpec and initialTLSConfig to distinguish
them from the package-level cached variables used by GetOrCreateTLSProfileSpec
and GetOrCreateTLSConfig, updating all references in that scope.
In `@operators/pkg/util/tls.go`:
- Around line 37-41: Remove the log.Error calls for failures from tlsClientFunc
and the other indicated site, returning wrapped errors instead (for example via
fmt.Errorf with %w) so each error is handled only once; update both error paths
in the relevant TLS client helper.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b363eb00-657b-46ab-b20c-96d7ec1b7b35
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumoperators/multiclusterobservability/controllers/multiclusterobservability/testdata/crd/config.openshift.io_apiservers.crd.yamlis excluded by!**/testdata/**operators/pkg/util/testdata/crd/config.openshift.io_apiservers.crd.yamlis excluded by!**/testdata/**
📒 Files selected for processing (24)
.github/workflows/lint.yamlcollectors/metrics/pkg/forwarder/forwarder.gocollectors/metrics/pkg/metricsclient/metricsclient.gogo.modoperators/endpointmetrics/config/rbac/emo_role.yamloperators/endpointmetrics/main.gooperators/endpointmetrics/manifests/prometheus/kube-state-metrics-deployment.yamloperators/endpointmetrics/manifests/prometheus/node-exporter-daemonset.yamloperators/endpointmetrics/manifests/prometheus/prometheus-resource.yamloperators/endpointmetrics/pkg/rendering/renderer.gooperators/endpointmetrics/pkg/rendering/renderer_test.gooperators/multiclusterobservability/main.gooperators/multiclusterobservability/manifests/endpoint-observability/role.yamloperators/multiclusterobservability/pkg/rendering/renderer_alertmanager.gooperators/multiclusterobservability/pkg/rendering/renderer_alertmanager_test.gooperators/multiclusterobservability/pkg/rendering/renderer_proxy.gooperators/multiclusterobservability/pkg/rendering/renderer_test.gooperators/pkg/config/config.gooperators/pkg/util/client.gooperators/pkg/util/tls.gooperators/pkg/util/tls_integration_test.gooperators/pkg/util/tls_test.goproxy/cmd/main.goproxy/pkg/proxy/tls.go
💤 Files with no reviewable changes (3)
- operators/endpointmetrics/manifests/prometheus/prometheus-resource.yaml
- operators/endpointmetrics/manifests/prometheus/node-exporter-daemonset.yaml
- operators/endpointmetrics/manifests/prometheus/kube-state-metrics-deployment.yaml
| // nosemgrep: go-tls-bypass - client-side TLS fallback when no TLS config is provided. | ||
| fromTransport.TLSClientConfig = &tls.Config{ | ||
| // nosemgrep: go-crypto-tls-version - client-side TLS. | ||
| MinVersion: tls.VersionTLS12, | ||
| InsecureSkipVerify: true, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,260p' collectors/metrics/pkg/forwarder/forwarder.goRepository: stolostron/multicluster-observability-operator
Length of output: 9608
🏁 Script executed:
ast-grep outline collectors/metrics/pkg/forwarder/forwarder.go --view expandedRepository: stolostron/multicluster-observability-operator
Length of output: 2874
🏁 Script executed:
rg -n "InsecureSkipVerify|nosemgrep|nosec|tls.Config|CA|certificate" collectors/metrics/pkg/forwarder/forwarder.goRepository: stolostron/multicluster-observability-operator
Length of output: 1532
🏁 Script executed:
wc -l collectors/metrics/pkg/forwarder/forwarder.go && sed -n '1,240p' collectors/metrics/pkg/forwarder/forwarder.goRepository: stolostron/multicluster-observability-operator
Length of output: 8979
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('collectors/metrics/pkg/forwarder/forwarder.go')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 'InsecureSkipVerify' in line or 'nosemgrep' in line or 'nosec' in line or 'TLSClientConfig' in line or 'CAFile' in line or 'RootCAs' in line:
print(f"{i}: {line}")
PYRepository: stolostron/multicluster-observability-operator
Length of output: 1563
🏁 Script executed:
rg -n "forwarder\.go|tls-insecure-skip-verify-go|nosemgrep|no[[:space:]]*sec|InsecureSkipVerify|RootCAs|CAFile" -S .Repository: stolostron/multicluster-observability-operator
Length of output: 12769
🏁 Script executed:
sed -n '120,220p' collectors/metrics/pkg/forwarder/forwarder.goRepository: stolostron/multicluster-observability-operator
Length of output: 3961
🏁 Script executed:
rg -n "tls-insecure-skip-verify-go|go-tls-bypass|nosemgrep|G402" collectors/metrics/pkg/forwarder/forwarder.go collectors -SRepository: stolostron/multicluster-observability-operator
Length of output: 2370
🏁 Script executed:
nl -ba collectors/metrics/pkg/forwarder/forwarder.go | sed -n '130,205p'Repository: stolostron/multicluster-observability-operator
Length of output: 220
Remove the insecure TLS fallback in collectors/metrics/pkg/forwarder/forwarder.go:139-144,190-195.
The no-CA path still sets InsecureSkipVerify: true, which disables certificate-chain and hostname validation for both forwarder clients. Use system roots by default or require a trusted CA bundle instead; this keeps the TLS chain anchored instead of bolting the door open.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 139-143: TLS certificate verification is disabled by setting InsecureSkipVerify: true on the tls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. Remove InsecureSkipVerify (or set it to false) and provide a proper RootCAs pool to trust custom certificates instead.
Context: tls.Config{
// nosemgrep: go-crypto-tls-version - client-side TLS.
MinVersion: tls.VersionTLS12,
InsecureSkipVerify: true,
}
Note: [CWE-295] Improper Certificate Validation.
(tls-insecure-skip-verify-go)
🪛 OpenGrep (1.23.0)
[ERROR] 140-144: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 140-144: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
🤖 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 `@collectors/metrics/pkg/forwarder/forwarder.go` around lines 139 - 144, Remove
the InsecureSkipVerify fallback from both forwarder client TLS configuration
blocks around the transport setup, including the corresponding paths near the
second referenced location. Use system certificate roots by leaving RootCAs
unset or configure a provided trusted CA bundle, while retaining an appropriate
minimum TLS version and updating related comments or suppression annotations.
Sources: Path instructions, MCP tools, Linters/SAST tools
| kubeClient kubernetes.Interface | ||
| crdClient crdClientSet.Interface | ||
| ocpClient ocpClientSet.Interface | ||
| promClient promClientSet.Interface | ||
| ocpConfigClient client.Client |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files operators/pkg/util/client.go operators/pkg/util/tls.go
echo '--- client.go ---'
cat -n operators/pkg/util/client.go | sed -n '1,240p'
echo '--- tls.go ---'
cat -n operators/pkg/util/tls.go | sed -n '1,220p'Repository: stolostron/multicluster-observability-operator
Length of output: 13659
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "GetOrCreateOCPConfigCRClient|GetOrCreateTLSProfileSpec|GetOrCreateTLSConfig|SetTLSSecurityConfiguration|tlsClientFunc" operatorsRepository: stolostron/multicluster-observability-operator
Length of output: 5805
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "MaxConcurrentReconciles|controller-runtime|SetupWithManager|Reconcile\\(" operatorsRepository: stolostron/multicluster-observability-operator
Length of output: 50402
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,410p' operators/multiclusterobservability/main.go | cat -nRepository: stolostron/multicluster-observability-operator
Length of output: 4647
Guard the cached OCP config client
operators/pkg/util/client.go:127-149 memoizes ocpConfigClient without any synchronization. This path is reachable from the TLS rendering flow used by controllers, so concurrent workers can race on the nil check and initialization. Use sync.Once or another guarded init, and return wrapped errors instead of logging and returning the same failure twice.
🤖 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 `@operators/pkg/util/client.go` around lines 31 - 35, Protect the memoized
ocpConfigClient initialization in the relevant client getter with sync.Once or
equivalent synchronization so concurrent TLS rendering workers cannot race.
Preserve the initialized client for subsequent calls, and wrap initialization
errors with context while returning them directly instead of logging and
repeatedly returning the same failure.
Source: Path instructions
| // TODO(guidonguido): access to changes is eventually consistent. Both controller manager | ||
| // and test goroutine are accessing it. Consider turning it atomic. | ||
| var changes []profileChange | ||
|
|
||
| watcher := &tlsutil.SecurityProfileWatcher{ | ||
| Client: mgr.GetClient(), | ||
| InitialTLSProfileSpec: *tlsProfileSpec, | ||
| OnProfileChange: func(ctx context.Context, oldSpec, newSpec configv1.TLSProfileSpec) { | ||
| changes = append(changes, profileChange{old: oldSpec, new: newSpec}) | ||
| }, | ||
| } | ||
| require.NoError(t, watcher.SetupWithManager(mgr)) | ||
|
|
||
| mgrCtx, mgrCancel := context.WithCancel(context.Background()) | ||
| defer mgrCancel() | ||
|
|
||
| go func() { | ||
| if err := mgr.Start(mgrCtx); err != nil { | ||
| t.Logf("manager stopped: %v", err) | ||
| } | ||
| }() | ||
|
|
||
| require.True(t, mgr.GetCache().WaitForCacheSync(mgrCtx)) | ||
|
|
||
| // Change profile from Intermediate to Modern | ||
| require.NoError(t, k8sClient.Get(context.Background(), client.ObjectKeyFromObject(apiServer), apiServer)) | ||
| apiServer.Spec.TLSSecurityProfile = &configv1.TLSSecurityProfile{ | ||
| Type: configv1.TLSProfileModernType, | ||
| } | ||
| require.NoError(t, k8sClient.Update(context.Background(), apiServer)) | ||
|
|
||
| err = wait.PollUntilContextTimeout(context.Background(), 200*time.Millisecond, 10*time.Second, true, func(ctx context.Context) (bool, error) { | ||
| return len(changes) >= 1, nil | ||
| }) | ||
| require.NoError(t, err, "expected at least 1 profile change callback") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Data race on changes slice — protect with a mutex.
The OnProfileChange callback appends to changes from the controller manager's goroutine while wait.PollUntilContextTimeout reads len(changes) and the post-poll assertions read changes[0] from the test goroutine. This is a data race on the slice header; the Go race detector will flag it and it can cause flaky failures or panics if the slice is reallocated mid-read. The TODO at line 119 acknowledges this but it should be fixed before merge.
As per coding guidelines, check for potential race conditions in controller logic. As per path instructions for **/*_test.go, ensure tests are deterministic and not flaky.
🔒 Proposed fix: guard `changes` with `sync.Mutex`
import (
"context"
"crypto/tls"
"fmt"
"os"
"path/filepath"
+ "sync"
"testing"
"time"
...
)
// Inside TestIntegrationSecurityProfileWatcherOnProfileChange:
-// TODO(guidonguido): access to changes is eventually consistent. Both controller manager
-// and test goroutine are accessing it. Consider turning it atomic.
-var changes []profileChange
+var (
+ mu sync.Mutex
+ changes []profileChange
+)
watcher := &tlsutil.SecurityProfileWatcher{
...
OnProfileChange: func(ctx context.Context, oldSpec, newSpec configv1.TLSProfileSpec) {
+ mu.Lock()
+ defer mu.Unlock()
changes = append(changes, profileChange{old: oldSpec, new: newSpec})
},
}
err = wait.PollUntilContextTimeout(context.Background(), 200*time.Millisecond, 10*time.Second, true, func(ctx context.Context) (bool, error) {
+ mu.Lock()
+ defer mu.Unlock()
return len(changes) >= 1, nil
})
require.NoError(t, err, "expected at least 1 profile change callback")
+ mu.Lock()
+ defer mu.Unlock()
assert.Equal(t, *tlsProfileSpec, changes[0].old)
assert.Equal(t, *configv1.TLSProfiles[configv1.TLSProfileModernType], changes[0].new)📝 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.
| // TODO(guidonguido): access to changes is eventually consistent. Both controller manager | |
| // and test goroutine are accessing it. Consider turning it atomic. | |
| var changes []profileChange | |
| watcher := &tlsutil.SecurityProfileWatcher{ | |
| Client: mgr.GetClient(), | |
| InitialTLSProfileSpec: *tlsProfileSpec, | |
| OnProfileChange: func(ctx context.Context, oldSpec, newSpec configv1.TLSProfileSpec) { | |
| changes = append(changes, profileChange{old: oldSpec, new: newSpec}) | |
| }, | |
| } | |
| require.NoError(t, watcher.SetupWithManager(mgr)) | |
| mgrCtx, mgrCancel := context.WithCancel(context.Background()) | |
| defer mgrCancel() | |
| go func() { | |
| if err := mgr.Start(mgrCtx); err != nil { | |
| t.Logf("manager stopped: %v", err) | |
| } | |
| }() | |
| require.True(t, mgr.GetCache().WaitForCacheSync(mgrCtx)) | |
| // Change profile from Intermediate to Modern | |
| require.NoError(t, k8sClient.Get(context.Background(), client.ObjectKeyFromObject(apiServer), apiServer)) | |
| apiServer.Spec.TLSSecurityProfile = &configv1.TLSSecurityProfile{ | |
| Type: configv1.TLSProfileModernType, | |
| } | |
| require.NoError(t, k8sClient.Update(context.Background(), apiServer)) | |
| err = wait.PollUntilContextTimeout(context.Background(), 200*time.Millisecond, 10*time.Second, true, func(ctx context.Context) (bool, error) { | |
| return len(changes) >= 1, nil | |
| }) | |
| require.NoError(t, err, "expected at least 1 profile change callback") | |
| var ( | |
| mu sync.Mutex | |
| changes []profileChange | |
| ) | |
| watcher := &tlsutil.SecurityProfileWatcher{ | |
| Client: mgr.GetClient(), | |
| InitialTLSProfileSpec: *tlsProfileSpec, | |
| OnProfileChange: func(ctx context.Context, oldSpec, newSpec configv1.TLSProfileSpec) { | |
| mu.Lock() | |
| defer mu.Unlock() | |
| changes = append(changes, profileChange{old: oldSpec, new: newSpec}) | |
| }, | |
| } | |
| require.NoError(t, watcher.SetupWithManager(mgr)) | |
| mgrCtx, mgrCancel := context.WithCancel(context.Background()) | |
| defer mgrCancel() | |
| go func() { | |
| if err := mgr.Start(mgrCtx); err != nil { | |
| t.Logf("manager stopped: %v", err) | |
| } | |
| }() | |
| require.True(t, mgr.GetCache().WaitForCacheSync(mgrCtx)) | |
| // Change profile from Intermediate to Modern | |
| require.NoError(t, k8sClient.Get(context.Background(), client.ObjectKeyFromObject(apiServer), apiServer)) | |
| apiServer.Spec.TLSSecurityProfile = &configv1.TLSSecurityProfile{ | |
| Type: configv1.TLSProfileModernType, | |
| } | |
| require.NoError(t, k8sClient.Update(context.Background(), apiServer)) | |
| err = wait.PollUntilContextTimeout(context.Background(), 200*time.Millisecond, 10*time.Second, true, func(ctx context.Context) (bool, error) { | |
| mu.Lock() | |
| defer mu.Unlock() | |
| return len(changes) >= 1, nil | |
| }) | |
| require.NoError(t, err, "expected at least 1 profile change callback") | |
| mu.Lock() | |
| defer mu.Unlock() | |
| assert.Equal(t, *tlsProfileSpec, changes[0].old) | |
| assert.Equal(t, *configv1.TLSProfiles[configv1.TLSProfileModernType], changes[0].new) |
🤖 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 `@operators/pkg/util/tls_integration_test.go` around lines 119 - 153, Protect
the shared changes slice with a sync.Mutex in the test. Lock around the append
in OnProfileChange, and lock when reading its length during
PollUntilContextTimeout and when accessing post-poll entries such as changes[0];
remove the obsolete TODO acknowledging the race.
Sources: Coding guidelines, Path instructions
| var ( | ||
| tlsProfileSpec *ocinfrav1.TLSProfileSpec | ||
| tlsConfig func(*tls.Config) | ||
| // Override in tests to inject a fake client. | ||
| tlsClientFunc = func() (client.Client, error) { //nolint:gocritic // intentional lambda for test injection | ||
| return GetOrCreateOCPConfigCRClient() | ||
| } | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Same unguarded package-var cache pattern as client.go's ocpConfigClient.
tlsProfileSpec and tlsConfig are memoized via bare check-then-act with no mutex/sync.Once, same race exposure as flagged in operators/pkg/util/client.go (GetOrCreateOCPConfigCRClient). Worth applying the same fix here for consistency once that one is resolved.
Also applies to: 33-35, 63-71
🤖 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 `@operators/pkg/util/tls.go` around lines 20 - 27, Protect the memoized package
variables tlsProfileSpec and tlsConfig with the same synchronization approach
used to fix client.go’s ocpConfigClient. Update the functions accessing these
values, including the code around lines 33–35 and 63–71, to use sync.Once or an
appropriate mutex so concurrent calls cannot race during initialization;
preserve tlsClientFunc’s test injection behavior.
| func GetOrCreateTLSConfig(ctx context.Context) (func(*tls.Config), error) { | ||
| if tlsConfig != nil { | ||
| return tlsConfig, nil | ||
| } | ||
|
|
||
| profileSpec, err := GetOrCreateTLSProfileSpec(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| tlsConfig, unsupportedCiphers := tlsutil.NewTLSConfigFromProfile(*profileSpec) | ||
| if len(unsupportedCiphers) > 0 { | ||
| log.Info("TLS configuration contains unsupported ciphers that will be ignored", "ciphers", unsupportedCiphers) | ||
| } | ||
|
|
||
| return tlsConfig, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n 'tlsConfig' operators/pkg/util/*.goRepository: stolostron/multicluster-observability-operator
Length of output: 842
🏁 Script executed:
rg -n 'GetOrCreateTLSConfig\(' .Repository: stolostron/multicluster-observability-operator
Length of output: 1159
Keep the TLS config cache from being shadowed
operators/pkg/util/tls.go:88 uses :=, which creates a new local tlsConfig and leaves the package cache nil. Every call rebuilds the config; switch to = (or predeclare unsupportedCiphers) so the memoization actually sticks.
🤖 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 `@operators/pkg/util/tls.go` around lines 78 - 94, The TLS cache assignment in
GetOrCreateTLSConfig is shadowing the package-level tlsConfig variable, so
memoization never persists. Replace the short declaration with an assignment and
separately declare unsupportedCiphers as needed, ensuring the returned
configuration is stored in the shared tlsConfig cache.
| func SetTLSSecurityConfiguration(ctx context.Context, args []string, tlsCipherSuitesArg string, minTLSversionArg string) ([]string, error) { | ||
| tlsProfileSpec, err := GetOrCreateTLSProfileSpec(ctx) | ||
| if err != nil { | ||
| if isTest, err := strconv.ParseBool(os.Getenv("UNIT_TEST")); err == nil && isTest { | ||
| log.Info("running unit test, skipping TLS profile adherence") | ||
| return args, nil | ||
| } | ||
| log.Error(err, "unable to get TLS security configuration") | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Production code silently bypasses TLS enforcement based on a UNIT_TEST env var.
When GetOrCreateTLSProfileSpec errors, this checks os.Getenv("UNIT_TEST") and, if true, returns the original, unmodified args — meaning no --tls-cipher-suites/--tls-min-version get applied at all, with only an info log. That's a live security downgrade path sitting in production code, gated by an environment variable rather than compile-time test wiring. If UNIT_TEST=true ever leaks into a real deployment (stale env var, copied manifest, debug flag left on), TLS profile enforcement silently no-ops instead of failing loudly — on a PR whose entire point is enforcing TLS profiles.
This exists because tlsClientFunc (the real DI seam, used by tls_test.go in this same package) is unexported and unusable from the renderer test packages, which is why renderer_test.go/renderer_alertmanager_test.go reach for this env var instead. Consider exposing a small exported test-injection hook (e.g. in a testutil/exported setter) so downstream packages can inject a fake client, and drop the env-var branch from the production path entirely.
🤖 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 `@operators/pkg/util/tls.go` around lines 96 - 105, Remove the UNIT_TEST
environment-variable bypass from SetTLSSecurityConfiguration so
GetOrCreateTLSProfileSpec errors always log and return the error. Provide an
exported test-injection hook or setter around the existing tlsClientFunc seam,
update renderer_test.go and renderer_alertmanager_test.go to use it, and
eliminate their reliance on UNIT_TEST for simulating failures.
21c302c to
5129f81
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/lint.yaml (2)
44-45: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFetch the pinned rules commit explicitly.
--depth 1only fetches the current branch tip, so Line 45 will eventually fail whenb85a9f57b02d0202e7cf62a77fd110a26718b3aais no longer that tip. Fetch the SHA directly instead of cloning first. (git-scm.com)🔧 Proposed fix
- run: git clone --depth 1 https://github.com/smith-xyz/argus-observe-rules.git /tmp/argus-observe-rules && git -C /tmp/argus-observe-rules checkout b85a9f57b02d0202e7cf62a77fd110a26718b3aa + run: | + git init /tmp/argus-observe-rules + git -C /tmp/argus-observe-rules remote add origin https://github.com/smith-xyz/argus-observe-rules.git + git -C /tmp/argus-observe-rules fetch --depth 1 origin b85a9f57b02d0202e7cf62a77fd110a26718b3aa + git -C /tmp/argus-observe-rules checkout --detach FETCH_HEAD🤖 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 @.github/workflows/lint.yaml around lines 44 - 45, Update the “Clone argus-observe-rules” workflow step to fetch the pinned commit b85a9f57b02d0202e7cf62a77fd110a26718b3aa explicitly, rather than relying on a shallow clone of the branch tip followed by checkout. Preserve cloning into /tmp/argus-observe-rules and ensure the pinned SHA is available before checkout.
64-65: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin
sebrandon1/tls-config-linttoc2701708a4a961ae6489effaaf002b3ba208e4fa.
@v1is mutable; using the full commit SHA removes a supply-chain hinge and keeps the workflow on the reviewed code path. One less gremlin in the pipeline. ✅🤖 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 @.github/workflows/lint.yaml around lines 64 - 65, Update the “TLS Config Lint” workflow step to replace the mutable sebrandon1/tls-config-lint@v1 reference with the specified full commit SHA c2701708a4a961ae6489effaaf002b3ba208e4fa, preserving the existing action and step configuration.
🤖 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 @.github/workflows/lint.yaml:
- Line 69: Update the TLS-lint configuration’s exclude-dirs entry so it no
longer excludes the entire collectors/metrics/pkg/forwarder directory; remove
the exclusion or narrow it to only the specific fallback TLS case already
covered by suppressions in forwarder.go, while allowing token.go and the package
tests to remain linted.
---
Outside diff comments:
In @.github/workflows/lint.yaml:
- Around line 44-45: Update the “Clone argus-observe-rules” workflow step to
fetch the pinned commit b85a9f57b02d0202e7cf62a77fd110a26718b3aa explicitly,
rather than relying on a shallow clone of the branch tip followed by checkout.
Preserve cloning into /tmp/argus-observe-rules and ensure the pinned SHA is
available before checkout.
- Around line 64-65: Update the “TLS Config Lint” workflow step to replace the
mutable sebrandon1/tls-config-lint@v1 reference with the specified full commit
SHA c2701708a4a961ae6489effaaf002b3ba208e4fa, preserving the existing action and
step configuration.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0ab3ec5c-1562-4202-9185-41cd258161d0
📒 Files selected for processing (1)
.github/workflows/lint.yaml
guidonguido
force-pushed
the
ci/tls-profile-semgrep
branch
2 times, most recently
from
86d76b6 to
8dda63d
Compare
Signed-off-by: Guido Ricioppo <griciopp@redhat.com> Add tls profile tests Signed-off-by: Guido Ricioppo <griciopp@redhat.com>
Signed-off-by: Guido Ricioppo <griciopp@redhat.com>
Signed-off-by: Guido Ricioppo <griciopp@redhat.com>
Signed-off-by: Guido Ricioppo <griciopp@redhat.com>
Affects forwarder and alert-forward Signed-off-by: Guido Ricioppo <griciopp@redhat.com>
guidonguido
force-pushed
the
ci/tls-profile-semgrep
branch
from
8dda63d to
bd15d39
Compare
|
@guidonguido: 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. |
1 participant
Add a check to statically verify the code does not contain manual TLS version/ciphers configurations.
Semgrep rules inherited from HPCASE-maintained semgrep rules for detecting TLS configuration issues: https://github.com/smith-xyz/argus-observe-rules
BLOCKERS:
Summary by CodeRabbit
apiserversaccess and verbs to support TLS profile discovery.