fix(satellite): decouple Ground Control certificate verification from --use-unsecure - #617
Draft
ojasshelke46 wants to merge 6 commits into
Draft
Conversation
…fication --use-unsecure is documented as "Use insecure (HTTP) connections to registries", but createHTTPClient also set InsecureSkipVerify from it, turning off verification of Ground Control's certificate on the HTTPS connection used for registration and heartbeats. That request carries the registration token, and the response carries the Harbor robot credentials and the state URLs the satellite replicates from. Anyone on the network path could present their own certificate, capture both, and point the satellite at images of their choosing. Nothing in the flag's name or help text suggested it weakened Ground Control authentication, so an operator enabling it to reach a plain-HTTP test registry had no reason to expect this. The two concerns are now separate settings. use_unsecure keeps its registry-only meaning. Ground Control certificate verification is controlled by a new, explicitly named ground_control_skip_tls_verify (--gc-skip-tls-verify, GC_SKIP_TLS_VERIFY), which is never derived from use_unsecure and logs a prominent warning on every use. createHTTPClient no longer accepts use_unsecure at all, so the coupling cannot be reintroduced by passing the wrong value. Tests assert use_unsecure alone leaves InsecureSkipVerify false, and reproduce the attack end to end: registration against a TLS server with an untrusted self-signed certificate now fails and the token never leaves the satellite, where previously the handshake succeeded. Signed-off-by: Ojas Shelke <ojasshelke733@gmail.com>
|
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 34 |
| Duplication | 8 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Contributor
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds ChangesGround Control TLS verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SatelliteCLI
participant ConfigManager
participant GroundControlClient
participant GroundControlServer
SatelliteCLI->>ConfigManager: Initialize GroundControlSkipTLSVerify
ConfigManager-->>GroundControlClient: Provide TLS verification setting
GroundControlClient->>GroundControlServer: Register or report over HTTPS
GroundControlServer-->>GroundControlClient: Return response
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/satellite/state/registration_process.go`:
- Around line 255-264: Ensure the Ground Control client cannot inherit the
generic TLS bypass: in the registration transport setup, force the
`satTLS.Config`/`TLSConfig.SkipVerify` value to false unless `skipTLSVerify` is
explicitly enabled. Add a regression test covering `TLSConfig.SkipVerify=true`
with `GroundControlSkipTLSVerify=false`, verifying certificate verification
remains enabled.
In `@internal/satellite/state/reporting_process.go`:
- Line 161: Update the non-SPIFFE status-report flow around createHTTPClient so
UseUnsecure never permits an HTTP syncURL; require HTTPS before sending status
reports while preserving any SPIFFE-specific behavior. Add a regression test
covering true UseUnsecure with an HTTP status-report URL and verify the request
is rejected or not sent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5664f52-3a44-4807-a50f-238ba8d23b8a
📒 Files selected for processing (10)
.env.examplecmd/satellite/main.gointernal/env/harbor-satellite.gointernal/satellite/state/gc_tls_verify_test.gointernal/satellite/state/registration_process.gointernal/satellite/state/reporting_process.gopkg/config/config.gopkg/config/getters.gopkg/config/manager.gopkg/config/manager_test.go
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/satellite/state/registration_process.go">
<violation number="1" location="internal/satellite/state/registration_process.go:256">
P3: Enabling `--gc-skip-tls-verify` floods logs with the SECURITY warning on every heartbeat because it is inside the per-request client constructor. Emitting it once during application startup, or guarding it with a one-time mechanism, preserves the warning without recurring log noise.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| if useUnsecure { | ||
| if skipTLSVerify { | ||
| logger.FromContext(ctx).Warn(). |
Contributor
There was a problem hiding this comment.
P3: Enabling --gc-skip-tls-verify floods logs with the SECURITY warning on every heartbeat because it is inside the per-request client constructor. Emitting it once during application startup, or guarding it with a one-time mechanism, preserves the warning without recurring log noise.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/satellite/state/registration_process.go, line 256:
<comment>Enabling `--gc-skip-tls-verify` floods logs with the SECURITY warning on every heartbeat because it is inside the per-request client constructor. Emitting it once during application startup, or guarding it with a one-time mechanism, preserves the warning without recurring log noise.</comment>
<file context>
@@ -238,18 +238,30 @@ func registerSatellite(groundControlURL, path, token string, tlsCfg config.TLSCo
- if useUnsecure {
+ if skipTLSVerify {
+ logger.FromContext(ctx).Warn().
+ Msg("SECURITY: Ground Control TLS certificate verification is DISABLED " +
+ "(ground_control_skip_tls_verify). The registration token and Harbor " +
</file context>
Review of the previous commit surfaced four more ways the Ground Control channel could be weakened, plus a warning that never fired. The custom-TLS branch of createHTTPClient forwarded tls.skip_verify into the client config, making it a second, undocumented way to disable Ground Control certificate verification without touching ground_control_skip_tls_verify. That branch is now pinned to false: only the dedicated setting can turn verification off. The same if/else also meant enabling the skip flag short-circuited past client certificate and CA loading, so a Ground Control requiring client auth would reject the connection — the opposite of what the flag is for. Certificate material is now loaded whenever tlsCfg supplies it, and skipTLSVerify affects InsecureSkipVerify alone. Status reports took the same shape from another entry point: the non-SPIFFE path allowed a plain-HTTP sync URL when use_unsecure was set, then attached registry Basic Auth credentials to it. HTTPS is now required unconditionally there; the SPIFFE path is unchanged. The CLI override was applied only during the initial config load. Ground Control ships config that replaces the local one wholesale on every reload, so a satellite started with --gc-skip-tls-verify had verification silently turned back on mid-run. The override is now recorded on the ConfigManager, reapplied on reload, and ORed into the getter so no future reload path can drop it. The startup warning lived in createHTTPClient, which runs on every registration and heartbeat. It now fires once from main.go via a shared constant, so there is a single copy of the message. Tests cover each bypass, and a table-driven matrix asserts that across use_unsecure, ground_control_skip_tls_verify and tls.skip_verify in every combination, only the dedicated setting ever yields InsecureSkipVerify. Existing reporting tests moved to TLS servers, since plain-HTTP status reports are no longer accepted. Signed-off-by: Ojas Shelke <ojasshelke733@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/config/manager.go`:
- Line 225: Replace the direct ConfigManager mutation assigning
gcSkipTLSVerifyOverride with initialization through cm.With(). If no suitable
modifier exists, add a dedicated modifier for this field and use it at the call
site, preserving the existing value.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf54c682-a598-4331-af7b-bbd3231135c5
📒 Files selected for processing (8)
cmd/satellite/main.gointernal/satellite/state/gc_tls_verify_test.gointernal/satellite/state/registration_process.gointernal/satellite/state/reporting_process.gointernal/satellite/state/reporting_process_test.gopkg/config/constants.gopkg/config/getters.gopkg/config/manager.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/satellite/state/reporting_process.go
- cmd/satellite/main.go
| if err != nil { | ||
| return nil, warnings, fmt.Errorf("failed to create config manager: %w", err) | ||
| } | ||
| cm.gcSkipTLSVerifyOverride = gcSkipTLSVerify |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use a cm.With() modifier for this mutation.
Line 225 directly mutates ConfigManager. Initialize gcSkipTLSVerifyOverride through a cm.With() modifier. Add a dedicated modifier if none exists.
As per coding guidelines, “Use cm.With() modifiers for all ConfigManager mutations; never mutate configuration through cm.GetConfig() directly.”
🤖 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 `@pkg/config/manager.go` at line 225, Replace the direct ConfigManager mutation
assigning gcSkipTLSVerifyOverride with initialization through cm.With(). If no
suitable modifier exists, add a dedicated modifier for this field and use it at
the call site, preserving the existing value.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The flag was written into the config the satellite persists. WriteConfig runs at startup and ReloadConfig folds it back into the validated config, so a single run with --gc-skip-tls-verify left ground_control_skip_tls_verify=true in config.json. Since the getter ORs the stored field with the in-memory override, a later run without the flag kept connecting with InsecureSkipVerify and nothing on the command line said so. An operator dropping the flag to re-secure the channel had no way to tell it was still disabled short of reading config.json. The override now lives only on the ConfigManager. InitConfigManager no longer copies it into the config, and ReloadConfig no longer folds it back in: the getter already ORs the in-memory value, so it survives reconciliation without being written anywhere. An explicit ground_control_skip_tls_verify in config.json is still honoured, since that is a separate deliberate choice rather than a leftover. The assignment also moved behind setGCSkipTLSVerifyOverride so it is made under the manager's lock rather than by touching the struct field directly. Tests cover the flag not reaching disk, verification returning when the flag is dropped, the override surviving a reload without being persisted, and the config-file setting still applying on its own. Signed-off-by: Ojas Shelke <ojasshelke733@gmail.com>
Two follow-ups on the Ground Control channel. The HTTPS requirement for status reports sat inside the non-SPIFFE branch, so a SPIFFE-enabled satellite pointed at an http:// Ground Control still sent reports in plaintext. A transport's TLS config is not applied to an http:// URL, so holding an SVID does not make the connection encrypted, and the requirement was only advertised as unconditional. The check now runs on syncURL before the client is chosen, covering both paths. createHTTPClient also always assigns a TLS config now, and net/http skips automatic HTTP/2 negotiation whenever TLSClientConfig is non-nil unless ForceAttemptHTTP2 is set (see Transport.protocols). Ground Control connections that previously ran over HTTP/2 had silently dropped to HTTP/1.1. ForceAttemptHTTP2 restores it; the custom-TLS paths, which never negotiated HTTP/2 because they always set a TLS config, gain it too, which matches http.DefaultTransport. Tests assert the SPIFFE path rejects a plain-HTTP sync URL before it attempts to connect, and that the client negotiates HTTP/2 against an HTTP/2-enabled server. Signed-off-by: Ojas Shelke <ojasshelke733@gmail.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> Signed-off-by: Ojas Shelke <ojasshelke733@gmail.com>
The handlers in the Ground Control TLS tests called require.NoError directly. testify's require calls t.FailNow, which the testing package only permits from the goroutine running the test, so a failed write assertion would have called runtime.Goexit on a handler goroutine and left the test in an undefined state. Handlers now record what they saw in a small mutex-guarded handlerRecord and the test goroutine asserts on it after the request completes. The mutex also covers the bools the handlers used to set, which were written on the handler goroutine and read on the test goroutine without synchronisation. No change to what any of the tests assert. Verified under -race. Signed-off-by: Ojas Shelke <ojasshelke733@gmail.com>
ojasshelke46
force-pushed
the
fix/gc-tls-verify-separate-flag
branch
from
August 12, 2026 12:57
e6a2935 to
0a239ca
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--use-unsecureis documented as controlling plain-HTTP connections toregistries. It also silently disables TLS certificate verification on
the Satellite's HTTPS connection to Ground Control, which is undocumented
and unrelated to what the flag claims to do.
Problem
internal/satellite/state/registration_process.go, lines 248-252, setstransport.TLSClientConfig.InsecureSkipVerify = useUnsecure— derivingGround Control's certificate verification directly from the registry
flag. The same client is reused for heartbeats in
internal/satellite/state/reporting_process.go, line 161.Enrollment carries the Satellite's registration token and gets back the
Harbor robot account credentials plus the state and config URLs the
Satellite will trust from then on. With verification off, anyone on the
network path — commonly true at an edge site — can present their own
certificate, capture the token and credentials, and hand back their own
URLs. The Satellite replicates whatever it's told to after that, with no
error and nothing to review in the logs. An operator enabling
--use-unsecureto reach a plain-HTTP test registry has no reason tosuspect it also does this.
Fix
Split the two concerns into independent flags:
--use-unsecurenow only permits plain-HTTP registry connections.--gc-skip-tls-verifyindependently controlsInsecureSkipVerifyfor the Ground Control client. It defaults tofalseand logs a warning on every startup where it's enabled.Neither flag is derived from the other.
Tests
--use-unsecurealone leavesInsecureSkipVerifyfalse on theGround Control transport.
--gc-skip-tls-verifyalone leaves registry connectionsunaffected.
--gc-skip-tls-verifyis set.Behavior change worth calling out
Anyone currently relying on
--use-unsecureto also skip Ground Controlcertificate verification needs to add
--gc-skip-tls-verifyexplicitlyafter this change. That's the intent: the two should never have been
coupled.
Summary by CodeRabbit
New Features
Bug Fixes
Tests