Skip to content

Return rustls listener FIPS failures as errors#885

Open
eldryoth wants to merge 2 commits into
cloudflare:mainfrom
eldryoth:rustls-listener-fips-result
Open

Return rustls listener FIPS failures as errors#885
eldryoth wants to merge 2 commits into
cloudflare:mainfrom
eldryoth:rustls-listener-fips-result

Conversation

@eldryoth
Copy link
Copy Markdown

@eldryoth eldryoth commented May 22, 2026

Summary

This PR makes rustls listener construction able to report configuration failures through Pingora's normal Result path 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 existing build() method remains available for compatibility and delegates to try_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 default ring provider. One example is a deployment profile that builds rustls with the AWS-LC FIPS-capable provider and must fail closed if the final ServerConfig does not report fips().

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

  • Added TlsSettings::try_build() -> pingora_error::Result<Acceptor> for rustls listeners.
  • Kept TlsSettings::build() -> Acceptor for source compatibility.
  • Changed listener stack construction to call try_build() and propagate the error.
  • Added rustls listener configuration hooks for:
    • explicit CryptoProvider
    • explicit ALPN protocol list
    • TLS 1.2 / TLS 1.3 minimum protocol selection
    • cipher suite selection
    • key exchange group selection
    • certificate resolver support
    • requiring the final ServerConfig to report FIPS mode
  • Re-exported the needed rustls server/provider types from pingora-rustls.
  • Added a regression test proving thatTitle:

Compatibility

This is intended to be low risk for existing Pingora users:

  • Existing TlsSettings::intermediate(cert, key) behavior is preserved.
  • Existing TlsSettings::build() remains available.
  • If no custom provider is configured, the rustls listener still installs/uses the default ring provider as before.
  • Existing callers that do not opt into set_require_fips(true) are not subject to the new FIPS check.
  • The new failure propagation only affects Pingora's own listener-stack build path by turning configuration failures into normal Result errors 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::ServerConfig does 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 fmt
  • cargo check -p pingora-core --features rustls
  • cargo test -p pingora-core --features rustls try_build_returns_error_when_required_fips_is_not_reported

@eldryoth eldryoth force-pushed the rustls-listener-fips-result branch from 9987b73 to 49baf28 Compare May 22, 2026 16:53
@drcaramelsyrup drcaramelsyrup added the enhancement New feature or request label May 22, 2026
@drcaramelsyrup
Copy link
Copy Markdown
Collaborator

@nojima @fabian4 , if you have time, would you be able to review this PR?

tls: self
.tls
.take()
.map(|tls| tls.try_build().map(Arc::new))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the other TLS backends need try_build() too? This path can use their TlsSettings as well.

@53v3n3d4
Copy link
Copy Markdown

I may be wrong, but for this listener FIPS work the digest in hash_certificate used in TlsStream can't continue to use ring. ServerConfig will use the crypto provider passed but seems ring will be hashing certificate...

@eldryoth
Copy link
Copy Markdown
Author

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 -p pingora-memory-cache --lib, while this PR only changes rustls/listener code paths:

  • pingora-core listener TLS backend construction
  • pingora-core rustls TlsStream digest handling
  • pingora-rustls certificate hash helpers

The memory-cache target passes locally, and the relevant checks for this PR pass:

  • cargo check -p pingora-core
  • cargo check -p pingora-core --features rustls
  • cargo check -p pingora-core --features s2n
  • cargo check -p pingora-core --features openssl
  • cargo test -p pingora-core --features rustls try_build_returns_error_when_required_fips_is_not_reported

The same CI matrix appears to pass on Rust 1.84.0 and 1.91.1; only nightly reports the unrelated pingora-memory-cache --lib failure.

@53v3n3d4
Copy link
Copy Markdown

@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.

@eldryoth
Copy link
Copy Markdown
Author

Good question. I checked this, and this PR does not make ring disappear from the dependency tree.

Today pingora-rustls still has a direct ring dependency and enables rustls with the ring feature, so cargo tree still includes ring. This PR is narrower: it lets applications pass an explicit rustls CryptoProvider, requires the final ServerConfig::fips() when requested, and avoids using ring for the rustls listener certificate digest path when a provider-backed SHA-256 implementation is available.

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 require_fips(true).

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 pingora-rustls crypto backend features configurable and remove/gate the direct ring fallback.

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 ServerConfig is built inside Pingora. In our downstream vendored Pingora patch we therefore had to add a final fail-closed panic if the listener config does not report ServerConfig::fips():

https://github.com/valkyoth/fluxheim/blob/main/vendor/pingora-core/src/listeners/tls/rustls/mod.rs#L92-L99

But release builds use panic = "abort", so this is intentionally process-killing rather than allowing startup with a non-FIPS listener: https://github.com/valkyoth/fluxheim/blob/main/Cargo.toml#L166-L168

Our side that wires this policy into Pingora is here:
https://github.com/valkyoth/fluxheim/blob/main/src/runtime.rs#L1113-L1135

It sets the selected rustls crypto provider and then calls set_require_fips(fips_required). The problem is that Pingora’s listener build path was not fallible, so downstreams could not turn the final ServerConfig::fips() check into a normal startup error.

@53v3n3d4
Copy link
Copy Markdown

Thanks for reply and good project. I didn't know the fips in ServerConfig but remembered me the aws_lc_rs::try_fips_mode() which maybe rustls uses it. Also returns a bool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants