Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slight tweaks and integration CI to support Bind9 #1423

Merged
merged 6 commits into from
Feb 16, 2024

Conversation

samuel40791765
Copy link
Contributor

@samuel40791765 samuel40791765 commented Jan 26, 2024

Issues:

Addresses CryptoAlg-2162

Description of changes:

  1. Integration CI dimension for Bind9

  2. Resolved "cmocka unit tests" for Bind9

    • Additional <openssl/asn1.h> import in <openssl/objects.h>: Bind depends on some ASN1 functions, but does not directly import the corresponding header. OpenSSL imports the asn1 header file in objects.h (which Bind is pulling these symbols from), so I've added the header file reference to objects.h.

    • SSL_get_error error anticipation fixing: There were several failures discovered to be related this, thanks to research done in Implement SSL_MODE_AUTO_RETRY #1333 and dumping out the logs from Bind9 built with OpenSSL and AWS-LC. The issue was pinned down the check implemented in google/boringssl@9a38e92. This check used to exist before the final return of SSL_get_error in OpenSSL. BoringSSL moved this earlier in the function with google/boringssl@fcf2583. However, much of the functions guards for i < 0 checks have been removed since OpenSSL 1.1.1, so the early logic no longer applies.
      This check has evolved into SSL_ERROR_ZERO_RETURN in our code. Moving the logic further down helps us gain better parity with OpenSSL 1.1.1. Doing so passes the bind test failures for proxystream_test, tls_test, and doh_test. This also happens to help our integration with CPython.

    • We actually already use SSL_AUTO_RETRY by default in AWS-LC. The recent change mentioned in the point above surrounding the flag (208327e) was just to make some of the errors consistent in CPython when the flag was used. I've reverted the special behavior surrounding it since it should no longer be needed.

    • Assertion for SSL_set_shutdown: The assertion was added in 63006a9, where it’s stated that they didn’t want SSL_set_shutdown messing up the state machine. This assertion is causing failures in tlsdns_test for Bind9, so it appears that we'll have to remove this to gain better OpenSSL parity. This assertion is non-existent in OpenSSL today.

  3. Patch file needed for Bind seems to be slight bug in their build configuration. This was from a fairly recent commit. We can look to contribute this sometime soon.

Call-outs:

  • We don't need SSL_AUTO_RETRY anymore to support CPython, so I've removed and regenerated the patch. This simplifies our support for CPython.
  • There's still an additional test suite for Bind9 (system tests) that has failures we'll need to resolve. The plan is to investigate this as a subsequent PR.

Testing:

New CI dimension

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.

@samuel40791765 samuel40791765 force-pushed the bind-support branch 5 times, most recently from 60d7511 to ebf4e19 Compare January 26, 2024 01:04
@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ddc449d) 76.83% compared to head (68cc907) 76.86%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1423      +/-   ##
==========================================
+ Coverage   76.83%   76.86%   +0.03%     
==========================================
  Files         425      425              
  Lines       71535    71526       -9     
==========================================
+ Hits        54962    54981      +19     
+ Misses      16573    16545      -28     

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

@samuel40791765 samuel40791765 force-pushed the bind-support branch 3 times, most recently from 7c5cae9 to db6a639 Compare February 1, 2024 01:20
@samuel40791765 samuel40791765 marked this pull request as ready for review February 1, 2024 01:22
@samuel40791765 samuel40791765 requested a review from a team as a code owner February 1, 2024 01:22
ssl/ssl_lib.cc Outdated Show resolved Hide resolved
ssl/ssl_lib.cc Show resolved Hide resolved
tests/ci/integration/run_bind9_integration.sh Outdated Show resolved Hide resolved
tests/ci/integration/run_bind9_integration.sh Show resolved Hide resolved
ssl/ssl_lib.cc Show resolved Hide resolved
tests/ci/integration/run_bind9_integration.sh Show resolved Hide resolved

proxystream_test_CPPFLAGS = \
- $(AM_CPPFLAGS)
+ $(AM_CPPFLAGS) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any idea how this works with OpenSSL and Bind right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bind normally depends on pkgconfig to tell it where the correct artifacts are. All other tests in the same file follow this pattern.
I'm suspecting that their CI is relying on the system's installed OpenSSL package referenced in some global flags, so this isn't failing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we install our package config?

Copy link
Contributor Author

@samuel40791765 samuel40791765 Feb 15, 2024

Choose a reason for hiding this comment

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

Yeah we install our binaries with pkgconfig, but it's not in the standard global path. Without the flags here the test binary still won't find the right artifacts.

aws-lc/CMakeLists.txt

Lines 1137 to 1139 in ddc449d

# AWS-LC may be installed in a non-standard prefix. If OpenSSL exists in the standard path,
# the downstream integration may build with the system's OpenSSL version instead.
# Consider adjusting the PKG_CONFIG_PATH environment to get around this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a built in way to tell package config to look for other package config in a different location?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the PKG_CONFIG_PATH environment variable was pkgconfig's own recommendation to get around this. I added the comment based off of that.
We use this in all our integration ci scripts that use pkgconfig.

@samuel40791765 samuel40791765 merged commit 171ee7a into aws:main Feb 16, 2024
39 checks passed
@samuel40791765 samuel40791765 deleted the bind-support branch February 16, 2024 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants