Update to upstream df3b58e #109
Merged
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
These probably shouldn't be public API, but ah well. Bug: 426 Change-Id: I4c5a81c70d3b2d5866ef494ac2a6710a662103c8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63947 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
X509_INFO only exists to be a return value to PEM_X509_INFO_read. There is no use in letting callers create these objects, since they cannot do anything with it. Only X509_INFO_free is needed. Also cut a ton of unused fields from X509_PKEY. Change-Id: I322589f04883903e1fe5c23c3966ecf631e85b7f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64127 Commit-Queue: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Before making further changes. This pass is without InsertBraces, will follow up with InsertBraces for separate review Bug: 659 Change-Id: Ie311dcd932aec5f98c16573f9326b1918a639adf Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64067 Commit-Queue: David Benjamin <[email protected]> Auto-Submit: Bob Beck <[email protected]> Reviewed-by: David Benjamin <[email protected]>
Bug: 659 Change-Id: I48eeda0bcd0de45d70644c321138225f83cb6c60 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64107 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Bob Beck <[email protected]> Auto-Submit: Bob Beck <[email protected]>
Change-Id: I553dc69083878bb33d0a62f512622d77be9cdee9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64068 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Bob Beck <[email protected]>
Change-Id: Ifc0c3cdec56ff9d50b49da3484bb6c34b32b7b97 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64087 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Bob Beck <[email protected]>
This was never used externally. It's a remnant of when we supported stack-allocated X509_STOREs, but now its opaque. Change-Id: Idb997237ca81f4c35795cfc8c9d2ee222629e1ce Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64128 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
We move the last piece of used stuff here into the library string_utils. Bug: 668 Change-Id: Idde14a497204a3b40a602ed6c03a5859eee80811 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64167 Auto-Submit: Bob Beck <[email protected]> Commit-Queue: Adam Langley <[email protected]> Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Bob Beck <[email protected]>
Move the file reader into the anonymous namespace in test_helpers which is the only user. Bug: 668 Change-Id: Idd650d14fb7f9e0b7b15a7fd08e21f9a7081cc14 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64168 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Bob Beck <[email protected]> Auto-Submit: Bob Beck <[email protected]>
Move these functions into the library bssl::string_util namespace Bug: 668 Change-Id: I1665176217fb25164cb321a4092391bf60592a88 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64187 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: Bob Beck <[email protected]> Auto-Submit: Bob Beck <[email protected]>
It ceased being a service long ago. The relevant piece to get the test data is moved to the anonymous namespace in test_helpers which it the only remaining user. Bug: 668 Change-Id: I23d8a7166ed61e83f12a7e82473beec316c56d86 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64169 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Adam Langley <[email protected]>
… context This wasn't possible when X509_STORE_CTX was stack-allocated because X509_STORE_CTX_init needed to account for an uninitialized struct. But now it is always initialized, so we can avoid this footgun. This also matches what OpenSSL does nowadays. Change-Id: I266be58204b8cd374fa4896c1c66a35ffaa762ea Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64141 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
This is a bit of a mess. The immediate motivation here is that there is no legitimate reason to ever call X509_OBJECT_free_contents outside of the library. Unsurprisingly, this means rust-openssl uses it. rust-openssl uses it because they want to be able to free X509_OBJECTs. Add OpenSSL 1.1.x's X509_OBJECT_free, which is what they should be using it instead. As it turns out, they don't *actually* need to free X509_OBJECTs. This is just some design mistake that cause them to need free functions for types they never free. On top of that, the only reason rust-openssl references X509_OBJECT is for X509_STORE_get0_objects, but their use of that API is a Rust safety violation anyway. It's all a mess. As for whether freeing it ever makes sense, the question is whether X509_STORE_get_by_subject needs to be a public API. In so far as it is public, callers would need to create empty X509_OBJECTs as an output, now that X509_OBJECT is opaque. There are also other users of X509_STORE_get0_objects that might benefit from an X509_STORE_get1_objects, in which case X509_OBJECT_free will be useful. For now just to unblock fixing the more immediate rust-openssl mistake (rather than the underlying mistake), add the APIs that X509_STORE_get_by_subject callers would need if they existed. There's quite a bit to clean up around X509_OBJECT, but start by adding these APIs. As part of this, since rust-openssl prevents us from removing X509_OBJECT_free_contents, deprecate it and fix it to leave the X509_OBJECT in a self-consistent state. (This is moot because rust-openssl will never call it, but still.) Change-Id: I78708f2d2464eb9a18844fef0d62cb0a727b9f47 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64129 Reviewed-by: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Some things of note: - Anyone calling X509_OBJECT_up_ref_count is breaking X509_OBJECT's internal invariants, or relying on someone else handing back an X509_OBJECT with broken invariants. - X509_LOOKUP_by_subject hands back an X509_OBJECT with broken internal invariants. Fortunately, it is never called, so unexport it as a the first step to cleaning this up. Change-Id: Ia67693f802671cf857bf51aec6e20f27d1525212 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64130 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
x509.h isn't ready for doc.go yet, but fix a few mistakes caught by previewing it. Bug: 426 Change-Id: I79630cc1cbe5737cea96143b54c2fa42882077a0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64140 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Change-Id: I8d1d3578e0e05757744b905689939708a9353e8d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64131 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
The only user of the tiny utility class to clear the error stack was verify_certificate_chain, which can have it in it's anonymous namespace. It was however, used to great effect as a horrible dirty cheat to get us openssl/base.h during the conversion. That time is past, so just let us include <openssl/base.h> normally, and get rid of this. Bug: 668 Change-Id: Ie8537d5b1d6789acb35182f3d781f7505ad79109 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64188 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: Bob Beck <[email protected]> Auto-Submit: Bob Beck <[email protected]>
It has one in upstream OpenSSL. The most recent OpenSSL release is hitting a compatibility issue with postgres, which seems like it'll get fixed by postgres using BIO_get_app_data. Add it on our end too. https://www.postgresql.org/message-id/CAN55FZ1eDDYsYaL7mv%2BoSLUij2h_u6hvD4Qmv-7PK7jkji0uyQ%40mail.gmail.com Homebrew/homebrew-core#155651 Change-Id: I5bf226cc3506a114cd62f885a8c15006512dfc65 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64227 Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
Although this is only used by <openssl/pem.h>, one existing caller expects the free functions to be defined in <openssl/x509.h>. It's not really worth it to put it in the other header, so just move it back. Change-Id: I7e719d51110b567296fcd797f72d13aa41de73af Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64287 Commit-Queue: David Benjamin <[email protected]> Commit-Queue: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Change-Id: If817b912dd016672b7920e1c2e68244d418099d9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64288 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Other than the verify callback, these are practically unused. The one exception is that gRPC have used a couple of the CRL functions, though in somewhat questionable ways. Keep those for now, but we need to work with them to fix their code. There's also a bit of mess around check_issued, largely due to further rust-openssl misunderstandings. Update-Note: Removed a bunch of unused X509_STORE callback functions. We can restore them if someone was using them. Change-Id: I9c47581784c56a4c3b762e603a20ad7d5d612c65 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64133 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
There are quite a lot of these, and I haven't organized them into sections yet or anything, but get some easy ones. While I'm here, also do the check_private_key functions. They're pretty easy. Bug: 426 Change-Id: I1a5217d27908255833c350893bc3180ced82b0d0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64134 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
FIPS 186-2 included some ad-hoc PRF, based on SHA-1's internals, to generate some numbers for DSA, though nothing uses it for DSA anymore. However, it made its way into EAP-SIM and EAP-AKA. Some applications implement it by reaching into the SHA_CTX structure. https://boringssl-review.googlesource.com/c/boringssl/+/63967 broke some of those callers. Rather than revert it, just add a support API for this PRF and we'll move that caller to it. Reference: https://csrc.nist.gov/files/pubs/fips/186-2/upd1/final/docs/fips186-2-change1.pdf Bug: 566, 667 Change-Id: I5821811f15f20f9f43165fcda23befad03ff277a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64307 Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Bob Beck <[email protected]> Auto-Submit: David Benjamin <[email protected]>
I'm getting tired of having to rederive the best way to convince the compiler to emit addc and subb functions. Do it once and use the Clang builtins when available, because compilers seem to generally be terrible at this. (See llvm/llvm-project#73847.) The immediate trigger was the FIPS 186-2 PRF, which completely doesn't matter, but reminded me of this mess. As far as naming and calling conventions go, I just mimicked the Clang ones. In doing so, also use the Clang builtins when available, which helps Clang x86_64 no-asm builds a bit: Before: Did 704 ECDH P-384 operations in 1018920us (690.9 ops/sec) Did 1353 ECDSA P-384 signing operations in 1077927us (1255.2 ops/sec) Did 1190 ECDSA P-384 verify operations in 1020788us (1165.8 ops/sec) Did 784 RSA 2048 signing operations in 1058644us (740.6 ops/sec) Did 34000 RSA 2048 verify (same key) operations in 1011854us (33601.7 ops/sec) Did 30000 RSA 2048 verify (fresh key) operations in 1005974us (29821.8 ops/sec) Did 7799 RSA 2048 private key parse operations in 1061203us (7349.2 ops/sec) Did 130 RSA 4096 signing operations in 1082617us (120.1 ops/sec) Did 10472 RSA 4096 verify (same key) operations in 1082857us (9670.7 ops/sec) Did 9086 RSA 4096 verify (fresh key) operations in 1039164us (8743.6 ops/sec) Did 2574 RSA 4096 private key parse operations in 1078946us (2385.7 ops/sec) After: Did 775 ECDH P-384 operations in 1008465us (768.5 ops/sec) Did 1474 ECDSA P-384 signing operations in 1062096us (1387.8 ops/sec) Did 1485 ECDSA P-384 verify operations in 1086574us (1366.7 ops/sec) Did 812 RSA 2048 signing operations in 1043705us (778.0 ops/sec) Did 36000 RSA 2048 verify (same key) operations in 1005643us (35798.0 ops/sec) Did 33000 RSA 2048 verify (fresh key) operations in 1028256us (32093.2 ops/sec) Did 10087 RSA 2048 private key parse operations in 1018067us (9908.0 ops/sec) Did 132 RSA 4096 signing operations in 1033049us (127.8 ops/sec) Did 11000 RSA 4096 verify (same key) operations in 1070502us (10275.6 ops/sec) Did 9812 RSA 4096 verify (fresh key) operations in 1047618us (9366.0 ops/sec) Did 3245 RSA 4096 private key parse operations in 1083247us (2995.6 ops/sec) But this is also a no-asm build, so we don't really care. Builds with assembly, broadly, do not use these codepaths. The exception is the generic ECC code on 32-bit Arm, which has a few mod-add functions, and we don't have 32-bit Arm bn_add_words assembly: Before: Did 168 ECDH P-384 operations in 1003229us (167.5 ops/sec) Did 330 ECDSA P-384 signing operations in 1076600us (306.5 ops/sec) Did 319 ECDSA P-384 verify operations in 1080750us (295.2 ops/sec) After: Did 195 ECDH P-384 operations in 1026458us (190.0 ops/sec) Did 350 ECDSA P-384 signing operations in 1005392us (348.1 ops/sec) Did 341 ECDSA P-384 verify operations in 1008486us (338.1 ops/sec) Change-Id: Ia3fa51e59398224b9c39180e1d856bb412aa1246 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64309 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Clang seems to be undoing the constant-time code here. This is another constant-time table lookup, so this is more of the usual problem. Found with the valgrind-based tooling. I didn't switch this to constant_time_conditional_memxor since we still haven't figured out https://crbug.com/boringssl/655, but we definitely need some kind of abstraction for this pattern. Change-Id: Ic11873b23cde31375ac1a326ed09ac1ca53ec913 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64310 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
The getter and setter are never used, largely because named parameters don't do anything. The field only exists for X509_VERIFY_PARAM_lookup, where we have to cast away const because the library expects to have to free the string. Just replace X509_VERIFY_PARAM_lookup with a handful of strcmp calls. As part of this, merge the pkcs7 and smime_sign entries. They were identical. Update-Note: Removed some unused functions. Change-Id: If4eaad52b50d28b83335f6b545af81063e0756d7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64135 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Although new and free share a zero function, most of the function is irrelevant to both (either because we're about to free the object or have allocated it with zalloc), or only one of them. While I'm here, remove some unnecessary NULL checks guarding free functions. Change-Id: I9906ebc16c931f51b6341958bcf0a10426a1211b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64136 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
The public headers are not yet merged. That will be doen in the subsequent CL. This required teaching make_errors.go that x509v3 are found elsewhere, also to skip irrelevant OPENSSL_DECLARE_ERROR_REASON calls. Change-Id: Ic40de51f9a5325acd60262c614924dc3407b800c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64137 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Change-Id: I53147d1f96d1f99909f5c8bda00cefb088677a0e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64138 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Our repository is sometimes copied into other repositories, at which point all the absolute links don't work. README.md used "./whatever", which seems to work most consistently, so copy that. Change-Id: I63bbf01b8f3870112d1df571adaa6cc82f58e5eb Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64347 Commit-Queue: David Benjamin <[email protected]> Auto-Submit: David Benjamin <[email protected]> Commit-Queue: Bob Beck <[email protected]> Reviewed-by: Hubert Chao <[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]>
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]>
…: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]>
If the file doesn't exist, we'll fail when we go to read it anyway. The stat call just adds a needless ifdef here. Change-Id: I00a52f988bc1d45622e559b496ef2293b3719863 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66011 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
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]>
It is not actually possible to configure an inconsistent certificate and private key pair (short of mutating the objects after you've configured them). The functions that configure certificates and private keys will refuse to get CERT into an inconsistent state. SSL_CTX_check_private_key is really just checking that you have a certificate and private key at all. Some callers (notably pyOpenSSL's tests) are written as if SSL_CTX_check_private_key does something more, but that's only because they also configure certificate and private key in the wrong order. If you configure the key first, configuring the certificate silently drops the mismatched private key because OpenSSL thinks you're overwriting an identity. SSL_CTX_check_private_key is really just detecting this case. Add tests for all this behavior, document that certificates should be configured first, and then deprecate SSL_CTX_check_private_key because, in the correct order, this function is superfluous. This will get shuffled around with SSL_CREDENTIAL, so add some tests first. Bug: 249 Change-Id: I3fcc0f51add1826d581583b43ff003c0dea979dd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66447 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
Update-Note: <openssl/kyber.h> has moved to <openssl/experimental/kyber.h> Change-Id: I51d37aeb2b6cfbbaae494cc38f1b0a82669d2692 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66467 Reviewed-by: David Benjamin <[email protected]> Auto-Submit: Bob Beck <[email protected]> Commit-Queue: Bob Beck <[email protected]>
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]>
We have ssl_has_certificate and ssl_has_private_key calls scattered throughout libssl, but they're never actually tested. The checks are also a little subtle because of cert->chain's weird representation of the leaf being missing but a chain configured. In hindsight, possibly we should have made them separate fields, but it's too late now. We'd have to get rid of SSL_CTX_get0_chain and SSL_get0_chain. Normally we don't bother with these functions, under the "you should know what you configured" theory, but one caller needed it recently in https://boringssl-review.googlesource.com/c/boringssl/+/66087 The tests also confirm that most of the ssl_has_private_key calls, other than the one in ssl_has_certificate, are redundant. The ssl_has_certificate calls are also in an odd place. This will all get shuffled around with SSL_CREDENTIAL, so set up tests first. Bug: 249 Change-Id: If1bb7097a15649e593886c3c22e2cc829a853830 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66508 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
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]>
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]>
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]>
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]>
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]>
In that case, we rely on AKID/SKID matches to disambiguate. However, OpenSSL's internal interfaces are not very good at handling this case and often work around their own bugs. As a precursor to, hopefully, cleaning that up someday, test this, with both direct adding and hash_dir. I've just tested the basic case here. Looking at the code, I think there are bugs where, e.g., if CA1 was added directly and CA2 is only accessible via hash_dir, X509_STORE_CTX_get1_issuer does not know to check hash_dir for CA2, because internal interfaces get in the way. Bug: 685 Change-Id: I32737661c84d6a006cf9d5ae1ec42b3f27437bf0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66010 Reviewed-by: Bob Beck <[email protected]> Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
As with all FFI libraries, the Rust conventions around safety don't really work well. There's a ton of noise from bindgen needing to conservatively mark everything unsafe, obscuring true safety sharp edges like Rust's FFI-incompatible empty slice representation. Change-Id: I2199e61b4900a01e3610772063765c5bb0cb493c Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66287 Commit-Queue: David Benjamin <[email protected]> Reviewed-by: Bob Beck <[email protected]>
It took 3.5 years, but this header is now DONE! I opted to add a section for each extension just because there were so many functions. It's a little weird because, for example, we don't have a section for key usage because it's just BIT STRING. But I think this is better than having a great big "types for built-in extensions" section. Fixed: 426 Change-Id: Ifc7684cc6ff6a211ea1f5065eff67663adf004b3 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66392 Reviewed-by: Bob Beck <[email protected]> Commit-Queue: David Benjamin <[email protected]>
This allows building libpki in aosp with some changes to the downstream bp file Bug: b/322154153 Change-Id: I68773079ec44929b71c1990d13bd3198a4c57ea7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66587 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]>
Rather than using the pre-generated certificates, generate them on the
fly. This allows TLS stacks for which certificate validation and
verification are coupled to work as expected. Certificates and keys are
written to temporary files which are then passed to the shim, and
cleaned up on exit. This requires reworking how testCase passes
certs/keys by adding a new field, sendCertificate, rather than manually
setting the -cert-file and -key-file flags. Incidentally the
rsaChainCertificate is removed, since it was essentially unused, and all
tests that used it also work with rsaCertificate. Finally, include a
single SAN ("test") in all certificates, which fixes some TLS stacks
which require this to operate (such as rustls, which currently
regenerates all the certificates currently in the tree to add a SAN).
Additionally, add a new flag, -trust-cert, which tells the the shim
which certificates it should trust. Shims for TLS stacks which
can completely decouple validation and verification of X509 certificates
(like BoringSSL) can ignore this flag, but for stacks where this
functionality is somewhat more intertwined (like Go), this allows the
shim to properly process the sent certificates.
Change-Id: Ic5c63e18fb2b852cc693aacb3b06cfe7993bc90c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62565
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: David Benjamin <[email protected]>
baentsch
reviewed
Mar 2, 2024
No other changes. I'll update README after I build Chromium next week |
baentsch
approved these changes
Mar 2, 2024
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.
This PR doesn‘t update the algorithm.
Please refer to the next PR for the algorithm update.