feat(tls): add configurable TLS security profile#2385
Merged
Conversation
Contributor
Reviewer's GuideIntroduce a configurable TLS security profile system for the HTTP server, switching the default from a hardcoded modern (TLS 1.3 only) profile to an intermediate (TLS 1.2+) profile, and adding support for custom TLS versions and cipher configuration, along with tests and documentation for the new options. Flow diagram for TLS security profile selection and SSL acceptor creationflowchart TD
A[HttpServerConfig
tls_enabled true] --> B[HttpServerBuilder::try_from]
B --> C{TlsSecurityProfile::Custom
and tls_min_version is None}
C -- yes --> D[return Err
requires --http-server-tls-min-version]
C -- no --> E[HttpServerBuilder::tls]
E --> F[build_ssl_acceptor]
F --> G{tls.security_profile}
G -- Old --> H[SslAcceptor::mozilla_intermediate_v5
set_min_proto_version None]
G -- Intermediate --> I[SslAcceptor::mozilla_intermediate_v5]
G -- Modern --> J[SslAcceptor::mozilla_modern_v5]
G -- Custom --> K[set_min_proto_version
set_cipher_list
set_ciphersuites]
H --> L[Configured SslAcceptor]
I --> L
J --> L
K --> L
L --> M[log::info
Enabling TLS support
profile]
M --> N[HttpServerBuilder
uses SslAcceptor]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- For the
OldandCustomprofiles you’re always starting frommozilla_intermediate_v5and then tweaking a few knobs, which means ciphers and other defaults remain ‘intermediate’ rather than truly ‘old/custom’—it might be worth either documenting this explicitly in theTlsSecurityProfiledocs or adjusting the builder to more closely match the intended semantics. - The
Customprofile currently enforces onlytls_min_versionwhile leavingtls_ciphers/tls_ciphersuitesoptional, which is slightly at odds with the description of a user-defined profile; consider either relaxing themin_versionrequirement and treating all three as fully optional knobs or tightening validation to ensure the combination of options is consistent with how you expect this profile to be used.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For the `Old` and `Custom` profiles you’re always starting from `mozilla_intermediate_v5` and then tweaking a few knobs, which means ciphers and other defaults remain ‘intermediate’ rather than truly ‘old/custom’—it might be worth either documenting this explicitly in the `TlsSecurityProfile` docs or adjusting the builder to more closely match the intended semantics.
- The `Custom` profile currently enforces only `tls_min_version` while leaving `tls_ciphers`/`tls_ciphersuites` optional, which is slightly at odds with the description of a user-defined profile; consider either relaxing the `min_version` requirement and treating all three as fully optional knobs or tightening validation to ensure the combination of options is consistent with how you expect this profile to be used.
## Individual Comments
### Comment 1
<location path="common/infrastructure/src/app/http.rs" line_range="429-432" />
<code_context>
+/// Build an SSL acceptor configured according to the given TLS profile.
+fn build_ssl_acceptor(tls: &TlsConfiguration) -> anyhow::Result<openssl::ssl::SslAcceptorBuilder> {
+ let acceptor = match tls.security_profile {
+ TlsSecurityProfile::Old => {
+ let mut builder = SslAcceptor::mozilla_intermediate_v5(SslMethod::tls_server())
+ .context("creating Old TLS profile acceptor")?;
+ builder
+ .set_min_proto_version(None)
+ .context("clearing min TLS version for Old profile")?;
</code_context>
<issue_to_address>
**🚨 issue (security):** Clarify or tighten min TLS version for the `Old` profile to avoid enabling too-old protocols
Here you’re clearing the minimum protocol version after starting from `mozilla_intermediate_v5`. With some OpenSSL builds, `set_min_proto_version(None)` can re-enable SSLv3 or other pre–TLS 1.0 protocols, making the runtime behavior weaker than the documented `TLS 1.0+`. Consider explicitly setting `TLS1` (or the intended minimum) instead of `None`, and/or adjust the comment so it accurately describes the minimum protocol version actually enforced.
</issue_to_address>
### Comment 2
<location path="common/infrastructure/src/app/http.rs" line_range="364-368" />
<code_context>
.json_limit(value.json_limit.0.0 as _);
if value.tls_enabled {
+ if value.tls_security_profile == TlsSecurityProfile::Custom
+ && value.tls_min_version.is_none()
+ {
+ return Err(anyhow!(
+ "Custom TLS profile requires --http-server-tls-min-version"
+ ));
+ }
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider validating `Custom` profile invariants closer to `build_ssl_acceptor` as well
Right now `try_from` enforces that `Custom` profiles provide `tls_min_version`, but `build_ssl_acceptor` still accepts a `Custom` profile with `min_version = None` and silently falls back to the `mozilla_intermediate_v5` default. If a `TlsConfiguration` is constructed and passed directly, this invariant can be violated. Please add a guard in `build_ssl_acceptor` (e.g., return an error when `security_profile == Custom && min_version.is_none()`) so this misconfiguration is caught at the point of use as well.
Suggested implementation:
```rust
pub fn build_ssl_acceptor(
config: &TlsConfiguration,
) -> anyhow::Result<SslAcceptorBuilder> {
if config.tls_security_profile == TlsSecurityProfile::Custom
&& config.tls_min_version.is_none()
{
anyhow::bail!(
"Custom TLS profile requires --http-server-tls-min-version (tls_min_version must be set)"
);
}
```
Because I only see part of the file, you may need to adjust:
1. **Function signature / names**: If `build_ssl_acceptor` has a slightly different name or signature (e.g., takes `&HttpConfig` instead of `&TlsConfiguration`), update the `config` type/name in the guard to match.
2. **Error type / crate**: If `anyhow::Result` or `anyhow::bail!` is not used in this file, adapt the error handling to whatever is standard here (e.g., `Result<_, Error>` with `return Err(Error::Config(...))`).
3. **Field names**: Ensure the field names align with your actual config struct (`tls_security_profile` vs `security_profile`, `tls_min_version` vs `min_version`). The guard should check the `Custom` variant and the min-version option field actually used in your struct.
4. **Import**: If `TlsSecurityProfile` is not in scope in this module, add an appropriate `use` statement at the top of the file (e.g., `use crate::app::http::TlsSecurityProfile;` or wherever it is defined).
</issue_to_address>
### Comment 3
<location path="common/infrastructure/src/app/http.rs" line_range="270-279" />
<code_context>
+ )]
+ pub tls_min_version: Option<TlsMinVersion>,
+
+ /// TLS 1.2 cipher list in OpenSSL format (only used with custom profile)
+ #[arg(id = "http-server-tls-ciphers", long, env = "HTTP_SERVER_TLS_CIPHERS")]
+ pub tls_ciphers: Option<String>,
+
+ /// TLS 1.3 ciphersuites in OpenSSL format (only used with custom profile)
+ #[arg(
+ id = "http-server-tls-ciphersuites",
+ long,
+ env = "HTTP_SERVER_TLS_CIPHERSUITES"
+ )]
+ pub tls_ciphersuites: Option<String>,
+
/// Disable the request log
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Enforce or surface misuse when ciphers/ciphersuites are set without a `Custom` profile
Currently, when `security_profile` is not `Custom`, any values provided for `tls_ciphers` / `tls_ciphersuites` are silently ignored, despite the CLI docs stating they are "only used with custom profile". This can lead to unnoticed misconfigurations. Consider either rejecting configurations that set these fields without `Custom`, or emitting a clear warning that they are ignored unless `tls_security_profile=custom` is selected.
Suggested implementation:
```rust
/// Minimum TLS version (ignored unless `tls_security_profile=custom`)
```
```rust
/// TLS 1.2 cipher list in OpenSSL format (ignored unless `tls_security_profile=custom`)
```
```rust
/// TLS 1.3 ciphersuites in OpenSSL format (ignored unless `tls_security_profile=custom`)
```
`).
Here are the code changes to adjust the CLI docs:
<file_operations>
<file_operation operation="edit" file_path="common/infrastructure/src/app/http.rs">
<<<<<<< SEARCH
/// Minimum TLS version (only used with custom profile)
=======
/// Minimum TLS version (ignored unless `tls_security_profile=custom`)
>>>>>>> REPLACE
<<<<<<< SEARCH
/// TLS 1.2 cipher list in OpenSSL format (only used with custom profile)
=======
/// TLS 1.2 cipher list in OpenSSL format (ignored unless `tls_security_profile=custom`)
>>>>>>> REPLACE
<<<<<<< SEARCH
/// TLS 1.3 ciphersuites in OpenSSL format (only used with custom profile)
=======
/// TLS 1.3 ciphersuites in OpenSSL format (ignored unless `tls_security_profile=custom`)
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
To fully implement your suggestion of enforcing or warning on misuse at runtime, you will also need to:
1. Identify the struct that owns these fields (e.g., the CLI args/config struct in this file) and add a validation method such as:
```rust
impl HttpServerConfig {
pub fn validate_tls_profile(&self) {
if self.tls_security_profile != TlsSecurityProfile::Custom {
if self.tls_min_version.is_some()
|| self.tls_ciphers.is_some()
|| self.tls_ciphersuites.is_some()
{
tracing::warn!(
"TLS ciphers/ciphersuites/tls_min_version are configured but ignored \
because tls_security_profile is not `custom`"
);
}
}
}
}
```
(Adjust the struct name and logging crate to match your codebase.)
2. Call this validation method right after parsing CLI arguments / building this config, so that the warning is emitted on startup whenever these options are set but `tls_security_profile != Custom`. If you prefer hard failure instead of a warning, have this method return a `Result` and `bail!`/`Err` in that branch according to your existing error-handling conventions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+429
to
+432
| TlsSecurityProfile::Old => { | ||
| let mut builder = SslAcceptor::mozilla_intermediate_v5(SslMethod::tls_server()) | ||
| .context("creating Old TLS profile acceptor")?; | ||
| builder |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Clarify or tighten min TLS version for the Old profile to avoid enabling too-old protocols
Here you’re clearing the minimum protocol version after starting from mozilla_intermediate_v5. With some OpenSSL builds, set_min_proto_version(None) can re-enable SSLv3 or other pre–TLS 1.0 protocols, making the runtime behavior weaker than the documented TLS 1.0+. Consider explicitly setting TLS1 (or the intended minimum) instead of None, and/or adjust the comment so it accurately describes the minimum protocol version actually enforced.
Comment on lines
+364
to
+368
| if value.tls_security_profile == TlsSecurityProfile::Custom | ||
| && value.tls_min_version.is_none() | ||
| { | ||
| return Err(anyhow!( | ||
| "Custom TLS profile requires --http-server-tls-min-version" |
Contributor
There was a problem hiding this comment.
🚨 suggestion (security): Consider validating Custom profile invariants closer to build_ssl_acceptor as well
Right now try_from enforces that Custom profiles provide tls_min_version, but build_ssl_acceptor still accepts a Custom profile with min_version = None and silently falls back to the mozilla_intermediate_v5 default. If a TlsConfiguration is constructed and passed directly, this invariant can be violated. Please add a guard in build_ssl_acceptor (e.g., return an error when security_profile == Custom && min_version.is_none()) so this misconfiguration is caught at the point of use as well.
Suggested implementation:
pub fn build_ssl_acceptor(
config: &TlsConfiguration,
) -> anyhow::Result<SslAcceptorBuilder> {
if config.tls_security_profile == TlsSecurityProfile::Custom
&& config.tls_min_version.is_none()
{
anyhow::bail!(
"Custom TLS profile requires --http-server-tls-min-version (tls_min_version must be set)"
);
}Because I only see part of the file, you may need to adjust:
- Function signature / names: If
build_ssl_acceptorhas a slightly different name or signature (e.g., takes&HttpConfiginstead of&TlsConfiguration), update theconfigtype/name in the guard to match. - Error type / crate: If
anyhow::Resultoranyhow::bail!is not used in this file, adapt the error handling to whatever is standard here (e.g.,Result<_, Error>withreturn Err(Error::Config(...))). - Field names: Ensure the field names align with your actual config struct (
tls_security_profilevssecurity_profile,tls_min_versionvsmin_version). The guard should check theCustomvariant and the min-version option field actually used in your struct. - Import: If
TlsSecurityProfileis not in scope in this module, add an appropriateusestatement at the top of the file (e.g.,use crate::app::http::TlsSecurityProfile;or wherever it is defined).
Comment on lines
+270
to
+279
| /// TLS 1.2 cipher list in OpenSSL format (only used with custom profile) | ||
| #[arg(id = "http-server-tls-ciphers", long, env = "HTTP_SERVER_TLS_CIPHERS")] | ||
| pub tls_ciphers: Option<String>, | ||
|
|
||
| /// TLS 1.3 ciphersuites in OpenSSL format (only used with custom profile) | ||
| #[arg( | ||
| id = "http-server-tls-ciphersuites", | ||
| long, | ||
| env = "HTTP_SERVER_TLS_CIPHERSUITES" | ||
| )] |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Enforce or surface misuse when ciphers/ciphersuites are set without a Custom profile
Currently, when security_profile is not Custom, any values provided for tls_ciphers / tls_ciphersuites are silently ignored, despite the CLI docs stating they are "only used with custom profile". This can lead to unnoticed misconfigurations. Consider either rejecting configurations that set these fields without Custom, or emitting a clear warning that they are ignored unless tls_security_profile=custom is selected.
Suggested implementation:
/// Minimum TLS version (ignored unless `tls_security_profile=custom`)
/// TLS 1.2 cipher list in OpenSSL format (ignored unless `tls_security_profile=custom`)
/// TLS 1.3 ciphersuites in OpenSSL format (ignored unless `tls_security_profile=custom`)
`).
Here are the code changes to adjust the CLI docs:
<file_operations>
<file_operation operation="edit" file_path="common/infrastructure/src/app/http.rs">
<<<<<<< SEARCH
/// Minimum TLS version (only used with custom profile)
/// Minimum TLS version (ignored unless `tls_security_profile=custom`)
REPLACE
<<<<<<< SEARCH
/// TLS 1.2 cipher list in OpenSSL format (only used with custom profile)
/// TLS 1.2 cipher list in OpenSSL format (ignored unless `tls_security_profile=custom`)
REPLACE
<<<<<<< SEARCH
/// TLS 1.3 ciphersuites in OpenSSL format (only used with custom profile)
/// TLS 1.3 ciphersuites in OpenSSL format (ignored unless `tls_security_profile=custom`)
REPLACE
</file_operation>
</file_operations>
<additional_changes>
To fully implement your suggestion of enforcing or warning on misuse at runtime, you will also need to:
- Identify the struct that owns these fields (e.g., the CLI args/config struct in this file) and add a validation method such as:
(Adjust the struct name and logging crate to match your codebase.)
impl HttpServerConfig { pub fn validate_tls_profile(&self) { if self.tls_security_profile != TlsSecurityProfile::Custom { if self.tls_min_version.is_some() || self.tls_ciphers.is_some() || self.tls_ciphersuites.is_some() { tracing::warn!( "TLS ciphers/ciphersuites/tls_min_version are configured but ignored \ because tls_security_profile is not `custom`" ); } } } }
- Call this validation method right after parsing CLI arguments / building this config, so that the warning is emitted on startup whenever these options are set but
tls_security_profile != Custom. If you prefer hard failure instead of a warning, have this method return aResultandbail!/Errin that branch according to your existing error-handling conventions.
Contributor
|
Why would we default to a less secure profile? |
ctron
reviewed
Jun 9, 2026
| .json_limit(value.json_limit.0.0 as _); | ||
|
|
||
| if value.tls_enabled { | ||
| if value.tls_security_profile == TlsSecurityProfile::Custom |
Contributor
There was a problem hiding this comment.
Why not use clap's support for this?
Contributor
Author
Claude recommended it because it's OpenShift's default and it broadens our compatibility with TLS 1.2 clients. |
Contributor
I am sure compatibility with TLS 1.0 would be even higher. But that's the wrong direction. If the user wants a less secure profile, they can choose that. But I'd not default to it. Did anyone ever ask us to do this? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2385 +/- ##
==========================================
- Coverage 71.37% 71.34% -0.04%
==========================================
Files 441 441
Lines 26891 27004 +113
Branches 26891 27004 +113
==========================================
+ Hits 19193 19265 +72
- Misses 6576 6609 +33
- Partials 1122 1130 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Replace the hardcoded mozilla_modern_v5 (TLS 1.3 only) SSL acceptor with a configurable TLS security profile aligned with OpenShift TLS Security Profiles. The new --http-server-tls-security-profile flag supports four profiles: old (TLS 1.0+), intermediate (TLS 1.2+, new default), modern (TLS 1.3 only), and custom (user-defined min version and cipher suites). This changes the default from Modern to Intermediate to match the OpenShift default and broaden client compatibility. Implements TC-3426 Assisted-by: Claude Code
Change the default TLS security profile from intermediate (TLS 1.2+) to modern (TLS 1.3 only), preserving the existing security posture. Operators who need broader compatibility can opt in to a less restrictive profile via HTTP_SERVER_TLS_SECURITY_PROFILE. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
@ctron I have changed the default per your request. The ultimate ask in TC-2437 is that TPA fetches and uses the policy configured in OpenShift, but I don't think it's appropriate to add any k8s libs to TPA. Instead, I think our operator should fetch the policy and then set the corresponding environment variables that this PR introduces. If you agree/approve, I think we can go ahead and merge this. |
ctron
approved these changes
Jun 17, 2026
ctron
left a comment
ctron
left a comment
Contributor
There was a problem hiding this comment.
Yes, I agree that the backend should only provide options. The operator should translate and set those options.
|
Successfully created backport PR for |
Contributor
Author
|
For completeness, the operator side: trustification/trusted-profile-analyzer-operator#850 |
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
2 participants
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.
Replace the hardcoded mozilla_modern_v5 (TLS 1.3 only) SSL acceptor with a configurable TLS security profile aligned with OpenShift TLS Security Profiles. The new --http-server-tls-security-profile flag supports four profiles: old (TLS 1.0+), intermediate (TLS 1.2+, new default), modern (TLS 1.3 only), and custom (user-defined min version and cipher suites).
This changes the default from Modern to Intermediate to match the OpenShift default and broaden client compatibility.
Implements TC-3426
Assisted-by: Claude Code
Summary by Sourcery
Introduce configurable TLS security profiles for the HTTP server and align defaults with OpenShift’s intermediate profile.
New Features:
Enhancements:
Documentation:
Tests: