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.
| - [AWS-LC GitHub](https://github.com/aws/aws-lc) | ||
| - [AWS-LC FIPS 140-3 certificate #4816](https://csrc.nist.gov/projects/cryptographic-module-validation-program/certificate/4816) | ||
| - [oha load testing tool](https://github.com/hatoo/oha) | ||
| - [Implementation branch](https://github.com/cloudfoundry/haproxy-boshrelease/tree/aws-lc-multi) |
There was a problem hiding this comment.
This implementation link 404s - the aws-lc-multi branch does not exist in cloudfoundry/haproxy-boshrelease. The code lives on the fork sap-contributions/haproxy-boshrelease (branch aws-lc-multi) and is proposed upstream as PR #925. Please update this link so reviewers can reach the implementation.
| | `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.
| 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.
| 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.
|
|
||
| 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.
| - Author(s): @hoffmaen | ||
| - Status: Draft | ||
| - RFC Pull Request: | ||
| - Affected Component(s): haproxy-boshrelease |
There was a problem hiding this comment.
Two metadata gaps vs rfc-template.md: the 'Related RFCs' field is missing, and 'RFC Pull Request' is empty. The RFC README process asks you to fill in the PR link. Consider cross-linking rfc-0023-add-cf-supports-for-fips-stemcell under Related RFCs given the FIPS overlap.
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.