Replace Ruby bosh-nats-sync with Golang implementation#2746
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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/go.yml:
- Line 21: The actions/checkout steps lack the persist-credentials parameter
which increases the risk of token reuse in subsequent steps and artifacts. Add
the parameter `persist-credentials: false` to both instances of the
`actions/checkout@v7` action to disable credential persistence and reduce token
exposure risk. This parameter should be added as a separate line within the uses
section for both checkout actions.
- Around line 21-22: Replace the GitHub Actions version tag pins with full
commit SHA pins for enhanced security. Update the `uses:` entries for
`actions/checkout@v7` and `actions/setup-go@v6` (and any other actions mentioned
in the comment) to use their full commit SHA instead of the `@v*` tag format.
This applies to all `uses:` statements throughout the workflow file where
actions are currently pinned to version tags rather than specific commit hashes.
In `@src/bosh-nats-sync/go.mod`:
- Line 3: The Go toolchain and dependencies contain multiple security
vulnerabilities that must be addressed before release. Update the go directive
from version 1.26.1 to 1.26.3 to resolve five known security vulnerabilities.
Additionally, upgrade golang.org/x/net from v0.49.0 to v0.53.0 to address the
GO-2026-4918 vulnerability affecting net/http infinite loops, and upgrade
golang.org/x/sys from v0.40.0 to v0.44.0 to address the GO-2026-5024
vulnerability affecting sys/windows integer overflow. After making these version
updates in the go.mod file, refresh the module graph by running go mod tidy to
ensure all transitive dependencies are properly resolved and consistent.
In `@src/bosh-nats-sync/pkg/authprovider/auth_provider_test.go`:
- Around line 305-308: The test assertion in the "Token expiration deadline"
describe block is tautological because it compares two identical constant
expressions (60 * time.Second to 60 * time.Second) which will always pass
regardless of actual production code changes. Replace the hardcoded 60 *
time.Second on the left side of the Expect call in the It block with
authprovider.ExpirationDeadline so the test actually verifies the production
constant has the expected value, while keeping the right side as 60 *
time.Second for the comparison target.
In `@src/bosh-nats-sync/pkg/authprovider/auth_provider.go`:
- Around line 87-91: In the error handling block following the failed token
acquisition call to a.cfg.Token(a.ctx), change the return statement from
returning ("", nil) to returning ("", err) to properly propagate the
authentication error instead of suppressing it. This ensures that downstream
code receives the error and can handle the transient failure appropriately
through the intended retry and error handling flow.
- Around line 109-111: The http.Client being returned from this function lacks
an explicit timeout, which can cause token requests to hang indefinitely. Add a
Timeout field to the http.Client struct that is being returned to specify a
reasonable timeout duration for UAA token requests. This ensures that network
operations on the client will not block indefinitely and will allow sync
progress to continue even if token requests become unresponsive.
- Around line 99-107: The code currently silently ignores read errors and parse
failures when loading the configured CA certificate file in the block around
caCertPath and x509.NewCertPool(). Instead of only proceeding when err is nil,
you should fail fast by returning an error when either the file read fails or
when AppendCertsFromPEM fails to parse the certificates. This ensures that
configuration issues with the CA file are surfaced immediately rather than
silently degrading TLS trust behavior at runtime.
In `@src/bosh-nats-sync/pkg/config/config.go`:
- Around line 55-60: The NewLogger function dereferences the cfg parameter
without first checking if it is nil, which will cause a panic if a nil config is
passed. Add a nil check at the start of the NewLogger function before accessing
cfg.LogFile, and when cfg is nil, fall back to creating a logger that writes to
stdout instead of attempting to access cfg fields.
- Around line 41-53: The Load function unmarshals the YAML data into the Config
struct but does not validate the configuration before returning it, which can
lead to runtime failures when downstream code attempts to use fields like
Intervals.PollUserSync with time.NewTicker. After the yaml.Unmarshal call
succeeds, add validation logic to check that required fields are present and
that interval values (specifically Intervals.PollUserSync) are positive non-zero
durations. If validation fails, return an error with a descriptive message. This
ensures the config is semantically valid before being used.
In `@src/bosh-nats-sync/pkg/runner/runner_test.go`:
- Around line 193-204: The failCmdRunner in this test fails immediately on the
first call (startup reload), causing Run() to exit before reaching the periodic
ticker loop, so the periodic sync failure path is never tested. Modify the
failCmdRunner to succeed on the first invocation (to allow the startup reload to
complete) and fail on subsequent invocations (to test the periodic sync failure
path). Use a counter or state tracking within the failCmdRunner function to
differentiate between the first call and later calls, ensuring the test actually
validates periodic-sync failure as described.
In `@src/bosh-nats-sync/pkg/runner/runner.go`:
- Around line 57-59: The code creates a time.NewTicker with a duration from
r.config.Intervals.PollUserSync without validating that the value is greater
than zero, which can cause a panic if the config value is unset or invalid. Add
validation logic before creating the ticker to check if the interval duration is
greater than zero, and return an appropriate error or handle the invalid
configuration gracefully instead of allowing time.NewTicker to panic. This
validation should occur before the line where time.NewTicker is called.
In `@src/bosh-nats-sync/pkg/userssync/users_sync_test.go`:
- Around line 500-549: The test context is titled "when director becomes
available after retries" but the It block only verifies that Execute() succeeds
without validating that retries actually occurred. The /info handler is designed
to return StatusServiceUnavailable on the first request and StatusOK on
subsequent requests, but the test never asserts this retry behavior happened.
Add an assertion after the sync.Execute() call in the It block to check that
requestCount >= 2, ensuring the /info endpoint was called multiple times and the
retry mechanism in withDirectorConnection() was actually exercised.
In `@src/bosh-nats-sync/pkg/userssync/users_sync.go`:
- Around line 307-309: The deploymentName parameter in the getVMsByDeployment
method is directly embedded into the API path using fmt.Sprintf without URL
encoding, which will cause issues if the deployment name contains reserved URL
characters like forward slashes, percent signs, or spaces. Fix this by
URL-escaping the deploymentName before embedding it into the path string using
the appropriate URL encoding function from the net/url package, specifically
url.QueryEscape or url.PathEscape as appropriate for path parameters.
- Around line 107-127: The ConnectionWaitTimeout is used to calculate retry
attempts but doesn't enforce an overall elapsed-time budget across all probes.
Each HTTP call can block independently for up to 30 seconds, causing startup to
stall far longer than the configured timeout. In the withDirectorConnection()
method, create a deadline based on the configured timeout at the start and track
elapsed time throughout the retry loop. Before each call to
boshAPIResponseBody() and before sleeping, check if the elapsed time would
exceed the deadline and bail out if so. This ensures the total time spent across
all attempts, including individual HTTP timeouts, never exceeds the
ConnectionWaitTimeout. Apply the same deadline-checking pattern to all other
callers of boshAPIResponseBody() in getAuthHeader(), queryAllDeployments(), and
getVMsByDeployment() methods.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74b1ea7c-2402-441d-a6e3-cda6eee9250b
⛔ Files ignored due to path filters (2)
src/Gemfile.lockis excluded by!**/*.locksrc/bosh-nats-sync/go.sumis excluded by!**/*.sum
📒 Files selected for processing (49)
.github/workflows/go.yml.github/workflows/ruby.ymljobs/nats/specjobs/nats/templates/bosh_nats_syncjobs/nats/templates/bpm.ymlpackages/nats/packagingpackages/nats/specsrc/Gemfilesrc/bosh-nats-sync/.gitignoresrc/bosh-nats-sync/.golangci.ymlsrc/bosh-nats-sync/bin/bosh-nats-syncsrc/bosh-nats-sync/bosh-nats-sync.gemspecsrc/bosh-nats-sync/cmd/bosh-nats-sync/main.gosrc/bosh-nats-sync/go.modsrc/bosh-nats-sync/lib/nats_sync.rbsrc/bosh-nats-sync/lib/nats_sync/auth_provider.rbsrc/bosh-nats-sync/lib/nats_sync/config.rbsrc/bosh-nats-sync/lib/nats_sync/nats_auth_config.rbsrc/bosh-nats-sync/lib/nats_sync/runner.rbsrc/bosh-nats-sync/lib/nats_sync/users_sync.rbsrc/bosh-nats-sync/lib/nats_sync/version.rbsrc/bosh-nats-sync/lib/nats_sync/yaml_helper.rbsrc/bosh-nats-sync/pkg/authprovider/auth_provider.gosrc/bosh-nats-sync/pkg/authprovider/auth_provider_suite_test.gosrc/bosh-nats-sync/pkg/authprovider/auth_provider_test.gosrc/bosh-nats-sync/pkg/config/config.gosrc/bosh-nats-sync/pkg/config/config_suite_test.gosrc/bosh-nats-sync/pkg/config/config_test.gosrc/bosh-nats-sync/pkg/natsauthconfig/nats_auth_config.gosrc/bosh-nats-sync/pkg/natsauthconfig/nats_auth_config_suite_test.gosrc/bosh-nats-sync/pkg/natsauthconfig/nats_auth_config_test.gosrc/bosh-nats-sync/pkg/runner/runner.gosrc/bosh-nats-sync/pkg/runner/runner_suite_test.gosrc/bosh-nats-sync/pkg/runner/runner_test.gosrc/bosh-nats-sync/pkg/userssync/users_sync.gosrc/bosh-nats-sync/pkg/userssync/users_sync_suite_test.gosrc/bosh-nats-sync/pkg/userssync/users_sync_test.gosrc/bosh-nats-sync/spec/nats_sync/auth_provider_spec.rbsrc/bosh-nats-sync/spec/nats_sync/config_spec.rbsrc/bosh-nats-sync/spec/nats_sync/nats_auth_config_spec.rbsrc/bosh-nats-sync/spec/nats_sync/runner_spec.rbsrc/bosh-nats-sync/spec/nats_sync/users_sync_spec.rbsrc/bosh-nats-sync/spec/nats_sync_spec.rbsrc/bosh-nats-sync/spec/spec_helper.rbsrc/bosh-nats-sync/spec/support/uaa_helpers.rbsrc/bosh-nats-sync/testdata/director-subjectsrc/bosh-nats-sync/testdata/hm-subjectsrc/bosh-nats-sync/testdata/sample_config.ymlsrc/tasks/spec.rake
💤 Files with no reviewable changes (25)
- src/bosh-nats-sync/lib/nats_sync/version.rb
- jobs/nats/templates/bosh_nats_sync
- src/bosh-nats-sync/spec/support/uaa_helpers.rb
- src/bosh-nats-sync/lib/nats_sync/nats_auth_config.rb
- .github/workflows/ruby.yml
- src/bosh-nats-sync/.gitignore
- src/bosh-nats-sync/lib/nats_sync/yaml_helper.rb
- src/bosh-nats-sync/spec/nats_sync/auth_provider_spec.rb
- src/bosh-nats-sync/lib/nats_sync/runner.rb
- src/bosh-nats-sync/spec/nats_sync/runner_spec.rb
- src/bosh-nats-sync/spec/nats_sync/users_sync_spec.rb
- src/bosh-nats-sync/bin/bosh-nats-sync
- src/bosh-nats-sync/bosh-nats-sync.gemspec
- src/bosh-nats-sync/spec/nats_sync/nats_auth_config_spec.rb
- src/bosh-nats-sync/spec/nats_sync/config_spec.rb
- src/bosh-nats-sync/lib/nats_sync.rb
- src/Gemfile
- jobs/nats/spec
- src/bosh-nats-sync/lib/nats_sync/config.rb
- src/bosh-nats-sync/spec/nats_sync_spec.rb
- src/bosh-nats-sync/testdata/sample_config.yml
- src/bosh-nats-sync/spec/spec_helper.rb
- src/bosh-nats-sync/lib/nats_sync/users_sync.rb
- src/bosh-nats-sync/lib/nats_sync/auth_provider.rb
- jobs/nats/templates/bpm.yml
7577681 to
d923c47
Compare
d923c47 to
b468aaa
Compare
84f41f0 to
8d773dc
Compare
fddaf21 to
5980267
Compare
118abae to
5b1d58a
Compare
…olled tls.Config Address item 1 in PR review comment #2746 (comment): authprovider and userssync both built their director/UAA TLS client by hand (manual tls.Config{MinVersion}, x509.NewCertPool(), AppendCertsFromPEM). CF already ships code.cloudfoundry.org/tlsconfig as the standard helper for exactly this, and the reviewer noted the hand-rolled config was actually *less* hardened than the org-standard one (no MaxVersion, no cipher-suite pinning). Deliberately does NOT touch items 2/3 from the same comment (reusing bosh-cli's director/uaa packages for the director API and UAA calls) per explicit instruction to avoid a github.com/cloudfoundry/bosh-cli dependency; that remains a footprint-vs-reuse tradeoff for a future PR, not something to pull in incidentally here. pkg/authprovider/auth_provider.go: - buildHTTPClient now builds its *tls.Config via tlsconfig.Build(tlsconfig.WithExternalServiceDefaults()).Client(...), applying tlsconfig.WithAuthorityFromFile(caCertPath) only when CAFilePath() resolves to a file with non-empty content. WithExternalServiceDefaults() pins MinVersion/MaxVersion to TLS 1.2-1.3 and a curated Mozilla-Intermediate cipher-suite list, in place of the previous MinVersion-only config. - Preserves existing behavior exactly: a configured-but-unreadable CA file is still a hard error, an empty file still falls back to the system trust store, and a malformed (but present/non-empty) cert is still a hard error. pkg/userssync/users_sync.go: - buildHTTPClient now uses the same tlsconfig.Build(...).Client(...) pattern for the director API client. Replaced directorCACertPool (manual x509.CertPool + AppendCertsFromPEM) with a small usableCACertContent(path) predicate that gates whether tlsconfig.WithAuthorityFromFile is applied. - Preserves existing behavior: director_ca_cert missing/unreadable/empty falls back to the system trust store (no error, matching Ruby's usable_director_ca_cert?), while a configured-but-unparseable cert is a hard error rather than a silent fallback. pkg/userssync/export_test.go: - Replaced the removed DirectorCACertPool test hook with BuildDirectorTLSConfig, which builds the real *http.Client via buildHTTPClient and returns its *tls.Config, so tests assert against the actual TLS configuration (RootCAs, MinVersion) rather than an internal CertPool-returning helper that no longer exists. pkg/userssync/users_sync_test.go: - Rewrote the "directorCACertPool" spec block against BuildDirectorTLSConfig: same four cases (not configured / missing file / whitespace-only file / unparseable content) now assert on tlsCfg.RootCAs and the error message, plus a new case asserting MinVersion is pinned to TLS 1.2 via WithExternalServiceDefaults. Dependency footprint: go.mod/go.sum/vendor pick up code.cloudfoundry.org/tlsconfig v0.61.0 and a matching golang.org/x/sys patch bump (v0.44.0 -> v0.46.0, already an existing indirect dependency). tlsconfig's own test-only dependencies (certstrap, go.step.sm/crypto, golang.org/x/crypto, etc.) appear in go.sum for module-graph completeness but are not imported by any non-test file, so `go mod vendor` does not pull them into vendor/ or the build. Verified `go build`, `go vet`, and `go test -race ./...` all pass with both the default module resolution and explicit `-mod=vendor` (matching how packages/nats/packaging invokes go build); gofmt and golangci-lint are clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
35830f0 to
87e76cb
Compare
…olled tls.Config Address item 1 in PR review comment #2746 (comment): authprovider and userssync both built their director/UAA TLS client by hand (manual tls.Config{MinVersion}, x509.NewCertPool(), AppendCertsFromPEM). CF already ships code.cloudfoundry.org/tlsconfig as the standard helper for exactly this, and the reviewer noted the hand-rolled config was actually *less* hardened than the org-standard one (no MaxVersion, no cipher-suite pinning). Deliberately does NOT touch items 2/3 from the same comment (reusing bosh-cli's director/uaa packages for the director API and UAA calls) per explicit instruction to avoid a github.com/cloudfoundry/bosh-cli dependency; that remains a footprint-vs-reuse tradeoff for a future PR, not something to pull in incidentally here. pkg/authprovider/auth_provider.go: - buildHTTPClient now builds its *tls.Config via tlsconfig.Build(tlsconfig.WithExternalServiceDefaults()).Client(...), applying tlsconfig.WithAuthorityFromFile(caCertPath) only when CAFilePath() resolves to a file with non-empty content. WithExternalServiceDefaults() pins MinVersion/MaxVersion to TLS 1.2-1.3 and a curated Mozilla-Intermediate cipher-suite list, in place of the previous MinVersion-only config. - Preserves existing behavior exactly: a configured-but-unreadable CA file is still a hard error, an empty file still falls back to the system trust store, and a malformed (but present/non-empty) cert is still a hard error. pkg/userssync/users_sync.go: - buildHTTPClient now uses the same tlsconfig.Build(...).Client(...) pattern for the director API client. Replaced directorCACertPool (manual x509.CertPool + AppendCertsFromPEM) with a small usableCACertContent(path) predicate that gates whether tlsconfig.WithAuthorityFromFile is applied. - Preserves existing behavior: director_ca_cert missing/unreadable/empty falls back to the system trust store (no error, matching Ruby's usable_director_ca_cert?), while a configured-but-unparseable cert is a hard error rather than a silent fallback. pkg/userssync/export_test.go: - Replaced the removed DirectorCACertPool test hook with BuildDirectorTLSConfig, which builds the real *http.Client via buildHTTPClient and returns its *tls.Config, so tests assert against the actual TLS configuration (RootCAs, MinVersion) rather than an internal CertPool-returning helper that no longer exists. pkg/userssync/users_sync_test.go: - Rewrote the "directorCACertPool" spec block against BuildDirectorTLSConfig: same four cases (not configured / missing file / whitespace-only file / unparseable content) now assert on tlsCfg.RootCAs and the error message, plus a new case asserting MinVersion is pinned to TLS 1.2 via WithExternalServiceDefaults. Dependency footprint: go.mod/go.sum/vendor pick up code.cloudfoundry.org/tlsconfig v0.61.0 and a matching golang.org/x/sys patch bump (v0.44.0 -> v0.46.0, already an existing indirect dependency). tlsconfig's own test-only dependencies (certstrap, go.step.sm/crypto, golang.org/x/crypto, etc.) appear in go.sum for module-graph completeness but are not imported by any non-test file, so `go mod vendor` does not pull them into vendor/ or the build. Verified `go build`, `go vet`, and `go test -race ./...` all pass with both the default module resolution and explicit `-mod=vendor` (matching how packages/nats/packaging invokes go build); gofmt and golangci-lint are clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
87e76cb to
1e15707
Compare
…olled tls.Config Address item 1 in PR review comment #2746 (comment): authprovider and userssync both built their director/UAA TLS client by hand (manual tls.Config{MinVersion}, x509.NewCertPool(), AppendCertsFromPEM). CF already ships code.cloudfoundry.org/tlsconfig as the standard helper for exactly this, and the reviewer noted the hand-rolled config was actually *less* hardened than the org-standard one (no MaxVersion, no cipher-suite pinning). Deliberately does NOT touch items 2/3 from the same comment (reusing bosh-cli's director/uaa packages for the director API and UAA calls) per explicit instruction to avoid a github.com/cloudfoundry/bosh-cli dependency; that remains a footprint-vs-reuse tradeoff for a future PR, not something to pull in incidentally here. pkg/authprovider/auth_provider.go: - buildHTTPClient now builds its *tls.Config via tlsconfig.Build(tlsconfig.WithExternalServiceDefaults()).Client(...), applying tlsconfig.WithAuthorityFromFile(caCertPath) only when CAFilePath() resolves to a file with non-empty content. WithExternalServiceDefaults() pins MinVersion/MaxVersion to TLS 1.2-1.3 and a curated Mozilla-Intermediate cipher-suite list, in place of the previous MinVersion-only config. - Preserves existing behavior exactly: a configured-but-unreadable CA file is still a hard error, an empty file still falls back to the system trust store, and a malformed (but present/non-empty) cert is still a hard error. pkg/userssync/users_sync.go: - buildHTTPClient now uses the same tlsconfig.Build(...).Client(...) pattern for the director API client. Replaced directorCACertPool (manual x509.CertPool + AppendCertsFromPEM) with a small usableCACertContent(path) predicate that gates whether tlsconfig.WithAuthorityFromFile is applied. - Preserves existing behavior: director_ca_cert missing/unreadable/empty falls back to the system trust store (no error, matching Ruby's usable_director_ca_cert?), while a configured-but-unparseable cert is a hard error rather than a silent fallback. pkg/userssync/export_test.go: - Replaced the removed DirectorCACertPool test hook with BuildDirectorTLSConfig, which builds the real *http.Client via buildHTTPClient and returns its *tls.Config, so tests assert against the actual TLS configuration (RootCAs, MinVersion) rather than an internal CertPool-returning helper that no longer exists. pkg/userssync/users_sync_test.go: - Rewrote the "directorCACertPool" spec block against BuildDirectorTLSConfig: same four cases (not configured / missing file / whitespace-only file / unparseable content) now assert on tlsCfg.RootCAs and the error message, plus a new case asserting MinVersion is pinned to TLS 1.2 via WithExternalServiceDefaults. Dependency footprint: go.mod/go.sum/vendor pick up code.cloudfoundry.org/tlsconfig v0.61.0 and a matching golang.org/x/sys patch bump (v0.44.0 -> v0.46.0, already an existing indirect dependency). tlsconfig's own test-only dependencies (certstrap, go.step.sm/crypto, golang.org/x/crypto, etc.) appear in go.sum for module-graph completeness but are not imported by any non-test file, so `go mod vendor` does not pull them into vendor/ or the build. Verified `go build`, `go vet`, and `go test -race ./...` all pass with both the default module resolution and explicit `-mod=vendor` (matching how packages/nats/packaging invokes go build); gofmt and golangci-lint are clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1e15707 to
e94d5d2
Compare
…olled tls.Config Address item 1 in PR review comment #2746 (comment): authprovider and userssync both built their director/UAA TLS client by hand (manual tls.Config{MinVersion}, x509.NewCertPool(), AppendCertsFromPEM). CF already ships code.cloudfoundry.org/tlsconfig as the standard helper for exactly this, and the reviewer noted the hand-rolled config was actually *less* hardened than the org-standard one (no MaxVersion, no cipher-suite pinning). Deliberately does NOT touch items 2/3 from the same comment (reusing bosh-cli's director/uaa packages for the director API and UAA calls) per explicit instruction to avoid a github.com/cloudfoundry/bosh-cli dependency; that remains a footprint-vs-reuse tradeoff for a future PR, not something to pull in incidentally here. pkg/authprovider/auth_provider.go: - buildHTTPClient now builds its *tls.Config via tlsconfig.Build(tlsconfig.WithExternalServiceDefaults()).Client(...), applying tlsconfig.WithAuthorityFromFile(caCertPath) only when CAFilePath() resolves to a file with non-empty content. WithExternalServiceDefaults() pins MinVersion/MaxVersion to TLS 1.2-1.3 and a curated Mozilla-Intermediate cipher-suite list, in place of the previous MinVersion-only config. - Preserves existing behavior exactly: a configured-but-unreadable CA file is still a hard error, an empty file still falls back to the system trust store, and a malformed (but present/non-empty) cert is still a hard error. pkg/userssync/users_sync.go: - buildHTTPClient now uses the same tlsconfig.Build(...).Client(...) pattern for the director API client. Replaced directorCACertPool (manual x509.CertPool + AppendCertsFromPEM) with a small usableCACertContent(path) predicate that gates whether tlsconfig.WithAuthorityFromFile is applied. - Preserves existing behavior: director_ca_cert missing/unreadable/empty falls back to the system trust store (no error, matching Ruby's usable_director_ca_cert?), while a configured-but-unparseable cert is a hard error rather than a silent fallback. pkg/userssync/export_test.go: - Replaced the removed DirectorCACertPool test hook with BuildDirectorTLSConfig, which builds the real *http.Client via buildHTTPClient and returns its *tls.Config, so tests assert against the actual TLS configuration (RootCAs, MinVersion) rather than an internal CertPool-returning helper that no longer exists. pkg/userssync/users_sync_test.go: - Rewrote the "directorCACertPool" spec block against BuildDirectorTLSConfig: same four cases (not configured / missing file / whitespace-only file / unparseable content) now assert on tlsCfg.RootCAs and the error message, plus a new case asserting MinVersion is pinned to TLS 1.2 via WithExternalServiceDefaults. Dependency footprint: go.mod/go.sum/vendor pick up code.cloudfoundry.org/tlsconfig v0.61.0 and a matching golang.org/x/sys patch bump (v0.44.0 -> v0.46.0, already an existing indirect dependency). tlsconfig's own test-only dependencies (certstrap, go.step.sm/crypto, golang.org/x/crypto, etc.) appear in go.sum for module-graph completeness but are not imported by any non-test file, so `go mod vendor` does not pull them into vendor/ or the build. Verified `go build`, `go vet`, and `go test -race ./...` all pass with both the default module resolution and explicit `-mod=vendor` (matching how packages/nats/packaging invokes go build); gofmt and golangci-lint are clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
e94d5d2 to
7e3388f
Compare
…olled tls.Config Address item 1 in PR review comment #2746 (comment): authprovider and userssync both built their director/UAA TLS client by hand (manual tls.Config{MinVersion}, x509.NewCertPool(), AppendCertsFromPEM). CF already ships code.cloudfoundry.org/tlsconfig as the standard helper for exactly this, and the reviewer noted the hand-rolled config was actually *less* hardened than the org-standard one (no MaxVersion, no cipher-suite pinning). Deliberately does NOT touch items 2/3 from the same comment (reusing bosh-cli's director/uaa packages for the director API and UAA calls) per explicit instruction to avoid a github.com/cloudfoundry/bosh-cli dependency; that remains a footprint-vs-reuse tradeoff for a future PR, not something to pull in incidentally here. pkg/authprovider/auth_provider.go: - buildHTTPClient now builds its *tls.Config via tlsconfig.Build(tlsconfig.WithExternalServiceDefaults()).Client(...), applying tlsconfig.WithAuthorityFromFile(caCertPath) only when CAFilePath() resolves to a file with non-empty content. WithExternalServiceDefaults() pins MinVersion/MaxVersion to TLS 1.2-1.3 and a curated Mozilla-Intermediate cipher-suite list, in place of the previous MinVersion-only config. - Preserves existing behavior exactly: a configured-but-unreadable CA file is still a hard error, an empty file still falls back to the system trust store, and a malformed (but present/non-empty) cert is still a hard error. pkg/userssync/users_sync.go: - buildHTTPClient now uses the same tlsconfig.Build(...).Client(...) pattern for the director API client. Replaced directorCACertPool (manual x509.CertPool + AppendCertsFromPEM) with a small usableCACertContent(path) predicate that gates whether tlsconfig.WithAuthorityFromFile is applied. - Preserves existing behavior: director_ca_cert missing/unreadable/empty falls back to the system trust store (no error, matching Ruby's usable_director_ca_cert?), while a configured-but-unparseable cert is a hard error rather than a silent fallback. pkg/userssync/export_test.go: - Replaced the removed DirectorCACertPool test hook with BuildDirectorTLSConfig, which builds the real *http.Client via buildHTTPClient and returns its *tls.Config, so tests assert against the actual TLS configuration (RootCAs, MinVersion) rather than an internal CertPool-returning helper that no longer exists. pkg/userssync/users_sync_test.go: - Rewrote the "directorCACertPool" spec block against BuildDirectorTLSConfig: same four cases (not configured / missing file / whitespace-only file / unparseable content) now assert on tlsCfg.RootCAs and the error message, plus a new case asserting MinVersion is pinned to TLS 1.2 via WithExternalServiceDefaults. Dependency footprint: go.mod/go.sum/vendor pick up code.cloudfoundry.org/tlsconfig v0.61.0 and a matching golang.org/x/sys patch bump (v0.44.0 -> v0.46.0, already an existing indirect dependency). tlsconfig's own test-only dependencies (certstrap, go.step.sm/crypto, golang.org/x/crypto, etc.) appear in go.sum for module-graph completeness but are not imported by any non-test file, so `go mod vendor` does not pull them into vendor/ or the build. Verified `go build`, `go vet`, and `go test -race ./...` all pass with both the default module resolution and explicit `-mod=vendor` (matching how packages/nats/packaging invokes go build); gofmt and golangci-lint are clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
7e3388f to
8befa9e
Compare
…olled tls.Config Address item 1 in PR review comment #2746 (comment): authprovider and userssync both built their director/UAA TLS client by hand (manual tls.Config{MinVersion}, x509.NewCertPool(), AppendCertsFromPEM). CF already ships code.cloudfoundry.org/tlsconfig as the standard helper for exactly this, and the reviewer noted the hand-rolled config was actually *less* hardened than the org-standard one (no MaxVersion, no cipher-suite pinning). Deliberately does NOT touch items 2/3 from the same comment (reusing bosh-cli's director/uaa packages for the director API and UAA calls) per explicit instruction to avoid a github.com/cloudfoundry/bosh-cli dependency; that remains a footprint-vs-reuse tradeoff for a future PR, not something to pull in incidentally here. pkg/authprovider/auth_provider.go: - buildHTTPClient now builds its *tls.Config via tlsconfig.Build(tlsconfig.WithExternalServiceDefaults()).Client(...), applying tlsconfig.WithAuthorityFromFile(caCertPath) only when CAFilePath() resolves to a file with non-empty content. WithExternalServiceDefaults() pins MinVersion/MaxVersion to TLS 1.2-1.3 and a curated Mozilla-Intermediate cipher-suite list, in place of the previous MinVersion-only config. - Preserves existing behavior exactly: a configured-but-unreadable CA file is still a hard error, an empty file still falls back to the system trust store, and a malformed (but present/non-empty) cert is still a hard error. pkg/userssync/users_sync.go: - buildHTTPClient now uses the same tlsconfig.Build(...).Client(...) pattern for the director API client. Replaced directorCACertPool (manual x509.CertPool + AppendCertsFromPEM) with a small usableCACertContent(path) predicate that gates whether tlsconfig.WithAuthorityFromFile is applied. - Preserves existing behavior: director_ca_cert missing/unreadable/empty falls back to the system trust store (no error, matching Ruby's usable_director_ca_cert?), while a configured-but-unparseable cert is a hard error rather than a silent fallback. pkg/userssync/export_test.go: - Replaced the removed DirectorCACertPool test hook with BuildDirectorTLSConfig, which builds the real *http.Client via buildHTTPClient and returns its *tls.Config, so tests assert against the actual TLS configuration (RootCAs, MinVersion) rather than an internal CertPool-returning helper that no longer exists. pkg/userssync/users_sync_test.go: - Rewrote the "directorCACertPool" spec block against BuildDirectorTLSConfig: same four cases (not configured / missing file / whitespace-only file / unparseable content) now assert on tlsCfg.RootCAs and the error message, plus a new case asserting MinVersion is pinned to TLS 1.2 via WithExternalServiceDefaults. Dependency footprint: go.mod/go.sum/vendor pick up code.cloudfoundry.org/tlsconfig v0.61.0 and a matching golang.org/x/sys patch bump (v0.44.0 -> v0.46.0, already an existing indirect dependency). tlsconfig's own test-only dependencies (certstrap, go.step.sm/crypto, golang.org/x/crypto, etc.) appear in go.sum for module-graph completeness but are not imported by any non-test file, so `go mod vendor` does not pull them into vendor/ or the build. Verified `go build`, `go vet`, and `go test -race ./...` all pass with both the default module resolution and explicit `-mod=vendor` (matching how packages/nats/packaging invokes go build); gofmt and golangci-lint are clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8befa9e to
9061c91
Compare
…olled tls.Config Address item 1 in PR review comment #2746 (comment): authprovider and userssync both built their director/UAA TLS client by hand (manual tls.Config{MinVersion}, x509.NewCertPool(), AppendCertsFromPEM). CF already ships code.cloudfoundry.org/tlsconfig as the standard helper for exactly this, and the reviewer noted the hand-rolled config was actually *less* hardened than the org-standard one (no MaxVersion, no cipher-suite pinning). Deliberately does NOT touch items 2/3 from the same comment (reusing bosh-cli's director/uaa packages for the director API and UAA calls) per explicit instruction to avoid a github.com/cloudfoundry/bosh-cli dependency; that remains a footprint-vs-reuse tradeoff for a future PR, not something to pull in incidentally here. pkg/authprovider/auth_provider.go: - buildHTTPClient now builds its *tls.Config via tlsconfig.Build(tlsconfig.WithExternalServiceDefaults()).Client(...), applying tlsconfig.WithAuthorityFromFile(caCertPath) only when CAFilePath() resolves to a file with non-empty content. WithExternalServiceDefaults() pins MinVersion/MaxVersion to TLS 1.2-1.3 and a curated Mozilla-Intermediate cipher-suite list, in place of the previous MinVersion-only config. - Preserves existing behavior exactly: a configured-but-unreadable CA file is still a hard error, an empty file still falls back to the system trust store, and a malformed (but present/non-empty) cert is still a hard error. pkg/userssync/users_sync.go: - buildHTTPClient now uses the same tlsconfig.Build(...).Client(...) pattern for the director API client. Replaced directorCACertPool (manual x509.CertPool + AppendCertsFromPEM) with a small usableCACertContent(path) predicate that gates whether tlsconfig.WithAuthorityFromFile is applied. - Preserves existing behavior: director_ca_cert missing/unreadable/empty falls back to the system trust store (no error, matching Ruby's usable_director_ca_cert?), while a configured-but-unparseable cert is a hard error rather than a silent fallback. pkg/userssync/export_test.go: - Replaced the removed DirectorCACertPool test hook with BuildDirectorTLSConfig, which builds the real *http.Client via buildHTTPClient and returns its *tls.Config, so tests assert against the actual TLS configuration (RootCAs, MinVersion) rather than an internal CertPool-returning helper that no longer exists. pkg/userssync/users_sync_test.go: - Rewrote the "directorCACertPool" spec block against BuildDirectorTLSConfig: same four cases (not configured / missing file / whitespace-only file / unparseable content) now assert on tlsCfg.RootCAs and the error message, plus a new case asserting MinVersion is pinned to TLS 1.2 via WithExternalServiceDefaults. Dependency footprint: go.mod/go.sum/vendor pick up code.cloudfoundry.org/tlsconfig v0.61.0 and a matching golang.org/x/sys patch bump (v0.44.0 -> v0.46.0, already an existing indirect dependency). tlsconfig's own test-only dependencies (certstrap, go.step.sm/crypto, golang.org/x/crypto, etc.) appear in go.sum for module-graph completeness but are not imported by any non-test file, so `go mod vendor` does not pull them into vendor/ or the build. Verified `go build`, `go vet`, and `go test -race ./...` all pass with both the default module resolution and explicit `-mod=vendor` (matching how packages/nats/packaging invokes go build); gofmt and golangci-lint are clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9061c91 to
cd58b16
Compare
…olled tls.Config Address item 1 in PR review comment #2746 (comment): authprovider and userssync both built their director/UAA TLS client by hand (manual tls.Config{MinVersion}, x509.NewCertPool(), AppendCertsFromPEM). CF already ships code.cloudfoundry.org/tlsconfig as the standard helper for exactly this, and the reviewer noted the hand-rolled config was actually *less* hardened than the org-standard one (no MaxVersion, no cipher-suite pinning). Deliberately does NOT touch items 2/3 from the same comment (reusing bosh-cli's director/uaa packages for the director API and UAA calls) per explicit instruction to avoid a github.com/cloudfoundry/bosh-cli dependency; that remains a footprint-vs-reuse tradeoff for a future PR, not something to pull in incidentally here. pkg/authprovider/auth_provider.go: - buildHTTPClient now builds its *tls.Config via tlsconfig.Build(tlsconfig.WithExternalServiceDefaults()).Client(...), applying tlsconfig.WithAuthorityFromFile(caCertPath) only when CAFilePath() resolves to a file with non-empty content. WithExternalServiceDefaults() pins MinVersion/MaxVersion to TLS 1.2-1.3 and a curated Mozilla-Intermediate cipher-suite list, in place of the previous MinVersion-only config. - Preserves existing behavior exactly: a configured-but-unreadable CA file is still a hard error, an empty file still falls back to the system trust store, and a malformed (but present/non-empty) cert is still a hard error. pkg/userssync/users_sync.go: - buildHTTPClient now uses the same tlsconfig.Build(...).Client(...) pattern for the director API client. Replaced directorCACertPool (manual x509.CertPool + AppendCertsFromPEM) with a small usableCACertContent(path) predicate that gates whether tlsconfig.WithAuthorityFromFile is applied. - Preserves existing behavior: director_ca_cert missing/unreadable/empty falls back to the system trust store (no error, matching Ruby's usable_director_ca_cert?), while a configured-but-unparseable cert is a hard error rather than a silent fallback. pkg/userssync/export_test.go: - Replaced the removed DirectorCACertPool test hook with BuildDirectorTLSConfig, which builds the real *http.Client via buildHTTPClient and returns its *tls.Config, so tests assert against the actual TLS configuration (RootCAs, MinVersion) rather than an internal CertPool-returning helper that no longer exists. pkg/userssync/users_sync_test.go: - Rewrote the "directorCACertPool" spec block against BuildDirectorTLSConfig: same four cases (not configured / missing file / whitespace-only file / unparseable content) now assert on tlsCfg.RootCAs and the error message, plus a new case asserting MinVersion is pinned to TLS 1.2 via WithExternalServiceDefaults. Dependency footprint: go.mod/go.sum/vendor pick up code.cloudfoundry.org/tlsconfig v0.61.0 and a matching golang.org/x/sys patch bump (v0.44.0 -> v0.46.0, already an existing indirect dependency). tlsconfig's own test-only dependencies (certstrap, go.step.sm/crypto, golang.org/x/crypto, etc.) appear in go.sum for module-graph completeness but are not imported by any non-test file, so `go mod vendor` does not pull them into vendor/ or the build. Verified `go build`, `go vet`, and `go test -race ./...` all pass with both the default module resolution and explicit `-mod=vendor` (matching how packages/nats/packaging invokes go build); gofmt and golangci-lint are clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fcdd9d7 to
8619ba8
Compare
8619ba8 to
b9a9ff5
Compare
Summary
bosh-nats-syncgem with a Go binary (src/bosh-nats-sync/) that mirrors all Ruby behavior including TLS verification preferences (uaa_ca_cert>director_ca_cert), HTTP peer verification, and startup reliabilitynatsBOSH job and package (director-ruby-3.3, gem bundling,BUNDLE_GEMFILE/GEM_HOMEenv vars)go.yml); removes the defunctnats_sync:parallelRuby matrix entry fromruby.ymlCommits
Golang bosh-nats-sync — full Go implementation with parity to the Ruby version:
uaa_ca_certpreferred overdirector_ca_cert)InsecureSkipVerify)isConnectionErrorretry logic coveringdeadline exceeded/eof(matching Ruby'sNet::OpenTimeout,Net::ReadTimeout,Errno::ECONNRESET)Remove ruby-isms now that nats is golang — strips Ruby from
jobs/nats/,packages/nats/,src/Gemfile, and CI:packages/nats/packagingnow builds the Go binary viago buildjobs/nats/wrapper script calls Go binary directly (no Ruby runtime sourcing)go.yml: addslint (bosh-nats-sync)andtest (bosh-nats-sync)jobsruby.yml: removesnats_sync:parallelmatrix entrysrc/bosh-nats-sync/.golangci.ymlTest plan
go.yml/ lint (bosh-nats-sync) passesgo.yml/ test (bosh-nats-sync) passesruby.yml/ unit_specs passes for all remaining sub-projects (common:parallel,monitor:parallel,release)spec/nats_templates_spec.rb) pass