From 953f643a250ca5568f75b091b58a027f61e3c1d4 Mon Sep 17 00:00:00 2001 From: Samuel Chiang Date: Mon, 5 Aug 2024 10:11:47 -0700 Subject: [PATCH 1/2] add support for OCSP_copy_nonce (#1711) Ruby consumes `OCSP_copy_nonce`, which copies the nonce from the request to the OCSP response. OpenSSL's `OCSP_copy_nonce` directly appends the new OCSP nonce to the list of extensions in the response. This allows multiple OCSP nonces to exist the same response since the existing nonces are not deleted. It's a bit strange to allow multiple nonces to exist in a single response. To make things stranger, `OCSP_check_nonce` directly takes the first OCSP nonce in the list to compare, so any newer nonces are ignored. We ultimately decided to follow OpenSSL's faulty implementation and document/test the behavior, there doesn't seem to be much value in diverging from the original and will only introduce more unanticipated behavioral changes. 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. --- crypto/ocsp/internal.h | 6 ++++++ crypto/ocsp/ocsp_extension.c | 28 ++++++++++++++++++++++++++++ crypto/ocsp/ocsp_test.cc | 21 +++++++++++++++++++++ include/openssl/ocsp.h | 20 +++++++++++++++++++- 4 files changed, 74 insertions(+), 1 deletion(-) diff --git a/crypto/ocsp/internal.h b/crypto/ocsp/internal.h index cbc44bc164..a5c2095930 100644 --- a/crypto/ocsp/internal.h +++ b/crypto/ocsp/internal.h @@ -281,6 +281,12 @@ OPENSSL_EXPORT int OCSP_REQUEST_get_ext_by_NID(OCSP_REQUEST *req, int nid, // by its position in the extension list. OPENSSL_EXPORT X509_EXTENSION *OCSP_REQUEST_get_ext(OCSP_REQUEST *req, int loc); +// OCSP_BASICRESP_add_ext adds a copy of |ex| to the extension list in +// |*bs|. It returns 1 on success and 0 on error. The new extension is +// inserted at index |loc|, shifting extensions to the right. If |loc| is -1 or +// out of bounds, the new extension is appended to the list. +int OCSP_BASICRESP_add_ext(OCSP_BASICRESP *bs, X509_EXTENSION *ex, int loc); + #define IS_OCSP_FLAG_SET(flags, query) (flags & query) #define OCSP_MAX_RESP_LENGTH (100 * 1024) diff --git a/crypto/ocsp/ocsp_extension.c b/crypto/ocsp/ocsp_extension.c index 721333bbf2..2deae46d5c 100644 --- a/crypto/ocsp/ocsp_extension.c +++ b/crypto/ocsp/ocsp_extension.c @@ -28,6 +28,11 @@ X509_EXTENSION *OCSP_REQUEST_get_ext(OCSP_REQUEST *req, int loc) { return X509v3_get_ext(req->tbsRequest->requestExtensions, loc); } +int OCSP_BASICRESP_add_ext(OCSP_BASICRESP *bs, X509_EXTENSION *ex, int loc) { + return (X509v3_add_ext(&bs->tbsResponseData->responseExtensions, ex, loc) != + NULL); +} + int OCSP_BASICRESP_get_ext_by_NID(OCSP_BASICRESP *bs, int nid, int lastpos) { return X509v3_get_ext_by_NID(bs->tbsResponseData->responseExtensions, nid, lastpos); @@ -37,6 +42,10 @@ X509_EXTENSION *OCSP_BASICRESP_get_ext(OCSP_BASICRESP *bs, int loc) { return X509v3_get_ext(bs->tbsResponseData->responseExtensions, loc); } +X509_EXTENSION *OCSP_BASICRESP_delete_ext(OCSP_BASICRESP *x, int loc) { + return X509v3_delete_ext(x->tbsResponseData->responseExtensions, loc); +} + int OCSP_SINGLERESP_add_ext(OCSP_SINGLERESP *sresp, X509_EXTENSION *ex, int loc) { GUARD_PTR(sresp); @@ -140,3 +149,22 @@ int OCSP_check_nonce(OCSP_REQUEST *req, OCSP_BASICRESP *bs) { } return OCSP_NONCE_EQUAL; } + +int OCSP_copy_nonce(OCSP_BASICRESP *resp, OCSP_REQUEST *req) { + GUARD_PTR(resp); + GUARD_PTR(req); + + // Check for nonce in request. + int req_idx = OCSP_REQUEST_get_ext_by_NID(req, NID_id_pkix_OCSP_Nonce, -1); + // If no nonce, that's OK. We return 2 in this case. + if (req_idx < 0) { + return 2; + } + X509_EXTENSION *req_ext = OCSP_REQUEST_get_ext(req, req_idx); + // Nonce found, but no entry at the index. + // This shouldn't happen under normal circumstances. + GUARD_PTR(req_ext); + + // Append the nonce. + return OCSP_BASICRESP_add_ext(resp, req_ext, -1); +} diff --git a/crypto/ocsp/ocsp_test.cc b/crypto/ocsp/ocsp_test.cc index 0da4669acd..7459054254 100644 --- a/crypto/ocsp/ocsp_test.cc +++ b/crypto/ocsp/ocsp_test.cc @@ -1387,6 +1387,27 @@ TEST_P(OCSPNonceTest, OCSPNonce) { } EXPECT_EQ(OCSP_check_nonce(ocspRequest.get(), basicResponse.get()), t.nonce_check_status); + + // Check that nonce copying from |req| to |bs| also works as expected. + if (t.nonce_check_status == OCSP_NONCE_RESPONSE_ONLY || + t.nonce_check_status == OCSP_NONCE_BOTH_ABSENT) { + EXPECT_EQ(OCSP_copy_nonce(basicResponse.get(), ocspRequest.get()), 2); + EXPECT_EQ(OCSP_check_nonce(ocspRequest.get(), basicResponse.get()), + t.nonce_check_status); + } else { + // OpenSSL's implementation of |OCSP_copy_nonce| keeps the original nonce in + // |resp| at the start of the list. We have to remove the old nonce prior to + // copying the new one over. + int resp_idx = OCSP_BASICRESP_get_ext_by_NID(basicResponse.get(), + NID_id_pkix_OCSP_Nonce, -1); + if (resp_idx >= 0) { + bssl::UniquePtr old_resp_ext( + OCSP_BASICRESP_delete_ext(basicResponse.get(), resp_idx)); + } + EXPECT_EQ(OCSP_copy_nonce(basicResponse.get(), ocspRequest.get()), 1); + EXPECT_EQ(OCSP_check_nonce(ocspRequest.get(), basicResponse.get()), + OCSP_NONCE_EQUAL); + } } TEST(OCSPTest, OCSPNonce) { diff --git a/include/openssl/ocsp.h b/include/openssl/ocsp.h index af29b1d566..b5caba4c33 100644 --- a/include/openssl/ocsp.h +++ b/include/openssl/ocsp.h @@ -195,6 +195,16 @@ OPENSSL_EXPORT int OCSP_request_add1_nonce(OCSP_REQUEST *req, // but aren't equal. OPENSSL_EXPORT int OCSP_check_nonce(OCSP_REQUEST *req, OCSP_BASICRESP *bs); +// OCSP_copy_nonce copies the nonce value (if any) from |req| to |resp|. Returns +// 1 on success and 0 on failure. If the optional nonce value does not exist in +// |req|, we return 2 instead. +// +// Note: |OCSP_copy_nonce| allows for multiple OCSP nonces to exist and appends +// the new nonce to the end of the extension list. This causes issues with +// |OCSP_check_nonce|, since it looks for the first one in the list. The old +// nonce extension should be deleted prior to calling |OCSP_copy_nonce|. +OPENSSL_EXPORT int OCSP_copy_nonce(OCSP_BASICRESP *resp, OCSP_REQUEST *req); + // OCSP_request_set1_name sets |requestorName| from an |X509_NAME| structure. OPENSSL_EXPORT int OCSP_request_set1_name(OCSP_REQUEST *req, X509_NAME *nm); @@ -352,7 +362,8 @@ OPENSSL_EXPORT int OCSP_parse_url(const char *url, char **phost, char **pport, // OCSP_id_issuer_cmp compares the issuers' name and key hash of |a| and |b|. It // returns 0 on equal. -OPENSSL_EXPORT int OCSP_id_issuer_cmp(const OCSP_CERTID *a, const OCSP_CERTID *b); +OPENSSL_EXPORT int OCSP_id_issuer_cmp(const OCSP_CERTID *a, + const OCSP_CERTID *b); // OCSP_id_cmp calls |OCSP_id_issuer_cmp| and additionally compares the // |serialNumber| of |a| and |b|. It returns 0 on equal. @@ -413,6 +424,13 @@ OPENSSL_EXPORT X509_EXTENSION *OCSP_BASICRESP_get_ext(OCSP_BASICRESP *bs, // OCSP |X509_EXTENSION| Functions +// OCSP_BASICRESP_delete_ext removes the extension in |x| at index |loc| and +// returns the removed extension, or NULL if |loc| was out of bounds. If an +// extension was returned, the caller must release it with +// |X509_EXTENSION_free|. +OPENSSL_EXPORT X509_EXTENSION *OCSP_BASICRESP_delete_ext(OCSP_BASICRESP *x, + int loc); + // OCSP_SINGLERESP_add_ext adds a copy of |ex| to the extension list in // |*sresp|. It returns 1 on success and 0 on error. The new extension is // inserted at index |loc|, shifting extensions to the right. If |loc| is -1 or From 98eeccfd3ff38b724837858d28dc962884e34f49 Mon Sep 17 00:00:00 2001 From: Shubham Mittal <107728331+smittals2@users.noreply.github.com> Date: Mon, 5 Aug 2024 10:29:23 -0700 Subject: [PATCH 2/2] Specifying CPU threads in cmake_build.sh to fix CI failures (#1740) ### Description of changes: A previous PR (https://github.com/aws/aws-lc/pull/1735/files) broke the CI but was merged in since the failed checks were not required. This PR fixes these failures by explicitly setting the number of threads to parallelize the make command (as opposed to using -j to spin up as many processes as possible). 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. --- .github/docker_images/cmake_build_versions/cmake_build.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/docker_images/cmake_build_versions/cmake_build.sh b/.github/docker_images/cmake_build_versions/cmake_build.sh index 46603a01e3..cc737dcbe3 100755 --- a/.github/docker_images/cmake_build_versions/cmake_build.sh +++ b/.github/docker_images/cmake_build_versions/cmake_build.sh @@ -7,8 +7,10 @@ set -ex -o pipefail echo "Building CMake Version: ${CMAKE_VERSION:-unknown}" +NUM_CPU_THREADS=$(grep -c ^processor /proc/cpuinfo) + # At the moment this works fine for all versions, in the future build logic can be modified to # look at it ${CMAKE_VERSION}. ./configure --prefix=/opt/cmake --system-curl --system-libarchive -make -j +make -j"${NUM_CPU_THREADS}" make install