Bump minimum swift-crypto v2 version to 2.0.1 #3859
Conversation
Motivation: Test and upgrade to the latest swift-crypto [2.0.1](https://github.com/apple/swift-crypto/releases/tag/2.0.1) release, which includes a BoringSSL update: apple/swift-crypto#95 Modifications: Tests that use real certs, which execute the OCSP code path, crash on Linux (`swift:5.5` and `swift:5.5-focal` docker images). (rdar://85280061) The crash happens on L351 of `CertificatePolicy.swift` (`BoringSSLOCSPClient.checkStatus`) because `response`, an `OCSP_RESPONSE` pointer, is `NULL`. It is `NULL` because the logic in BoringSSL's `crypto/asn1/asn1_lib.c` has changed (google/boringssl@ee510f5), so the return value needs to be handled differently in `PackageCollectionsSigningLibc/asn1/a_d2i_fp.c`. I also don't think this change is correct: google/boringssl@efab69b#diff-dc711de78524fcd1edcc461a96ca36b80cac1bf03a67287a9b434f772a6f4e3dR170. - Guard against `NULL` OCSP response in `BoringSSLOCSPClient`. - Update logic in `PackageCollectionsSigningLibc/asn1/a_d2i_fp.c`: - Add `ASN1_get_object_without_inf` and `asn1_get_length_without_inf`, copied from [BoringSSL](https://github.com/google/boringssl/blob/ee510f58895c6a88b59f1b66a94939d08ebdfe5b/crypto/asn1/asn1_lib.c) but reverting the ["0x80 fix"](google/boringssl@efab69b#diff-dc711de78524fcd1edcc461a96ca36b80cac1bf03a67287a9b434f772a6f4e3dR170), such that we use the same logic regardless of swift-crypto version. We can probably remove them later when we switch over to swift-crypto 2.x completely (unless I'm correct the 0x80 fix). - Remove `inf` in `asn1_d2i_read_bio`. - Note that there are `ASN1_get_object_with_inf` and `asn1_get_length_with_inf` too. We can use these instead to keep the same behavior as swift-crypt 2.0.0 and less. I will delete these if reviewers are ok with the current approach.
yim-lee
requested review from
abertelrud,
elsh,
neonichu and
tomerd
as code owners
|
@swift-ci please smoke test |
|
@Lukasa I would appreciate if you can take a look at this as well. Thanks! |
|
linux CI failure on asyncTest expected until CI gets newer toolchain cc @shahmishal |
|
We have a new toolchain, please re-request testing. |
|
@swift-ci please smoke test linux |
| @@ -54,7 +54,7 @@ automatic linking type with `-auto` suffix appended to product's name. | |||
| let autoProducts = [swiftPMProduct, swiftPMDataModelProduct] | |||
|
|
|||
| let useSwiftCryptoV2 = ProcessInfo.processInfo.environment["SWIFTPM_USE_SWIFT_CRYPTO_V2"] != nil | |||
There was a problem hiding this comment.
Are there any plans to update build-script and the CI to swift-crypto v2? Since this variable is not set, I think it's still testing against 1.1.5.
There was a problem hiding this comment.
Since this variable is not set, I think it's still testing against 1.1.5
Right, SwiftPM is still using swift-crypto v1.
Are there any plans to update build-script and the CI to swift-crypto v2?
No specific timeline yet. So far I've been testing locally (to do additional test cases that CI can't cover) whenever there's a new swift-crypto release. When we are ready to upgrade to v2 we'll probably remove this env var and stick with build-script.
Sources/PackageCollectionsSigning/Certificate/CertificatePolicy.swift
Outdated
Show resolved
Hide resolved
|
@swift-ci please smoke test |
Motivation:
Test and upgrade to the latest swift-crypto 2.0.1 release, which
includes a BoringSSL update: apple/swift-crypto#95
Modifications:
Tests that use real certs, which execute the OCSP code path, crash on Linux (
swift:5.5andswift:5.5-focaldocker images). (rdar://85280061)The crash happens on L351 of
CertificatePolicy.swift(BoringSSLOCSPClient.checkStatus) becauseresponse, anOCSP_RESPONSEpointer, isNULL. It isNULLbecause the logic in BoringSSL'scrypto/asn1/asn1_lib.chas changed (google/boringssl@ee510f5), so the return value needs to be handled differently inPackageCollectionsSigningLibc/asn1/a_d2i_fp.c.I also don't think this change is correct: google/boringssl@efab69b#diff-dc711de78524fcd1edcc461a96ca36b80cac1bf03a67287a9b434f772a6f4e3dR170.
NULLOCSP response inBoringSSLOCSPClient.PackageCollectionsSigningLibc/asn1/a_d2i_fp.c:ASN1_get_object_without_infandasn1_get_length_without_inf, copied from BoringSSL but reverting the "0x80 fix", such that we use the same logic regardless of swift-crypto version. We can probably remove them later when we switch over to swift-crypto 2.x completely (unless I'm correct the 0x80 fix).infinasn1_d2i_read_bio.ASN1_get_object_with_infandasn1_get_length_with_inftoo. We can use these instead to keep the same behavior as swift-crypt 2.0.0 and less. I will delete these if reviewers are ok with the current approach.