[DRAFT] Upstream merge 2024-10-23 #1955
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If we move SSL_certs_clear to ssl_cert.cc, ssl_cert_clear_certs does not need to be in the header. Moreover, its only other caller, ~CERT(), does not need to call it. Now that everything outside of SSL_X509_METHOD is managed with scopers, the destructor does it automatically. And cert_free on SSL_X509_METHOD already automatically calls cert_clear, so it's a no-op to do it again. Change-Id: Ief9c704cc45440288783564ac4db4a27fbec1bfc Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66370 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
No one would ever do this, but it works today and we should keep it working. As part of certificate selection, we'll be introducing an SSL_CREDENTIAL object. In doing so, the existing APIs will mutate a built-in "default" credential. If we made a shallow copy, it would break things. Bug: 249 Change-Id: I75b1486289659611184a42e87771a6cf7ddb5aa7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66372 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Now that delegated credentials comes with its own sigalg list (hooray for wasted ClientHello bytes), we don't need a delegated_credential_requested. It's already implicit in whether we parsed any sigalgs. Change-Id: I5169e4b24a41dd4973fc581087c881d34b5075fa Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66373 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> (cherry picked from commit c06c4d5ea6dc3118c2851e0010aa441161e2a983)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1955 +/- ##
=======================================
Coverage ? 79.01%
=======================================
Files ? 587
Lines ? 101303
Branches ? 14360
=======================================
Hits ? 80049
Misses ? 20598
Partials ? 656 ☔ View full report in Codecov by Sentry. |
Avoid rewriting the FILE scoper, and deal with the Android problem in one place. This header will also, in the next CL, be the home for a temporary directory helper for hash_dir. Change-Id: I4be69ef6c2ac3443b80ee8852bcce4078bf7f118 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66007 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Writing these tests revealed that actually this has been broken on Windows this whole time! First, the APIs to configure a directory actually configure a colon-separated list of directories. But file paths contain colons on Windows, so the actual separator on Windows is semicolon. But that got lost in the fork at some point. This CL fixes that. Second, X509_FILETYPE_ASN1 is broken because of a text vs binary mode mixup. The following CL will fix this. Some of the behaviors tested here around CRLs are not quite reasonable. See https://crbug.com/boringssl/690 for details. For now, I've tried to capture the existing behavior. As BY_DIR actually maintains some shared mutable state, I've also added TSAn tests. Another subtlety is that multiple CAs with the same name work, but the reason they work is pretty messy because OpenSSL's internal interfaces are incompatible with it. Instead, OpenSSL works around itself with the X509_STORE cache. These tests do not cover this case, but a subsequent CL will add tests for it. Change-Id: Ifd8f2faea164edb0eda771350cd9bf6dc94104e7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66008 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
BIO_*_filename, in upstream OpenSSL, opens in binary mode on Windows, not text mode. We seem to have lost those ifdefs in the fork. But since C mandates the 'b' suffix (POSIX just ignores it), apply it consistently to all OSes for simplicity. This fixes X509_FILETYPE_ASN1 in X509_STORE's file-based machinery on Windows. Also fix the various BIO_new_file calls to all specify binary mode. Looking through them, I don't think any of them care (they're all parsing PEM), but let's just apply it across the board so we don't have to think about this. Update-Note: BIO_read_filename, etc., now open in binary mode on Windows. This matches OpenSSL behavior. Change-Id: I7e555085d5c66ad2f205b476d0317570075bbadb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66009 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> (cherry picked from commit cadebfd6398e017addaae4878662aadb42f60bda)
…:CopyFrom This (internal) abstraction was originally made for trivial types, but if we ever got a complex type, we should use C++ copies, not C copies, to preserve all the rules of that type. A good STL will specialize std::copy to memmove/memcpy when possible, so this should not appreciably change the generated code. Change-Id: I76af334ef667e545dbbbe87315ce5b30a327358c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66427 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> (cherry picked from commit 6db6604cb507f4dda3c60ccc803f19338a3e7204)
justsmth
force-pushed
the
upstream-merge-2024-10-23
branch
3 times, most recently
from
ab83a00
to
817ddd7
Compare
Different BY_DIR_ENTRYs don't need to share a lock. Also switch some code to use OPENSSL_strndup. Change-Id: I3809e001afb9577bb96aab214e80e173900356fe Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66012 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit bca5875eb3c25348ec07758cde66ebec27031ce4)
This produces slightly nicer output, is less code, and helps us remember to check both the library and reason code. Change-Id: Ic49508accb0bc8a25cbb5b94cc7e4aeb1bd8cbd0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66507 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit ec2a08dda8e7b156253abcf21f2c7ea80de39f82)
While I'm here, unexport STACK_OF(X509V3_EXT_METHOD). We use it internally, but it never appears in any public APIs, and there's no real reason for any caller to use it. Bug: 426 Change-Id: I6057834847a37f435d1b687701a3e65b5afb2890 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66387 Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]> (cherry picked from commit 9d7535f51f84a079c05b27134fcf6111649c56c9)
This includes the somewhat odd "defaults" API, which I've currently left kind of handwavy. We should eventually decide what to do with this, be it remove it, decide /etc/ssl is a fine default, or do something else entirely. But I'll leave that to future us. (If nothing else, we really should make it return an error on Windows and macOS. It's really just Linux where /etc/ssl is a plausible platform API.) Bug: 426 Change-Id: Iacd2bb903f452ffe236a7a0b97e3072b5dcd8516 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66388 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> (cherry picked from commit ae1c1a482588ab9b6c5f97b8663cdc50ac2444dd)
Many of the extension types are not the extensions themselves, but the interious types used in various subfields. In preparation for when we rewrite these parsers with <openssl/bytestring.h>, having fewer of these means fewer compatibility functions to bridge the calling conventions. We do still need new/free functions, so that callers can construct extensions themselves. While I'm here, go ahead and expand the macros and document. (Top-level extension types need ASN1_ITEMs for X509V3_METHOD, and d2i/i2d functions for callers that wish to parse and serialize. But there's no real need to do this for the individual fields.) Update-Note: Some interior ASN.1 types no longer have d2i and i2d functions or ASN1_ITEMs. I checked code search and no one was using any of these. We can restore them as needed. Bug: 547 Change-Id: I0b2840bf4aea2212a757ce39b4918c8742043725 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66389 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit 7f151ca07d67d1eb51289169ffc51fe3b38f878d)
They're still in the "underdocumented" section for ease of review. I wanted to separate out expanding the macros from moving things around. Bug: 426 Change-Id: Ib5fcedf180b478d5552113025d9353d29bb1961f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66390 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit 4066ebc79f07c8b639fa1ca3f26aa2509727831d)
This is mostly all repetitive text, but a couple structures with unions deserve special warning. The "ADB" (ANY DEFINED BY) stuff is pretty scary. Bug: 426 Change-Id: I85d27dd4e4676cf51c30529c53b6f2867c205caf Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66391 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]> (cherry picked from commit db614a5677d90e48cfb2c0f8197f1b5168fceea5)
justsmth
force-pushed
the
upstream-merge-2024-10-23
branch
from
817ddd7
to
536f09b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
See internal notes.
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.