feat: add TLS configuration support#2738
Conversation
Add configurable TLS settings for the PAC controller via deployment environment variables. This allows the Tekton Operator to propagate TLS configuration (min version, cipher suites, curve preferences) to the controller without code changes. - Add pkg/tlsconfig package for parsing TLS configuration from environment variables into Go's tls.Config - Integrate tlsconfig into the adapter to apply TLS settings when TLS is enabled, with fallback to secure defaults on parse error - Add TLS_MIN_VERSION, TLS_CIPHER_SUITES, and TLS_CURVE_PREFERENCES env vars to the controller deployment with secure defaults matching Tekton Results Signed-off-by: Zaki Shaikh <zashaikh@redhat.com> Assisted-by: Claude Opus 4.6 (via Claude Code)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2738 +/- ##
==========================================
+ Coverage 59.47% 59.66% +0.18%
==========================================
Files 209 210 +1
Lines 20706 20884 +178
==========================================
+ Hits 12315 12460 +145
- Misses 7616 7645 +29
- Partials 775 779 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces configurable TLS settings for the controller by adding a new tlsconfig package. It allows users to specify the minimum TLS version, cipher suites, and curve preferences via environment variables (TLS_MIN_VERSION, TLS_CIPHER_SUITES, TLS_CURVE_PREFERENCES). The adapter has been updated to load these configurations and apply them to the HTTP server, with fallback defaults and detailed logging of the active TLS parameters. Comprehensive unit tests for the new configuration logic are also included. I have no feedback to provide.
There was a problem hiding this comment.
Code Review
This pull request introduces a new tlsconfig package to allow the controller's TLS settings—including minimum version, cipher suites, and curve preferences—to be configured via environment variables. The adapter is updated to load and apply these settings to the HTTP server. Review feedback highlights the need for stricter validation of numeric IDs for TLS versions, ciphers, and curves to prevent the use of insecure or unsupported configurations, and recommends returning errors for unrecognized inputs instead of silently skipping them.
| // Try parsing as decimal number first (e.g., "771" for TLS 1.3) | ||
| if num, err := strconv.ParseUint(version, 10, 16); err == nil { | ||
| return uint16(num), nil | ||
| } |
There was a problem hiding this comment.
The numeric TLS version is parsed but not validated against supported constants. This could allow setting invalid or unsupported protocol versions in the tls.Config. It is safer to restrict numeric inputs to the known range of TLS versions (1.0 to 1.3).
// Try parsing as decimal number first (e.g., "771" for TLS 1.3)
if num, err := strconv.ParseUint(version, 10, 16); err == nil {
v := uint16(num)
if v >= tls.VersionTLS10 && v <= tls.VersionTLS13 {
return v, nil
}
return 0, fmt.Errorf("unsupported numeric TLS version: %d", v)
}| if id, err := strconv.ParseUint(part, 10, 16); err == nil { | ||
| result = append(result, uint16(id)) | ||
| continue | ||
| } | ||
|
|
||
| // Try looking up by IANA name using Go's built-in cipher suite info | ||
| if id, ok := cipherMap[part]; ok { | ||
| result = append(result, id) | ||
| continue | ||
| } | ||
|
|
||
| // Unknown or insecure cipher - silently skip | ||
| // The resulting config is logged at startup for verification |
There was a problem hiding this comment.
There are two issues here:
- Security Bypass: Allowing arbitrary numeric IDs for cipher suites bypasses the secure ciphers only policy mentioned in the comments (line 108). Users could provide IDs for insecure ciphers that are supported by the Go runtime but excluded from tls.CipherSuites().
- Silent Failure: Unknown or misspelled cipher names are silently skipped. This can lead to a server running with a significantly different (and potentially much more restrictive) set of ciphers than the user intended, without a clear error.
It is recommended to validate numeric IDs against the secure list and return an error for any unrecognized input.
// Try parsing as numeric ID first
if id, err := strconv.ParseUint(part, 10, 16); err == nil {
uID := uint16(id)
// Ensure the numeric ID corresponds to a known secure cipher suite
isSecure := false
for _, suite := range tls.CipherSuites() {
if suite.ID == uID {
isSecure = true
break
}
}
if !isSecure {
return nil, fmt.Errorf("unknown or insecure cipher suite ID: %d", uID)
}
result = append(result, uID)
continue
}
// Try looking up by IANA name using Go's built-in cipher suite info
if id, ok := cipherMap[part]; ok {
result = append(result, id)
continue
}
return nil, fmt.Errorf("unknown or insecure cipher suite: %s", part)| if id, err := strconv.ParseUint(part, 10, 16); err == nil { | ||
| result = append(result, tls.CurveID(id)) | ||
| continue | ||
| } | ||
|
|
||
| // Try parsing as curve name | ||
| if id, ok := curveNameToID[part]; ok { | ||
| result = append(result, id) | ||
| continue | ||
| } | ||
|
|
||
| // Unknown curve - silently skip | ||
| // The resulting config is logged at startup for verification |
There was a problem hiding this comment.
Similar to the cipher suite parsing, numeric curve IDs are not validated against the supported list, and unknown curve names are silently ignored. This should be changed to return an error for any invalid input to ensure the configuration matches the user's intent.
// Try parsing as numeric ID first
if id, err := strconv.ParseUint(part, 10, 16); err == nil {
cID := tls.CurveID(id)
// Check if it's a known curve
isValid := false
for _, cid := range curveNameToID {
if cid == cID {
isValid = true
break
}
}
if !isValid {
return nil, fmt.Errorf("unknown curve ID: %d", cID)
}
result = append(result, cID)
continue
}
// Try parsing as curve name
if id, ok := curveNameToID[part]; ok {
result = append(result, id)
continue
}
return nil, fmt.Errorf("unknown curve name: %s", part)
| // parseTLSVersion converts a version string to tls.Version constant | ||
| // Supports: "1.0", "1.1", "1.2", "1.3" or numeric values like "771" (0x0303). | ||
| func parseTLSVersion(version string) (uint16, error) { | ||
| // Try parsing as decimal number first (e.g., "771" for TLS 1.3) |
There was a problem hiding this comment.
[warning/correctness] Incorrect comment: 771 (0x0303) is TLS 1.2, not TLS 1.3. TLS 1.3 is 772 (0x0304).
This matters because an operator reading this comment to figure out what numeric value to set for TLS 1.3 enforcement would use 771 and get TLS 1.2 instead — a less secure configuration than intended.
| // Try parsing as decimal number first (e.g., "771" for TLS 1.3) | |
| // Try parsing as decimal number first (e.g., "772" for TLS 1.3) |
| // Try parsing as decimal number first (e.g., "771" for TLS 1.3) | ||
| if num, err := strconv.ParseUint(version, 10, 16); err == nil { | ||
| return uint16(num), nil | ||
| } |
There was a problem hiding this comment.
[blocker/security] parseTLSVersion accepts any uint16 as a numeric TLS version without validation. For example, parseTLSVersion("999") succeeds and returns 999, which is not a valid TLS protocol version. Go's tls.Config will silently accept the invalid MinVersion value, leading to undefined behavior.
+1 to the Gemini review comment here — numeric inputs must be validated against known TLS version constants.
| } | |
| // Try parsing as decimal number first (e.g., "772" for TLS 1.3) | |
| if num, err := strconv.ParseUint(version, 10, 16); err == nil { | |
| v := uint16(num) | |
| switch v { | |
| case tls.VersionTLS10, tls.VersionTLS11, tls.VersionTLS12, tls.VersionTLS13: | |
| return v, nil | |
| default: | |
| return 0, fmt.Errorf("unsupported numeric TLS version: %d (supported: 768, 769, 771, 772)", v) | |
| } | |
| } |
| } | ||
|
|
||
| // cipherSuiteCache caches the mapping from cipher name to ID | ||
| // Built from Go's tls.CipherSuites() and tls.InsecureCipherSuites(). |
There was a problem hiding this comment.
[warning/correctness] Misleading var comment — says "Built from Go's tls.CipherSuites() and tls.InsecureCipherSuites()" but the implementation (line 114-122) only uses tls.CipherSuites(). The function godoc below correctly states secure-only, but this var-level comment contradicts it and could mislead contributors into thinking insecure suites are in the map.
| // Built from Go's tls.CipherSuites() and tls.InsecureCipherSuites(). | |
| // cipherSuiteCache caches the mapping from cipher name to ID | |
| // Built from Go's tls.CipherSuites() (secure ciphers only). |
| if id, err := strconv.ParseUint(part, 10, 16); err == nil { | ||
| result = append(result, uint16(id)) | ||
| continue | ||
| } |
There was a problem hiding this comment.
[blocker/security] Numeric cipher suite IDs bypass the secure-only restriction enforced for named ciphers. Named lookups go through getCipherSuiteMap() which only contains tls.CipherSuites() (secure), but a numeric ID like 5 (TLS_RSA_WITH_RC4_128_SHA) would be silently accepted.
The adapter's fallback-on-error (line 117 in adapter.go) partially mitigates this since the config still parses successfully — meaning the insecure cipher is used without any warning. Validate numeric IDs against tls.CipherSuites().
| } | |
| // Try parsing as numeric ID first | |
| if id, err := strconv.ParseUint(part, 10, 16); err == nil { | |
| uID := uint16(id) | |
| isSecure := false | |
| for _, suite := range tls.CipherSuites() { | |
| if suite.ID == uID { | |
| isSecure = true | |
| break | |
| } | |
| } | |
| if !isSecure { | |
| // Skip unknown/insecure numeric cipher IDs | |
| continue | |
| } | |
| result = append(result, uID) | |
| continue | |
| } |
theakshaypant
left a comment
theakshaypant
left a comment
There was a problem hiding this comment.
Generally well-structured implementation with clean separation of concerns. The tlsconfig package is well-designed and the adapter integration handles errors gracefully. However, there are security issues around numeric input validation that need to be addressed before merging.
What this does: Adds a new pkg/tlsconfig package for configuring TLS settings (min version, cipher suites, curve preferences) via environment variables, and integrates it into the adapter's HTTPS server setup.
What's good:
- Clean package design —
LoadFromEnvaccepting afunc(string) stringmakes testing straightforward without polluting real env vars - Adapter fallback to secure defaults on parse error (line 117 in adapter.go) is a solid defense-in-depth pattern
- The
LoadX509KeyPair+ListenAndServeTLS("","")pattern correctly separates cert loading from TLS config - Comprehensive test coverage with table-driven tests covering named lookups, numeric IDs, edge cases, and formatting helpers
- PQC (X25519Kyber768Draft00) forward-looking support is a nice touch
sync.Oncecaching on cipher suite map is correct for concurrent access
Findings (2 blockers, 2 warnings):
pkg/tlsconfig/tls_config.go:80-82— blocker/security:parseTLSVersionaccepts any numericuint16without validating against known TLS versions.parseTLSVersion("999")silently succeeds with an invalid version.pkg/tlsconfig/tls_config.go:145-148— blocker/security: Numeric cipher suite IDs bypass the secure-only policy. A numeric ID for an insecure cipher (e.g. RC4-based) is silently accepted while the same cipher's name would be correctly rejected. Same pattern applies toparseCurvePreferencesat line 219.pkg/tlsconfig/tls_config.go:79— warning/correctness: Comment says"771" for TLS 1.3but 771 is TLS 1.2 (0x0303). TLS 1.3 is 772 (0x0304). An operator using this comment as reference would configure a less secure version than intended.pkg/tlsconfig/tls_config.go:100— warning/correctness: Var comment says cache is "Built from CipherSuites() and InsecureCipherSuites()" but the implementation only usesCipherSuites().
Note: The security findings align with the earlier Gemini review. My inline comments include concrete suggestion blocks with fixes.
Issue coverage: No linked GitHub issue. The JIRA (SRVKP-9681) isn't publicly accessible, so I can't verify completeness against requirements. The implementation covers the described scope — env var-driven TLS config with secure defaults.
3 participants
📝 Description of the Change
Add configurable TLS settings for the PAC controller via deployment environment variables. This allows the Tekton Operator to propagate TLS configuration (min version, cipher suites, curve preferences) to the controller without code changes.
Assisted-by: Claude Opus 4.6 (via Claude Code)
🔗 Linked GitHub Issue
Fixes #
JIRA
https://redhat.atlassian.net/browse/SRVKP-9681
🧪 Testing Strategy
🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Claude noreply@anthropic.com
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.