Skip to content

Fix grpc-master-x86_64 integration test #3346

Open
nhatnghiho wants to merge 3 commits into
aws:mainfrom
nhatnghiho:fix-grpc-master
Open

Fix grpc-master-x86_64 integration test #3346
nhatnghiho wants to merge 3 commits into
aws:mainfrom
nhatnghiho:fix-grpc-master

Conversation

@nhatnghiho

@nhatnghiho nhatnghiho commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Issues:

Resolves P468646736

Description of changes:

gRPC's new certification selection offloading feature (code change) is causing our grpc-master-x86_64 integration test to fail. This PR fixes that.

  • gRPC's cert selection callback does PEM parsing during the TLS handshake, leaving a stale error on the queue. This causes SSL_get_error to misreport a non-fatal result as fatal SSL_ERROR_SSL. BoringSSL avoids this because a later credential selection loop incidentally clears the error queue. AWS-LC doesn't have this loop, so we now clear after select_certificate_cb returns success.
  • gRPC also started using SSL_CTX_set1_group_ids / SSL_set1_group_ids which AWS-LC lacked. This PR adds and exposes both.

Testing

New unit tests added. grpc-master passes

@github-actions

Copy link
Copy Markdown
Contributor

🔒 Security ReviewView Report

Please review before merging.

@codecov-commenter

codecov-commenter commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.18%. Comparing base (13fdb9e) to head (b3c7928).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
ssl/ssl_lib.cc 92.30% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3346   +/-   ##
=======================================
  Coverage   78.18%   78.18%           
=======================================
  Files         693      693           
  Lines      123959   123985   +26     
  Branches    17213    17215    +2     
=======================================
+ Hits        96912    96934   +22     
- Misses      26129    26134    +5     
+ Partials      918      917    -1     

☔ View full report in Codecov by Harness.
📢 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.

@nhatnghiho
nhatnghiho marked this pull request as ready for review July 14, 2026 15:39
@nhatnghiho
nhatnghiho requested a review from a team as a code owner July 14, 2026 15:39
Comment thread ssl/ssl_lib.cc
Comment on lines +2034 to +2036
if (ssl_group_id_to_nid(group_id) == NID_undef) {
return false;
}

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.

BoringSSL's equivalent (check_group_ids) pushes SSL_R_UNSUPPORTED_ELLIPTIC_CURVE when it hits an unknown ID.

Suggest matching upstream:

Suggested change
if (ssl_group_id_to_nid(group_id) == NID_undef) {
return false;
}
if (ssl_group_id_to_nid(group_id) == NID_undef) {
OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_ELLIPTIC_CURVE);
return false;
}

Comment thread ssl/ssl_lib.cc
Comment on lines +2031 to +2034
static bool ssl_check_group_ids(Array<uint16_t> *out_group_ids,
Span<const uint16_t> group_ids) {
for (uint16_t group_id : group_ids) {
if (ssl_group_id_to_nid(group_id) == NID_undef) {

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: BoringSSL also rejects duplicate group IDs here (check_no_duplicates). Duplicates would just get advertised twice in supported_groups, so probably harmless, but worth a decision.

Comment thread ssl/handshake_server.cc
Comment on lines 607 to 608
case ssl_select_cert_retry:
return ssl_hs_certificate_selection_pending;

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.

The default handles the success path, but I think the retry path has the same problem: if the callback returns ssl_select_cert_retry with stale errors on the queue, SSL_do_handshake returns <= 0 and SSL_get_error checks the error queue before rwstate, so the caller sees a fatal SSL_ERROR_SSL instead of SSL_ERROR_PENDING_CERTIFICATE.

Maybe clear in the retry case too?

      case ssl_select_cert_retry:
        ERR_clear_error();
        return ssl_hs_certificate_selection_pending;

Comment thread ssl/ssl_test.cc
server_ctx.get()));
}

TEST(SSLTest, Set1GroupIds) {

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.

Couple of small things:

  • OPENSSL_ARRAY_SIZE(valid_groups) instead of the literal 3/1 counts.
  • This never verifies the setting actually takes effect. Cheap addition: restrict the server to one group, handshake, and check SSL_get_group_id.
  • Might be worth pinning down num_group_ids == 0: BoringSSL explicitly resets to the default list, while we store an empty list that tls1_get_grouplist treats as "use defaults" -- functionally the same, but a test would keep it that way.

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.

3 participants