Return rustls listener FIPS failures as errors#885
Conversation
eldryoth
force-pushed
the
rustls-listener-fips-result
branch
from
9987b73 to
49baf28
Compare
| tls: self | ||
| .tls | ||
| .take() | ||
| .map(|tls| tls.try_build().map(Arc::new)) |
There was a problem hiding this comment.
Do the other TLS backends need try_build() too? This path can use their TlsSettings as well.
|
I may be wrong, but for this listener FIPS work the digest in |
|
Good catch @fabian4. The common listener stack can be built with any enabled TLS backend, so calling try_build() there means each backend TlsSettings needs that method. I added try_build() for the OpenSSL/BoringSSL and s2n listener settings, preserving their existing build() behavior. You are right @53v3n3d4. The ServerConfig could be built with the selected rustls provider, but the SslDigest certificate fingerprint still used pingora_rustls::hash_certificate(), which used ring directly. I changed the rustls listener acceptor to capture a SHA-256 hash implementation from the final ServerConfig crypto provider and pass it into the server TlsStream digest path. For require_fips(true), listener construction now fails closed if no SHA-256 provider is available. CI note: the nightly-only failure appears unrelated to this PR. The failing target is
The memory-cache target passes locally, and the relevant checks for this PR pass:
The same CI matrix appears to pass on Rust 1.84.0 and 1.91.1; only nightly reports the unrelated |
|
@eldryoth, a question when you build for fips this commit, did you check the tree if the ring is not being included? I see that default crypto provider not being gated. I am no reviewer, only read this because I am started using pingora with rustls/aws lc rs. |
|
Good question. I checked this, and this PR does not make Today The default provider path is intentionally left as-is for compatibility: if no provider is configured, Pingora keeps using the existing default ring behavior. For FIPS users, the intended path is to explicitly set the AWS-LC / FIPS provider and set So if we want instead “no ring in the dependency tree at all”, this PR is not sufficient. That would need a larger follow-up to make For us in our project we can already choose the rustls/AWS-LC FIPS provider and validate it before startup, but the final rustls listener But release builds use Our side that wires this policy into Pingora is here: It sets the selected rustls crypto provider and then calls |
|
Thanks for reply and good project. I didn't know the fips in ServerConfig but remembered me the |
| /// The TLS settings of a listening endpoint | ||
| pub struct TlsSettings { | ||
| alpn_protocols: Option<Vec<Vec<u8>>>, | ||
| protocol_versions: &'static [&'static SupportedProtocolVersion], |
There was a problem hiding this comment.
While &'static [...] is undoubtedly efficient, it can be extremely inconvenient in certain scenarios (such as dynamically reading values from configuration files). Given that there's no significant performance benefit to optimizing TlsSettings this way, would Vec<...> be a more suitable choice?
| /// Set the supported cipher suites for this endpoint. | ||
| pub fn set_cipher_suites(&mut self, cipher_suites: Vec<SupportedCipherSuite>) { | ||
| self.crypto_provider | ||
| .get_or_insert_with(pingora_rustls::ring_default_crypto_provider) | ||
| .cipher_suites = cipher_suites; | ||
| } | ||
|
|
||
| /// Set the supported key exchange groups for this endpoint. | ||
| pub fn set_kx_groups(&mut self, kx_groups: Vec<&'static dyn SupportedKxGroup>) { | ||
| self.crypto_provider | ||
| .get_or_insert_with(pingora_rustls::ring_default_crypto_provider) | ||
| .kx_groups = kx_groups; | ||
| } |
There was a problem hiding this comment.
I suspect that set_cipher_suites and set_kx_groups may be unnecessary. First, you can pass the configured *CryptoProvider directly via set_crypto_provider. Also, calling set_crypto_provider after set_cipher_suites causes the settings to be reset, which creates behavior that depends on the order of execution. I feel that this is likely to induce user errors.
| /// Require at least TLS 1.2 for this endpoint. | ||
| pub fn set_min_protocol_tls12(&mut self) { | ||
| self.protocol_versions = &TLS12_AND_13; | ||
| } | ||
|
|
||
| /// Require at least TLS 1.3 for this endpoint. | ||
| pub fn set_min_protocol_tls13(&mut self) { | ||
| self.protocol_versions = &TLS13_ONLY; | ||
| } |
There was a problem hiding this comment.
How about directly passing the protocol_versions array?
|
Future considerations: I find the current situation where both ring and aws_lc_rs coexist at runtime to be problematic. Using To prevent this, we should implement a compile-time option allowing users to choose between either ring or aws-lc-rs, ensuring that only the selected crypto provider is compiled. However, implementing this option in this PR would be too disruptive, so it should remain a future task. |
5 participants
Summary
This PR makes rustls listener construction able to report configuration failures through Pingora's normal
Resultpath instead of requiring downstream users to panic when a listener policy cannot be satisfied.It adds a fallible
TlsSettings::try_build()method for rustls listeners and uses it when Pingora builds listener transport stacks. The existingbuild()method remains available for compatibility and delegates totry_build().unwrap(), so existing direct callers keep the previous behavior.The PR also exposes a small set of rustls server configuration hooks needed by applications that must select a specific rustls crypto provider or verify the resulting listener configuration.
Motivation
rustls 0.23 requires an explicit
CryptoProvider. Some downstream applications need to use a provider selected by policy rather than Pingora's defaultringprovider. One example is a deployment profile that builds rustls with the AWS-LC FIPS-capable provider and must fail closed if the finalServerConfigdoes not reportfips().Before this change, Pingora's rustls listener path only exposed a panicking
build()API. That makes policy enforcement awkward for applications that need startup to fail cleanly with structured context. The only practical downstream option was to keep a final panic assertion after normal provider and TLS policy checks.This PR moves that failure into Pingora's normal error path.
What Changed
TlsSettings::try_build() -> pingora_error::Result<Acceptor>for rustls listeners.TlsSettings::build() -> Acceptorfor source compatibility.try_build()and propagate the error.CryptoProviderServerConfigto report FIPS modepingora-rustls.Compatibility
This is intended to be low risk for existing Pingora users:
TlsSettings::intermediate(cert, key)behavior is preserved.TlsSettings::build()remains available.set_require_fips(true)are not subject to the new FIPS check.Resulterrors instead of panics.Why this is useful downstream
Downstream applications that have compliance-driven TLS requirements can now express those requirements through Pingora's rustls listener configuration and let Pingora fail startup cleanly if the resulting
rustls::ServerConfigdoes not satisfy them.This does not make Pingora itself enforce FIPS policy globally. It only provides the hooks and error propagation needed for applications to opt into that policy.
Testing
cargo fmtcargo check -p pingora-core --features rustlscargo test -p pingora-core --features rustls try_build_returns_error_when_required_fips_is_not_reported