diff --git a/.github/workflows/integrations.yml b/.github/workflows/integrations.yml index 87bd013f99..b1465fa801 100644 --- a/.github/workflows/integrations.yml +++ b/.github/workflows/integrations.yml @@ -91,7 +91,7 @@ jobs: - name: Run integration build run: | ./tests/ci/integration/run_socat_integration.sh - python: + python-main: runs-on: ubuntu-latest steps: - name: Install OS Dependencies @@ -101,7 +101,18 @@ jobs: - uses: actions/checkout@v3 - name: Build AWS-LC, build python, run tests run: | - ./tests/ci/integration/run_python_integration.sh + ./tests/ci/integration/run_python_integration.sh main + python-releases: + runs-on: ubuntu-latest + steps: + - name: Install OS Dependencies + run: | + sudo apt-get update + sudo apt-get -y --no-install-recommends install cmake gcc ninja-build golang make + - uses: actions/checkout@v3 + - name: Build AWS-LC, build python, run tests + run: | + ./tests/ci/integration/run_python_integration.sh 3.10 3.11 3.12 bind9: runs-on: ubuntu-latest steps: diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c index 364aa8a515..bcad1043d8 100644 --- a/crypto/x509/x509_lu.c +++ b/crypto/x509/x509_lu.c @@ -196,6 +196,22 @@ X509_STORE *X509_STORE_new(void) { return NULL; } +int X509_STORE_lock(X509_STORE *v) { + if (v == NULL) { + return 0; + } + CRYPTO_MUTEX_lock_write(&v->objs_lock); + return 1; +} + +int X509_STORE_unlock(X509_STORE *v) { + if (v == NULL) { + return 0; + } + CRYPTO_MUTEX_unlock_write(&v->objs_lock); + return 1; +} + int X509_STORE_up_ref(X509_STORE *store) { CRYPTO_refcount_inc(&store->references); return 1; @@ -405,6 +421,28 @@ X509_CRL *X509_OBJECT_get0_X509_CRL(const X509_OBJECT *a) { return a->data.crl; } +int X509_OBJECT_set1_X509(X509_OBJECT *a, X509 *obj) { + if (a == NULL || !X509_up_ref(obj)) { + return 0; + } + + X509_OBJECT_free_contents(a); + a->type = X509_LU_X509; + a->data.x509 = obj; + return 1; +} + +int X509_OBJECT_set1_X509_CRL(X509_OBJECT *a, X509_CRL *obj) { + if (a == NULL || !X509_CRL_up_ref(obj)) { + return 0; + } + + X509_OBJECT_free_contents(a); + a->type = X509_LU_CRL; + a->data.crl = obj; + return 1; +} + static int x509_object_idx_cnt(STACK_OF(X509_OBJECT) *h, int type, X509_NAME *name, int *pnmatch) { X509_OBJECT stmp; diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index ae2385c39e..e0283dc70f 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -30,6 +30,7 @@ #include #include #include +#include #include #include @@ -1945,6 +1946,26 @@ TEST(X509Test, TestCRL) { ASSERT_EQ(nullptr, X509_OBJECT_get0_X509_CRL(&invalidCRL)); } +TEST(X509Test, TestX509GettersSetters) { + bssl::UniquePtr obj(X509_OBJECT_new()); + bssl::UniquePtr x509(CertFromPEM(kCRLTestRoot)); + bssl::UniquePtr crl(CRLFromPEM(kBasicCRL)); + + ASSERT_TRUE(obj); + ASSERT_TRUE(x509); + ASSERT_TRUE(crl); + + EXPECT_EQ(0, X509_OBJECT_get0_X509(obj.get())); + EXPECT_EQ(0, X509_OBJECT_get0_X509_CRL(obj.get())); + EXPECT_EQ(0, X509_OBJECT_set1_X509(nullptr, x509.get())); + EXPECT_EQ(0, X509_OBJECT_set1_X509_CRL(nullptr, crl.get())); + + EXPECT_EQ(1, X509_OBJECT_set1_X509(obj.get(), x509.get())); + EXPECT_EQ(x509.get(), X509_OBJECT_get0_X509(obj.get())); + EXPECT_EQ(1, X509_OBJECT_set1_X509_CRL(obj.get(), crl.get())); + EXPECT_EQ(crl.get(), X509_OBJECT_get0_X509_CRL(obj.get())); +} + TEST(X509Test, ManyNamesAndConstraints) { bssl::UniquePtr many_constraints(CertFromPEM( GetTestData("crypto/x509/test/many_constraints.pem").c_str())); @@ -5022,12 +5043,44 @@ TEST(X509Test, AddDuplicates) { ASSERT_TRUE(a); ASSERT_TRUE(b); + // To begin, add the certs to the store. Subsequent adds will be duplicative. EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); - EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); - EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); - EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); - EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); + + // Half the threads add duplicate certs, the other half take a lock and + // look them up to exercise un/locking functions. + const size_t kNumThreads = 10; + std::vector threads; + for (size_t i = 0; i < kNumThreads/2; i++) { + threads.emplace_back([&] { + // Sleep with some jitter to offset thread execution + uint8_t sleep_buf[1]; + ASSERT_TRUE(RAND_bytes(sleep_buf, sizeof(sleep_buf))); + std::this_thread::sleep_for(std::chrono::microseconds(1 + (sleep_buf[0] % 5))); + EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); + EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); + }); + threads.emplace_back([&] { + uint8_t sleep_buf[1]; + ASSERT_TRUE(RAND_bytes(sleep_buf, sizeof(sleep_buf))); + ASSERT_TRUE(X509_STORE_lock(store.get())); + // Sleep after taking the lock to cause contention. Sleep longer than the + // adder half of threads to ensure we hold the lock while they contend + // for it. |X509_OBJECT_retrieve_by_subject| is called because it doesn't + // take a lock on the store, thus avoiding deadlock. + std::this_thread::sleep_for(std::chrono::microseconds(11 + (sleep_buf[0] % 5))); + EXPECT_TRUE(X509_OBJECT_retrieve_by_subject( + store->objs, X509_LU_X509, X509_get_subject_name(a.get()) + )); + EXPECT_TRUE(X509_OBJECT_retrieve_by_subject( + store->objs, X509_LU_X509, X509_get_subject_name(b.get()) + )); + ASSERT_TRUE(X509_STORE_unlock(store.get())); + }); + } + for (auto &thread : threads) { + thread.join(); + } EXPECT_EQ(sk_X509_OBJECT_num(X509_STORE_get0_objects(store.get())), 2u); } diff --git a/crypto/x509/x509cset.c b/crypto/x509/x509cset.c index 6dd1fc0826..a35a086a19 100644 --- a/crypto/x509/x509cset.c +++ b/crypto/x509/x509cset.c @@ -138,6 +138,9 @@ int X509_CRL_sort(X509_CRL *c) { } int X509_CRL_up_ref(X509_CRL *crl) { + if (crl == NULL) { + return 0; + } CRYPTO_refcount_inc(&crl->references); return 1; } diff --git a/crypto/x509/x_x509.c b/crypto/x509/x_x509.c index 4f23fd68bb..963d9afd6f 100644 --- a/crypto/x509/x_x509.c +++ b/crypto/x509/x_x509.c @@ -198,6 +198,9 @@ X509 *X509_parse_from_buffer(CRYPTO_BUFFER *buf) { } int X509_up_ref(X509 *x) { + if (x == NULL) { + return 0; + } CRYPTO_refcount_inc(&x->references); return 1; } diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 6e09f02a1b..efa160214b 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2802,7 +2802,26 @@ OPENSSL_EXPORT int X509_OBJECT_get_type(const X509_OBJECT *a); OPENSSL_EXPORT X509 *X509_OBJECT_get0_X509(const X509_OBJECT *a); // X509_OBJECT_get0_X509_CRL returns the |X509_CRL| associated with |a| OPENSSL_EXPORT X509_CRL *X509_OBJECT_get0_X509_CRL(const X509_OBJECT *a); + +// X509_OBJECT_set1_X509 sets |obj| on |a| and uprefs |obj|. As with other set1 +// methods, |a| does not take ownership of |obj|; the caller is responsible for +// managing freeing |obj| when appropriate. +OPENSSL_EXPORT int X509_OBJECT_set1_X509(X509_OBJECT *a, X509 *obj); + +// X509_OBJECT_set1_X509_CRL sets CRL |obj| on |a| and uprefs |obj|. As with +// other set1 methods, |a| does not take ownership of |obj|; the caller is +// responsible for managing freeing |obj| when appropriate. +OPENSSL_EXPORT int X509_OBJECT_set1_X509_CRL(X509_OBJECT *a, X509_CRL *obj); + OPENSSL_EXPORT X509_STORE *X509_STORE_new(void); +// X509_STORE_lock takes a write lock on |v|. return 1 on success, 0 on failure. +// +// Avoid operations on the X509_STORE or a X509_STORE_CTX containing it while +// it is locked; many |X509_STORE_*| functions take this lock internally which +// will cause a deadlock when called on a locked store. +OPENSSL_EXPORT int X509_STORE_lock(X509_STORE *v); +// X509_STORE_unlock releases a lock on |v|. return 1 on success, 0 on failure +OPENSSL_EXPORT int X509_STORE_unlock(X509_STORE *v); OPENSSL_EXPORT int X509_STORE_up_ref(X509_STORE *store); OPENSSL_EXPORT void X509_STORE_free(X509_STORE *v); @@ -3124,6 +3143,7 @@ BORINGSSL_MAKE_UP_REF(X509_CRL, X509_CRL_up_ref) BORINGSSL_MAKE_DELETER(X509_EXTENSION, X509_EXTENSION_free) BORINGSSL_MAKE_DELETER(X509_INFO, X509_INFO_free) BORINGSSL_MAKE_DELETER(X509_LOOKUP, X509_LOOKUP_free) +BORINGSSL_MAKE_DELETER(X509_OBJECT, X509_OBJECT_free) BORINGSSL_MAKE_DELETER(X509_NAME, X509_NAME_free) BORINGSSL_MAKE_DELETER(X509_NAME_ENTRY, X509_NAME_ENTRY_free) BORINGSSL_MAKE_DELETER(X509_PKEY, X509_PKEY_free) diff --git a/tests/ci/integration/run_python_integration.sh b/tests/ci/integration/run_python_integration.sh index 7bdc3245df..3abd849b8f 100755 --- a/tests/ci/integration/run_python_integration.sh +++ b/tests/ci/integration/run_python_integration.sh @@ -95,6 +95,10 @@ function python_patch() { local branch=${1} local src_dir="${PYTHON_SRC_FOLDER}/${branch}" local patch_dir="${PYTHON_PATCH_FOLDER}/${branch}" + if [[ ! $(find -L ${patch_dir} -type f -name '*.patch') ]]; then + echo "No patch for ${branch}!" + exit 1 + fi git clone https://github.com/python/cpython.git ${src_dir} \ --depth 1 \ --branch ${branch} @@ -106,6 +110,11 @@ function python_patch() { done } +if [[ "$#" -eq "0" ]]; then + echo "No python branches provided for testing" + exit 1 +fi + mkdir -p ${SCRATCH_FOLDER} rm -rf ${SCRATCH_FOLDER}/* cd ${SCRATCH_FOLDER} @@ -126,9 +135,8 @@ pushd ${PYTHON_SRC_FOLDER} which sysctl && ( sysctl -w net.ipv6.conf.all.disable_ipv6=0 || /bin/true ) echo 0 >/proc/sys/net/ipv6/conf/all/disable_ipv6 || /bin/true -# NOTE: cpython keeps a unique branch per version, add version branches here # NOTE: As we add more versions to support, we may want to parallelize here -for branch in 3.10 3.11 3.12 main; do +for branch in "$@"; do python_patch ${branch} python_build ${branch} python_run_tests ${branch}