Merge branch 'main' into feature-add-openSSL3

.github/workflows/ci_linting.yml

@@ -74,20 +74,25 @@ jobs:
         run: |
           ./codebuild/bin/run_kwstyle.sh
           ./codebuild/bin/cpp_style_comment_linter.sh
-  pepeight:
+  ruff:
     runs-on: ubuntu-latest
     steps:
       - name: checkout
         uses: actions/checkout@v4
-      - name: Run autopep8
-        id: autopep8
-        uses: peter-evans/autopep8@v2
-        with:
-          args: --diff --exit-code .
+
+      - name: Set up uv
+        uses: astral-sh/setup-uv@v5
+
+      - name: Run Ruff formatting check
+        working-directory: tests/integrationv2
+        id: ruff_format
+        run: uv run ruff format --check .
+        continue-on-error: true
+
       - name: Check exit code
-        if: steps.autopep8.outputs.exit-code != 0
+        if: steps.ruff_format.outcome == 'failure'
         run: |
-            echo "Run 'autopep8 --in-place .' to fix"
+            echo "Run 'ruff format .' to fix formatting issues"
             exit 1
   clang-format:
     runs-on: ubuntu-latest

CMakeLists.txt

@@ -165,7 +165,7 @@ endif ()
 if(BUILD_TESTING AND BUILD_SHARED_LIBS OR S2N_FUZZ_TEST)
     target_compile_options(${PROJECT_NAME} PRIVATE -fvisibility=default)
 else()
-    target_compile_options(${PROJECT_NAME} PRIVATE -fvisibility=hidden -DS2N_EXPORTS)
+    target_compile_options(${PROJECT_NAME} PRIVATE -fvisibility=hidden -DS2N_EXPORTS=1)
 endif()
 
 if(S2N_LTO)
@@ -197,7 +197,7 @@ target_compile_options(${PROJECT_NAME} PRIVATE -include "${S2N_PRELUDE}")
 # Match on Release, RelWithDebInfo and MinSizeRel
 # See: https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html#variable:CMAKE_BUILD_TYPE
 if(CMAKE_BUILD_TYPE MATCHES Rel)
-    add_definitions(-DS2N_BUILD_RELEASE)
+    add_definitions(-DS2N_BUILD_RELEASE=1)
 endif()
 
 if(NO_STACK_PROTECTOR)
@@ -251,7 +251,7 @@ endif()
 
 if (NOT S2N_OVERRIDE_LIBCRYPTO_RAND_ENGINE)
     message(STATUS "Disabling libcrypto RAND engine override")
-    add_definitions(-DS2N_DISABLE_RAND_ENGINE_OVERRIDE)
+    add_definitions(-DS2N_DISABLE_RAND_ENGINE_OVERRIDE=1)
 endif()
 
 # For interning, we need to find the static libcrypto library. Cmake configs
@@ -316,7 +316,7 @@ function(feature_probe_result PROBE_NAME IS_AVAILABLE)
 
     # define the probe if available
     if(NORMALIZED)
-        add_definitions(-D${PROBE_NAME})
+        add_definitions(-D${PROBE_NAME}=1)
     endif()
 endfunction()
 
@@ -426,7 +426,7 @@ if (S2N_INTERN_LIBCRYPTO)
       DEPENDS libcrypto.symbols
     )
     add_dependencies(${PROJECT_NAME} s2n_libcrypto)
-    add_definitions(-DS2N_INTERN_LIBCRYPTO)
+    add_definitions(-DS2N_INTERN_LIBCRYPTO=1)
 
     if ((BUILD_SHARED_LIBS AND BUILD_TESTING) OR NOT BUILD_SHARED_LIBS)
         # if libcrypto needs to be interned, rewrite libcrypto references so use of internal functions will link correctly

api/unstable/crl.h

@@ -187,12 +187,16 @@ struct s2n_cert_validation_info;
  *
  * If the validation performed in the callback is successful, `s2n_cert_validation_accept()` MUST be called to allow
  * `s2n_negotiate()` to continue the handshake. If the validation is unsuccessful, `s2n_cert_validation_reject()`
- * MUST be called, which will cause `s2n_negotiate()` to error. The behavior of `s2n_negotiate()` is undefined if
- * neither `s2n_cert_validation_accept()` or `s2n_cert_validation_reject()` are called.
+ * MUST be called, which will cause `s2n_negotiate()` to error.
+ * 
+ * To use the validation callback asynchronously, return `S2N_SUCCESS` without calling `s2n_cert_validation_accept()`
+ * or `s2n_cert_validation_reject()`. This will pause the handshake, and `s2n_negotiate()` will throw an `S2N_ERR_T_BLOCKED`
+ * error and `s2n_blocked_status` will be set to `S2N_BLOCKED_ON_APPLICATION_INPUT`. Applications should call
+ * `s2n_cert_validation_accept()` or `s2n_cert_validation_reject()` to unpause the handshake before retrying `s2n_negotiate()`.
  *
  * The `info` parameter is passed to the callback in order to call APIs specific to the cert validation callback, like
- * `s2n_cert_validation_accept()` and `s2n_cert_validation_reject()`. The `info` argument is only valid for the
- * lifetime of the callback, and must not be used after the callback has finished.
+ * `s2n_cert_validation_accept()` and `s2n_cert_validation_reject()`. The `info` argument shares the same lifetime as
+ * `s2n_connection`.
  *
  * After calling `s2n_cert_validation_reject()`, `s2n_negotiate()` will fail with a protocol error indicating that
  * the cert has been rejected from the callback. If more information regarding an application's custom validation

bindings/rust/extended/s2n-tls/src/cert_chain.rs

@@ -1,9 +1,11 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-use crate::error::{Error, Fallible};
+use crate::error::{Error, ErrorType, Fallible};
 use s2n_tls_sys::*;
 use std::{
+    any::Any,
+    ffi::c_void,
     marker::PhantomData,
     ptr::{self, NonNull},
     sync::Arc,
@@ -13,6 +15,7 @@ use std::{
 ///
 /// [CertificateChain] is internally reference counted. The reference counted `T`
 /// must have a drop implementation.
+#[derive(Debug)]
 pub(crate) struct CertificateChainHandle<'a> {
     pub(crate) cert: NonNull<s2n_cert_chain_and_key>,
     is_owned: bool,
@@ -45,20 +48,57 @@ impl CertificateChainHandle<'_> {
             _lifetime: PhantomData,
         }
     }
+
+    /// Corresponds to [s2n_cert_chain_and_key_get_ctx].
+    fn context_mut(&mut self) -> Option<&mut Context> {
+        let context = unsafe { s2n_cert_chain_and_key_get_ctx(self.cert.as_ptr()) };
+        if context.is_null() {
+            None
+        } else {
+            Some(unsafe { &mut *(context as *mut Context) })
+        }
+    }
+
+    /// Corresponds to [s2n_cert_chain_and_key_get_ctx].
+    fn context(&self) -> Option<&Context> {
+        let context = unsafe { s2n_cert_chain_and_key_get_ctx(self.cert.as_ptr()) };
+        if context.is_null() {
+            None
+        } else {
+            Some(unsafe { &*(context as *const Context) })
+        }
+    }
 }
 
 impl Drop for CertificateChainHandle<'_> {
     /// Corresponds to [s2n_cert_chain_and_key_free].
     fn drop(&mut self) {
-        // ignore failures since there's not much we can do about it
         if self.is_owned {
+            if let Some(internal_context) = self.context_mut() {
+                drop(unsafe { Box::from_raw(internal_context) });
+            }
+            // ignore failures since there's not much we can do about it
             unsafe {
+                // null the cert chain context out of an abundance of caution
+                let _ = s2n_cert_chain_and_key_set_ctx(self.cert.as_ptr(), std::ptr::null_mut())
+                    .into_result();
+
                 let _ = s2n_cert_chain_and_key_free(self.cert.as_ptr()).into_result();
             }
         }
     }
 }
 
+/// An internal container to hold the customer supplied application context.
+///
+/// We can't directly store the application context on the `s2n_cert_chain_and_key`,
+/// because `*mut dyn Any` is a fat pointer (16 bytes) and can not be stored as
+/// a c_void (8 bytes).
+struct Context {
+    application_context: Box<dyn Any + Send + Sync>,
+}
+
+#[derive(Debug)]
 pub struct Builder {
     cert_handle: CertificateChainHandle<'static>,
 }
@@ -125,6 +165,39 @@ impl Builder {
         Ok(self)
     }
 
+    /// Associates an arbitrary application context with the CertificateChain to
+    /// be later retrieved via [`CertificateChain::application_context()`].
+    ///
+    /// This API will override an existing application context set on the Builder.
+    ///
+    /// Corresponds to [s2n_cert_chain_and_key_set_ctx].
+    pub fn set_application_context<T: Send + Sync + 'static>(
+        &mut self,
+        app_context: T,
+    ) -> Result<&mut Self, Error> {
+        match self.cert_handle.context_mut() {
+            Some(_) => Err(Error::bindings(
+                ErrorType::UsageError,
+                "cert builder error",
+                "set_application_context can only be called once",
+            )),
+            None => {
+                let app_context = Box::new(app_context);
+                let internal_context = Box::new(Context {
+                    application_context: app_context,
+                });
+                unsafe {
+                    s2n_cert_chain_and_key_set_ctx(
+                        self.cert_handle.cert.as_ptr(),
+                        Box::into_raw(internal_context) as *mut c_void,
+                    )
+                    .into_result()
+                }?;
+                Ok(self)
+            }
+        }
+    }
+
     /// Return an immutable, internally-reference counted CertificateChain.
     pub fn build(self) -> Result<CertificateChain<'static>, Error> {
         // This method is currently infallible, but returning a result allows
@@ -177,6 +250,23 @@ impl CertificateChain<'_> {
         }
     }
 
+    /// Retrieves a reference to the application context associated with the
+    /// CertificateChain.
+    ///
+    /// If an application context hasn't been set on the CertificateChain or if
+    /// the set application context isn't of type `T`, `None` will be returned.
+    ///
+    /// To set a context on the connection, use [`Builder::set_application_context()`].
+    ///
+    /// Corresponds to [s2n_cert_chain_and_key_get_ctx].
+    pub fn application_context<T: Send + Sync + 'static>(&self) -> Option<&T> {
+        if let Some(internal_context) = self.cert_handle.context() {
+            internal_context.application_context.downcast_ref()
+        } else {
+            None
+        }
+    }
+
     /// Return the length of this certificate chain.
     ///
     /// Note that the underlying API currently traverses a linked list, so this is a relatively
@@ -273,9 +363,12 @@ unsafe impl Send for Certificate<'_> {}
 mod tests {
     use crate::{
         config,
-        error::{ErrorSource, ErrorType},
+        error::{Error as S2NError, ErrorSource, ErrorType},
         security::DEFAULT_TLS13,
-        testing::{InsecureAcceptAllCertificatesHandler, SniTestCerts, TestPair},
+        testing::{
+            config_builder, CertKeyPair, InsecureAcceptAllCertificatesHandler, SniTestCerts,
+            TestPair,
+        },
     };
 
     use super::*;
@@ -495,4 +588,67 @@ mod tests {
         fn assert_send_sync<T: 'static + Send + Sync>() {}
         assert_send_sync::<CertificateChain<'static>>();
     }
+
+    /// sanity check for basic cert chain context interactions
+    #[test]
+    fn application_context_workflow() -> Result<(), S2NError> {
+        let context: Arc<u64> = Arc::new(0xC0FFEE);
+        let handle = Arc::clone(&context);
+        assert_eq!(Arc::strong_count(&handle), 2);
+
+        let default = CertKeyPair::default();
+        let mut chain = Builder::new()?;
+        chain.load_pem(default.cert(), default.key())?;
+        chain.set_application_context(context)?;
+        let chain = chain.build()?;
+
+        let invalid_type_get = chain.application_context::<u64>();
+        assert!(invalid_type_get.is_none());
+
+        let retrieved_context = chain.application_context::<Arc<u64>>().unwrap();
+        assert_eq!(*retrieved_context.as_ref(), 0xC0FFEE);
+        assert_eq!(Arc::strong_count(&handle), 2);
+        drop(chain);
+        assert_eq!(Arc::strong_count(&handle), 1);
+        Ok(())
+    }
+
+    /// When an application context is overridden, it should be error.
+    #[test]
+    fn application_context_override() -> Result<(), S2NError> {
+        let initial: Arc<u64> = Arc::new(0xC0FFEE);
+        let overridden: Arc<[u8; 6]> = Arc::new(*b"coffee");
+
+        let mut builder = Builder::new()?;
+        builder.set_application_context(initial)?;
+        let err = builder.set_application_context(overridden).unwrap_err();
+        assert_eq!(err.kind(), ErrorType::UsageError);
+
+        Ok(())
+    }
+
+    /// An application context should be retrievable from a selected cert after
+    /// the handshake.
+    #[test]
+    fn application_context_from_selected_cert() -> Result<(), S2NError> {
+        let default = CertKeyPair::default();
+        let mut chain = Builder::new()?;
+        chain.load_pem(default.cert(), default.key())?;
+        chain.set_application_context(0xC0FFEE_u64)?;
+
+        let mut server_config = config::Builder::new();
+        server_config.load_chain(chain.build()?)?;
+
+        let client_config = config_builder(&crate::security::DEFAULT).unwrap();
+
+        let mut test_pair =
+            TestPair::from_configs(&client_config.build()?, &server_config.build()?);
+        test_pair.handshake()?;
+
+        let selected_cert = test_pair.server.selected_cert().unwrap();
+        let context = selected_cert.application_context::<u64>();
+        assert_eq!(context, Some(&0xC0FFEE_u64));
+
+        Ok(())
+    }
 }

codebuild/bin/grep_simple_mistakes.sh

@@ -14,6 +14,20 @@
 
 FAILED=0
 
+#############################################
+# Grep for command line defines without values
+#############################################
+EMPTY_DEFINES=$(grep -Eon "\-D[^=]+=?" CMakeLists.txt | grep -v =)
+if [ ! -z "${EMPTY_DEFINES}" ]; then
+    FAILED=1
+    printf "\e[1;34mCommand line define is missing value:\e[0m "
+    printf "Compilers SHOULD set a default value of 1 when no default is given, "
+    printf "but that behavior is not required by any official spec. Set a value just in case. "
+    printf "For example: -DS2N_FOO=1 instead of -DS2N_FOO.\n"
+    printf "Found: \n"
+    echo "$EMPTY_DEFINES"
+fi
+
 #############################################
 # Grep for bindings methods without C documentation links.
 #############################################
@@ -74,7 +88,6 @@ KNOWN_MEMCMP_USAGE["$PWD/stuffer/s2n_stuffer_text.c"]=1
 KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_psk.c"]=1
 KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_protocol_preferences.c"]=1
 KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_cipher_suites.c"]=1
-KNOWN_MEMCMP_USAGE["$PWD/tls/s2n_config.c"]=1
 KNOWN_MEMCMP_USAGE["$PWD/utils/s2n_map.c"]=3
 
 for file in $S2N_FILES_ASSERT_NOT_USING_MEMCMP; do