Add misc. x509 un/lock and set1 functions (aws#1449)

Author: WillChilds-Klein

.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:

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;

crypto/x509/x509_test.cc

@@ -30,6 +30,7 @@
 #include <openssl/nid.h>
 #include <openssl/pem.h>
 #include <openssl/pool.h>
+#include <openssl/rand.h>
 #include <openssl/x509.h>
 #include <openssl/x509v3.h>
 
@@ -1945,6 +1946,26 @@ TEST(X509Test, TestCRL) {
   ASSERT_EQ(nullptr, X509_OBJECT_get0_X509_CRL(&invalidCRL));
 }
 
+TEST(X509Test, TestX509GettersSetters) {
+  bssl::UniquePtr<X509_OBJECT> obj(X509_OBJECT_new());
+  bssl::UniquePtr<X509> x509(CertFromPEM(kCRLTestRoot));
+  bssl::UniquePtr<X509_CRL> 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<X509> 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<std::thread> 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);
 }

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;
 }

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;
 }

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)

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}