Skip to content

Add SSL_use_cert_and_key for per-connection cert/key setting#3114

Merged
geedo0 merged 2 commits intoaws:mainfrom
geedo0:pg-fix
Mar 23, 2026
Merged

Add SSL_use_cert_and_key for per-connection cert/key setting#3114
geedo0 merged 2 commits intoaws:mainfrom
geedo0:pg-fix

Conversation

@geedo0
Copy link
Copy Markdown
Contributor

@geedo0 geedo0 commented Mar 20, 2026

Issues:

Addresses P400736589

Description of changes:

Describe AWS-LC’s current behavior and how your code changes that behavior. If there are no issues this pr is resolving, explain why this change is necessary.

Postgres recently started calling SSL_use_cert_and_key (the per-SSL
variant) which AWS-LC did not implement. This broke the postgres
integration test on both x86_64 and aarch64 with an undefined reference
linker error.

Extract the body of SSL_CTX_use_cert_and_key into a static helper
cert_use_cert_and_key(CERT *) and add SSL_use_cert_and_key as a thin
wrapper, following the established pattern used by SSL_use_PrivateKey,
ssl_set_cert, and cert_set_chain_and_key.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

CI

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

Postgres recently started calling SSL_use_cert_and_key (the per-SSL
variant) which AWS-LC did not implement. This broke the postgres
integration test on both x86_64 and aarch64 with an undefined reference
linker error.

Extract the body of SSL_CTX_use_cert_and_key into a static helper
cert_use_cert_and_key(CERT *) and add SSL_use_cert_and_key as a thin
wrapper, following the established pattern used by SSL_use_PrivateKey,
ssl_set_cert, and cert_set_chain_and_key.
@geedo0 geedo0 requested a review from a team as a code owner March 20, 2026 18:04
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 20, 2026

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.38%. Comparing base (c15e28d) to head (706cac7).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
ssl/ssl_cert.cc 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3114      +/-   ##
==========================================
+ Coverage   78.20%   78.38%   +0.17%     
==========================================
  Files         689      689              
  Lines      122048   122074      +26     
  Branches    17030    17036       +6     
==========================================
+ Hits        95446    95685     +239     
+ Misses      25698    25480     -218     
- Partials      904      909       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@samuel40791765
Copy link
Copy Markdown
Contributor

Seems like postgres is still failing, but the reason's just due to a mismatching error string.

#   Failed test 'pg.conf: connect fails without intermediate for sslmode=verify-ca: matches'
#   at t/004_sni.pl line 72.
#                   'psql: error: connection to server at "127.0.0.1", port 19675 failed: SSL error: CERTIFICATE_VERIFY_FAILED'
#     doesn't match '(?^:certificate verify failed)'
# using temp instance on port 58928 with PID 12462

Should be fixable by just adding 004_sni.pl to the flie list here:

for i in "${!POSTGRES_ERROR_STRING[@]}"; do
find ./ -type f -name "001_ssltests.pl" | xargs sed -i -e "s|${POSTGRES_ERROR_STRING[$i]}|${AWS_LC_EXPECTED_ERROR_STRING[$i]}|g"

Comment on lines +1235 to +1236
// we represent X509 chains as a CRYPTO_BUFFER stack. Therefore, we create a
// an internal copy and leave the |chain| parameter untouched. This means,
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.

NP: "... We create a an internal copy ..."

Comment on lines +1010 to +1012
if (!ssl->config) {
return 0;
}
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.

NP: should we set an error here?

OPENSSL_PUT_ERROR(SSL, ERR_R_PASSED_NULL_PARAMETER);

Comment on lines 252 to 256
if (cert_pkey->chain) {
cert_pkey->chain.reset();
}
cert_pkey->chain = std::move(certs);
cert->cert_private_key_idx = slot_idx;
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.

NP: Maybe for a future PR -- since cert_pkey->chain is being replaced here, shouldn't the cache be invalidated? Otherwise a caller that previously triggered the cache population would see stale data after this call.

  if (cert_pkey->chain) {
    cert_pkey->chain.reset();
  }
  cert_pkey->chain = std::move(certs);

  // Invalidate the parsed X509 chain cache for this slot since the backing
  // CRYPTO_BUFFER chain was just replaced.
  sk_X509_pop_free(cert_pkey->x509_chain, X509_free);
  cert_pkey->x509_chain = nullptr;

  cert->cert_private_key_idx = slot_idx;

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.

See: #3117

@geedo0 geedo0 merged commit 666d5e3 into aws:main Mar 23, 2026
449 of 456 checks passed
@geedo0 geedo0 deleted the pg-fix branch March 23, 2026 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants