RFC: AWS-LC as TLS Backend for HAProxy#1501
Conversation
af72f5e to
b0c5b6d
Compare
b0c5b6d to
d570aaf
Compare
|
Hi @hoffmaen, thank you for this pr. Is it intentionally in draft status? |
Soha-Albaghdady
left a comment
There was a problem hiding this comment.
LGTM, proposing some minor formatting and restructuring
|
@hoffmaen could you please ask in your WG (meeting) about feedback. |
Code ReviewPR #1501: RFC: AWS-LC as TLS Backend for HAProxy SummaryThis RFC proposes adding AWS-LC as an optional TLS backend for the haproxy-boshrelease to address OpenSSL 3.0.2 TLS-handshake performance and the lack of post-quantum key exchange. The problem statement is strong: it is well-evidenced (benchmarks, HAProxy upstream recommendation in haproxy#3086, FIPS 140-3 validation) and the non-breaking, opt-in framing with OpenSSL-as-default is the right compatibility posture. The core AWS-LC value proposition is convincing. The main gap is the packaging design: the RFC introduces a 7-tarball matrix (openssl, openssl-patched, awslc, awslc-patched, awslc-fips, awslc-fips-patched, multi) but only justifies 4 of the 7. The three '-patched' variants are never explained, and there is no analysis of the CI/compilation cost this matrix imposes despite it roughly doubling the build surface. There are also smaller correctness/process issues: the implementation link 404s (the branch lives on a fork as PR #925), the RFC metadata omits fields required by the template, and the document still carries Draft status with an empty RFC Pull Request field. Critical Issues - Must Fix
Important Issues - Should Fix
Minor Issues
Positive Notes
RecommendationRequest changes - critical issues need to be addressed before merging. |
rkoster
left a comment
There was a problem hiding this comment.
Requesting changes - see the detailed review comment above. Main blockers: the -patched variant axis is unjustified and there is no CI/build-cost analysis for the expanded matrix. Important: broken implementation link, missing testing strategy (impl branch changes no tests), and undefined cf-deployment integration.
| | `awslc` | AWS-LC | High-performance, non-FIPS | | ||
| | `awslc-patched` | AWS-LC | With custom HAProxy patches | | ||
| | `awslc-fips` | AWS-LC FIPS 3.3.0 | FIPS 140-3 validated | | ||
| | `awslc-fips-patched` | AWS-LC FIPS | FIPS with custom HAProxy patches | |
There was a problem hiding this comment.
The '-patched' axis (openssl-patched, awslc-patched, awslc-fips-patched) is introduced here but never explained anywhere in the RFC. What are the 'custom HAProxy patches', why are they needed, and who needs them? This axis doubles the variant count from 3 to 6 (and doubles the multi release's HAProxy binaries), which is the main driver of the added build/CI cost. Please justify each patched variant explicitly, or defer them to a follow-up if they are not required for the initial rollout.
There was a problem hiding this comment.
Thanks for the detailed review.
The RFC now has a "Custom HAProxy Patches (the -patched variants)" subsection under Release Variants. It documents what the patch does (relaxes RFC 6455 Sec-WebSocket-Key enforcement in the H1 mux), why it's required in some scenarios (legacy clients opening long-lived WebSocket connections without a spec-compliant key), why it can't be the default (it makes HAProxy intentionally non-compliant with RFC 6455), and why it ships as a separate variant rather than a runtime toggle (BOSH properties are deploy-time, not compile-time, and BOSH won't recompile on a property change, so a compile-time patch toggle isn't expressible as a property). The patch is a long-standing carried requirement, not new to any HAProxy version.
| aws-lc-fips → AWS-LC FIPS library (requires Go toolchain for delocate) | ||
| haproxy-openssl → HAProxy binary linked to system OpenSSL | ||
| haproxy-awslc → HAProxy binary linked to AWS-LC | ||
| haproxy-awslc-fips → HAProxy binary linked to AWS-LC FIPS |
There was a problem hiding this comment.
This build architecture makes the CI cost concrete: the multi release compiles 6 HAProxy binaries plus aws-lc, aws-lc-fips, cmake, and a Go toolchain, and six additional single-variant tarballs are produced on top. The RFC gives only ~15 min / ~25 min per-variant figures but never totals the pipeline time, artifact count/size, or per-variant BOSH test-matrix growth. For a shared community release this recurring cost is a first-order design concern. Please add a Costs / CI-impact section quantifying it and use it to justify the matrix size.
There was a problem hiding this comment.
Added a "Costs and CI Impact" section. The key correction: a BOSH release tarball only ships source + blobs. The actual HAProxy/AWS-LC compilation happens on the target VM at deploy time, and for the single-variant releases each operator compiles only the one variant they deploy. So extra variants impose no compile cost on operators who don't use them (the multi release is the only thing that compiles everything, and it's explicitly a migration/testing artifact). The incremental cost is confined to the Concourse release-build pipeline: all the extra variants together add a few minutes of pipeline wall-clock plus the disk to store the extra tarballs. Given we cut 9 releases in H1 2026, that recurring cost is negligible. The acceptance-test matrix is per-backend (awslc, awslc-fips), so it doesn't grow with the -patched axis.
| The job wrapper resolves the binary path at startup: | ||
|
|
||
| 1. If `/var/vcap/packages/haproxy-<ssl_variant>` exists → use variant binary (multi release) | ||
| 2. Otherwise → fall back to `/var/vcap/packages/haproxy` (single-variant release) |
There was a problem hiding this comment.
What happens on a single-variant release if an operator sets ha_proxy.ssl_variant to a valid-but-absent variant? As written, the fallback to /var/vcap/packages/haproxy could silently run a different TLS backend than the operator intended (e.g. they believe they are on awslc-fips but are actually on openssl). Recommend failing fast with a clear error instead of silent fallback, and stating that behaviour here.
There was a problem hiding this comment.
Good catch - the silent fallback is now documented as a configuration error that must fail fast.
|
|
||
| In practice this has no client impact: DHE is rarely required by modern clients, and ECDHE provides equivalent or better security with superior performance. Existing cipher configurations that include both DHE and ECDHE ciphers continue to work — HAProxy silently skips DHE entries that AWS-LC cannot fulfill, and clients negotiate ECDHE instead. | ||
|
|
||
| **Compatibility note**: A client would only break on AWS-LC if it supports **DHE but not ECDHE**. Such stacks predate ~2010 and also predate TLS 1.2. Under a TLS 1.2-minimum policy (already enforced in this deployment), they are filtered out at the protocol layer regardless of cipher selection — every client that can negotiate TLS 1.2 also supports ECDHE. Clients that currently happen to negotiate DHE will transparently fall back to ECDHE with no visible impact. |
There was a problem hiding this comment.
These behavioural claims (DHE absent, clients transparently fall back to ECDHE, existing mixed cipher configs keep working) are exactly the kind of thing that needs an automated test - but the implementation branch (aws-lc-multi) changes no acceptance or rspec tests at all (the existing suite has https_frontend_test.go, mtls_frontend_test.go, strict_sni_test.go). Please add a Testing section covering which variants get automated coverage and what it asserts: cipher/curve negotiation, DHE->ECDHE fallback, SNI + mTLS regression, ssl_variant selecting the correct binary, and a FIPS-active assertion for the FIPS variants. Separately, please clarify the cf-deployment expectation - haproxy is opt-in there via operations/use-haproxy.yml (one pinned release), so state whether a companion ops-file / ssl_variant wiring is planned or whether cf-deployment integration is out of scope.
There was a problem hiding this comment.
Regarding cf-deployment integration: Added a short cf-deployment Integration subsection stating this is out of scope. haproxy-boshrelease is opt-in in cf-deployment via use-haproxy.yml (auto-bumped release version); operators adopting AWS-LC manage their own release pinning outside that flow, so no cf-deployment change is required here. The RFC notes that first-class support could later be added via a variant selector in use-haproxy.yml that still tracks automatic version bumps, but that is a cf-deployment-side change and out of scope for this RFC.
There was a problem hiding this comment.
Added a Testing section. The core approach is to run the existing acceptance suite (HTTPS over TLS 1.2/1.3, ALPN, mTLS, strict SNI, WebSocket, + ~20 feature tests) against the AWS-LC-linked binary - since those already drive the full TLS data path, they're the real regression guarantee that swapping the TLS library preserves behaviour, and they directly cover the SNI + mTLS points. The one new test is an rspec spec for the ssl_variant binary selector.
We deliberately diverged on a few of the specific asks:
- DHE→ECDHE fallback: removed, because the original "fallback" claim was wrong — there is no fallback. Under the shipped default cipher order (ECDHE ahead of DHE, via SSL_OP_CIPHER_SERVER_PREFERENCE), an ECDHE-capable client already negotiates ECDHE, so a client still on DHE has no ECDHE alternative and simply fails on AWS-LC. The RFC now states this accurately, so there's no fallback behaviour to test.
- Per-cipher/curve negotiation tests: not added — the suite's style is functional (drive a real client, assert success), not primitive-level assertions; the differences are documented and bounded by the recommended config instead.
- FIPS-active acceptance run: not added — each acceptance run recompiles the release (~20–30 min for an AWS-LC variant), so a second full run for awslc-fips roughly doubles cost for little signal given FIPS is a library property; FIPS activation is verified out-of-band during release validation.
Note the implementation branch is still draft and will follow the RFC, so the test changes land on the implementation PR rather than being present now.
56c430c to
fa70885
Compare
Link to the document for quick review: rfc-draft-haproxy-awslc.md
Summary
This PR adds the RFC for AWS-LC as TLS Backend for the haproxy-boshrelease.
AWS-LC is independent of the version provided via the BOSH stemcell and can be kept up to date interdependently. The OpenSSL that is currently used via the stemcell is on an outdated version that has severe performance impacts in high-load scenarios. These would be improved in newer OpenSSL versions, but not resolved. HAProxy explicitely recommends against OpenSSL in production (haproxy/haproxy#3086).
AWS-LC alleviates these issues and is proposed to be added as an optional release. For operators nothing changes, unless they want to opt into using AWS-LC.
AWS-LC is FIPS 140-3 compliant.