refactor(tls): use IANA cipher suite names instead of OpenSSL names - #1
Open
zhujian7 wants to merge 5 commits into
Open
refactor(tls): use IANA cipher suite names instead of OpenSSL names#1zhujian7 wants to merge 5 commits into
zhujian7 wants to merge 5 commits into
Conversation
…-cluster-management-io#217) * Add pkg/tls and pkg/watcher packages pkg/tls provides TLS configuration helpers for OCM components: - parse TLS version and cipher suites from flags or a ConfigMap - build crypto/tls.Config and controller-runtime TLSOpts functions - Options type with pflag integration and priority-based config loading (flags > ConfigMap > defaults) pkg/watcher provides a generic ConfigMapWatcher that triggers graceful shutdown (via context cancellation) or a custom callback when a watched ConfigMap is created, updated, or deleted. The initial hash is seeded from the config in use at startup so any drift detected on first sync also triggers a restart. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com> * Trim pkg/tls and pkg/watcher to only what ocm and addon-framework use Remove exported symbols that are not called by either consumer: - options.go deleted (Options, NewOptions, AddFlags, GetTLSConfig, StartConfigMapWatcher, GetTLSConfigForServer — none used) - GetSupportedCipherSuites deleted from cipher.go Make internal-only helpers unexported: - ParseTLSVersion → parseTLSVersion - ParseCipherSuites → parseCipherSuites - DefaultMinTLSVersion → defaultMinTLSVersion - ParseTLSConfigFromConfigMap → parseTLSConfigFromConfigMap - HashConfigMapData → hashConfigMapData (watcher package) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com> * Add StartTLSConfigMapWatcher helper and tests for pkg/tls StartTLSConfigMapWatcher combines ConfigMap loading, watcher seeding, and background goroutine startup into a single call. It: - loads the current TLS config (falling back to defaults if absent) - seeds the watcher with the effective config so mid-startup changes are detected - starts the watcher in a background goroutine - returns the active TLSConfig so callers can apply it immediately - rejects a nil onChangeFn to prevent silent no-ops Add tls_test.go with 96.9% statement coverage across all exported functions and the StartTLSConfigMapWatcher watcher integration. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com> * Add tests for pkg/watcher Tests cover: - hashConfigMapData: nil/empty, single/multi-key sorted output, order independence, collision resistance, different values - NewConfigMapWatcher: field and initialHash initialization - Start AddFunc: no initData sets baseline without trigger, matching initData no-ops, differing initData triggers - Start UpdateFunc: same data no-ops, changed data triggers - Start DeleteFunc: always triggers - Wrong CM name is ignored - cancelFunc is called when no onChangeFunc is set - Pre-cancelled context returns error Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com> * Fix revive stutter lint errors in pkg/tls Rename exported functions whose names begin with the package name (tls.*TLS...) to avoid the revive 'exported' stutter warning: TLSConfigFromFlags -> ConfigFromFlags TLSVersionToString -> VersionToString TLSConfigToFunc -> ConfigToFunc Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com> * Address PR review comments pkg/watcher/watcher.go: - Use sync.Once to protect one-time initialisation of initialHash in AddFunc, eliminating the potential data race - Handle cache.DeletedFinalStateUnknown tombstones in DeleteFunc to avoid a panic when the informer reconnects after a disconnect pkg/watcher/watcher_test.go: - Replace hard-coded 200ms waits with a noEventWait constant (500ms) for more stable CI behaviour - cancelWrapper in TestStart_CancelFuncCalledWhenNoOnChangeFunc now calls the real cancel function; uses a dedicated watcher context so the test select does not race between cancelCalled and ctx.Done() pkg/tls/config.go + callers: - Rename TLSConfigFromFlags -> ConfigFromFlags - Rename TLSVersionToString -> VersionToString - Rename TLSConfigToFunc -> ConfigToFunc to fix revive stutter lint errors (tls.TLS*) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com> * Add README for pkg/tls Documents the ConfigMap format, three use cases (flag-based config, ConfigMap watcher, and operator-to-component flag forwarding), and an API reference table for all exported functions and constants. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com> * Address second round of PR review comments pkg/tls/config.go: - ConfigToFunc: guard against nil TLSConfig (returned by ConfigFromFlags when no flags are set) by returning a no-op func instead of panicking pkg/tls/configmap.go: - StartTLSConfigMapWatcher: seed watcher with raw cm.Data instead of re-serializing through VersionToString/CipherSuitesToString. The watcher hashes raw cm.Data, so normalization differences (e.g. "TLSv1.2" vs "VersionTLS12", or an injected empty "cipherSuites" key) produce a mismatched hash and trigger a spurious restart loop on every startup. - StartTLSConfigMapWatcher: when no CM exists at startup, seed with only non-empty default fields (minTLSVersion only) so a CM created later with non-defaults triggers onChangeFn, while a CM with the same defaults does not. - StartTLSConfigMapWatcher: call w.Start synchronously so it blocks until the informer cache is synced before returning, guaranteeing the watcher is ready when the function returns and removing the goroutine-induced race. pkg/tls/tls_test.go: - ConfigToFunc: add nil TLSConfig test case and negative assertions for CipherSuites and MaxVersion not being set when not expected - StartTLSConfigMapWatcher: replace bool + time.Sleep (data race) with buffered channel + select for race-safe, deterministic event assertions - StartTLSConfigMapWatcher: add test verifying that a CM created with non-default TLS version triggers restart when no CM existed at startup Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: zhujian <jiazhu@redhat.com> * Replace pkg/watcher with base controller in pkg/tls Replace the custom ConfigMapWatcher (pkg/watcher) with a controller built on pkg/basecontroller/factory. The new implementation: - Eliminates the startup race-window workaround (hash seeding via sync.Once) by seeding lastHash from the warm lister after WaitForCacheSync, before starting the controller goroutine - Removes the custom Add/Update/Delete event handlers — Sync() reads from the lister and compares hashes in one place - Deletes pkg/watcher entirely (it was only used by pkg/tls) Signed-off-by: Jia Zhu <jiazhu@redhat.com> Signed-off-by: zhujian <jiazhu@redhat.com> * Fail closed when all cipher suites in ConfigMap are unsupported If cipherSuites is non-empty but every name is unknown, the previous code silently left cfg.CipherSuites nil, causing ConfigToFunc to fall back to Go's default suites instead of rejecting the invalid policy. Return an error when no supported cipher suite is found, consistent with ConfigFromFlags behavior. Add a regression test. Signed-off-by: zhujian <jiazhu@redhat.com> * chore: polish constant doc comments in pkg/tls Signed-off-by: zhujian <jiazhu@redhat.com> --------- Signed-off-by: zhujian <jiazhu@redhat.com> Signed-off-by: Jia Zhu <jiazhu@redhat.com> Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Switch cipherMap keys and cipherIDToName output from OpenSSL-style (e.g. ECDHE-RSA-AES128-GCM-SHA256) to IANA format (e.g. TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256), which matches what Go's crypto/tls package uses. cipherIDToName now looks up the name from tls.CipherSuites() and tls.InsecureCipherSuites() by ID instead of reverse-iterating cipherMap, so the returned name is always the authoritative Go/IANA name. Update tests and README accordingly. Signed-off-by: zhujian <jiazhu@redhat.com>
Replace the hand-maintained cipherMap allowlist with direct lookups against tls.CipherSuites() and tls.InsecureCipherSuites(): - Secure ciphers are accepted silently - Insecure ciphers are accepted but logged via klog.Warningf - Unrecognized names are still rejected (existing behavior) This removes the maintenance burden of keeping cipherMap in sync with Go's cipher suite lists, and automatically picks up new secure ciphers added in future Go releases. Previously excluded insecure ciphers (RC4-based, TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA) are now accepted with a warning instead of being silently rejected. Signed-off-by: zhujian <jiazhu@redhat.com>
zhujian7
force-pushed
the
feat/tls-iana-cipher-names
branch
from
March 30, 2026 02:50
0ff1f46 to
2596f85
Compare
Replace per-call tls.CipherSuites()/InsecureCipherSuites() iterations with three package-level maps built in init(): secureCiphersByName map[string]uint16 -- name→ID for secure suites insecureCiphersByName map[string]uint16 -- name→ID for insecure suites cipherNamesByID map[uint16]string -- ID→name for all suites parseCipherSuites now does O(1) map lookups instead of O(n) linear scans, and no longer needs the findCipherID helper. cipherIDToName is reduced to a single map lookup with no allocation. Signed-off-by: zhujian <jiazhu@redhat.com>
- ConfigFromFlags: trim whitespace from minVersion and cipherSuites before the empty check so whitespace-only flag values are treated as absent - tls_test.go: remove `expectedLen > 0` guard on CipherSuites length assertions so zero-length expectations are also verified Signed-off-by: zhujian <jiazhu@redhat.com>
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
cipherMapkeys from OpenSSL-style names (e.g.ECDHE-RSA-AES128-GCM-SHA256) to IANA format (e.g.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256), which matches what Go'scrypto/tlspackage usescipherIDToNameto look ups.Namefromtls.CipherSuites()andtls.InsecureCipherSuites()by ID, replacing the reverse map iteration — this guarantees the returned name is always the authoritative Go/IANA nameparseCipherSuitesandCipherSuitesToStringdoc comments; no structural changes to those functionsCipherSuitesToStringdescription to reference IANA formatRelated issue(s)
Fixes #open-cluster-management-io/ocm#1443