Skip to content

Conversation

djc
Copy link
Member

@djc djc commented Mar 27, 2025

No description provided.

@djc djc requested review from cpu and ctz March 27, 2025 12:44
@djc djc force-pushed the eku-not-found-context branch from 88559a5 to ff5f073 Compare March 27, 2025 12:44
Copy link

codecov bot commented Mar 27, 2025

Codecov Report

Attention: Patch coverage is 97.61905% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.91%. Comparing base (85d885d) to head (19f17b1).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/verify_cert.rs 97.60% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #337      +/-   ##
==========================================
+ Coverage   97.76%   97.91%   +0.15%     
==========================================
  Files          20       20              
  Lines        4348     4470     +122     
==========================================
+ Hits         4251     4377     +126     
+ Misses         97       93       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@djc
Copy link
Member Author

djc commented Mar 28, 2025

I think we should probably figure out the rustls part of this before we merge/release it.

@ctz
Copy link
Member

ctz commented Apr 4, 2025

I think we should probably figure out the rustls part of this before we merge/release it.

Since the code that originates these errors is server- or client-specific, we could possibly make the error variants also be specific to that. eg, CertificateError::CertificateCannotAuthenticateServer { found_purposes: Vec<ExtendedKeyPurpose> }

I think ideally for the error representation we could have an enum like: enum ExtendedKeyPurpose { ServerAuth, ClientAuth, Unrecognized(Vec<u32>) } where Unrecognized is the decoded OID numerals. But decoding those would be net-new code that we currently don't have or need.

@djc
Copy link
Member Author

djc commented Apr 9, 2025

I think ideally for the error representation we could have an enum like: enum ExtendedKeyPurpose { ServerAuth, ClientAuth, Unrecognized(Vec<u32>) } where Unrecognized is the decoded OID numerals. But decoding those would be net-new code that we currently don't have or need.

I was trying to figure out if I can replace the current KeyPurposeId with something like your proposed ExtendedKeyPurpose, but that quickly runs into issues: practically this type needs to be Copy for efficient verification and given the constraints of the existing public API, but in RequiredEkuNotFoundContext it needs to own the allocation of a peer-supplied OID. We could only have the enum for use in RequiredEkuNotFoundContext but that feels like too much complexity too?

Diff below One attempt:
diff --git a/src/lib.rs b/src/lib.rs
index b285de2..1e812a6 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -82,7 +82,7 @@ pub use {
     error::{DerTypeId, Error, InvalidNameContext},
     rpk_entity::RawPublicKeyEntity,
     trust_anchor::anchor_from_trusted_cert,
-    verify_cert::{KeyUsage, RequiredEkuNotFoundContext, VerifiedPath},
+    verify_cert::{KeyPurposeId, KeyUsage, RequiredEkuNotFoundContext, VerifiedPath},
 };
 
 #[cfg(feature = "alloc")]
diff --git a/src/verify_cert.rs b/src/verify_cert.rs
index 8476904..c560602 100644
--- a/src/verify_cert.rs
+++ b/src/verify_cert.rs
@@ -439,7 +439,7 @@ pub struct RequiredEkuNotFoundContext {
     pub required: KeyUsage,
     /// The ExtendedKeyUsage OIDs present in the certificate.
     #[cfg(feature = "alloc")]
-    pub present: Vec<Vec<u8>>,
+    pub present: Vec<KeyPurposeId<'static>>,
 }
 
 /// The expected key usage of a certificate.
@@ -460,14 +460,18 @@ impl KeyUsage {
     ///
     /// As specified in <https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.12>, this does not require the certificate to specify the eKU extension.
     pub const fn server_auth() -> Self {
-        Self::required_if_present(EKU_SERVER_AUTH)
+        Self {
+            inner: ExtendedKeyUsage::RequiredIfPresent(KeyPurposeId::ServerAuth),
+        }
     }
 
     /// Construct a new [`KeyUsage`] as appropriate for client certificate authentication.
     ///
     /// As specified in <>, this does not require the certificate to specify the eKU extension.
     pub const fn client_auth() -> Self {
-        Self::required_if_present(EKU_CLIENT_AUTH)
+        Self {
+            inner: ExtendedKeyUsage::RequiredIfPresent(KeyPurposeId::ClientAuth),
+        }
     }
 
     /// Construct a new [`KeyUsage`] requiring a certificate to support the specified OID.
@@ -545,16 +549,21 @@ impl ExtendedKeyUsage {
                 Self::Required(eku) => *eku,
                 Self::RequiredIfPresent(eku) => *eku,
             }
-            .oid_value,
+            .oid_value(),
             value,
         )
     }
 }
 
 /// An OID value indicating an Extended Key Usage (EKU) key purpose.
-#[derive(Clone, Debug, Copy)]
-struct KeyPurposeId<'a> {
-    oid_value: untrusted::Input<'a>,
+#[derive(Clone, Copy, Debug)]
+pub enum KeyPurposeId<'a> {
+    /// Client certificate authentication.
+    ClientAuth,
+    /// Server certificate authentication.
+    ServerAuth,
+    /// Other key usage purposes, as an OBJECT IDENTIFIER in bytes.
+    Other(&'a [u8]),
 }
 
 impl<'a> KeyPurposeId<'a> {
@@ -562,42 +571,49 @@ impl<'a> KeyPurposeId<'a> {
     ///
     /// `oid` is the OBJECT IDENTIFIER in bytes.
     const fn new(oid: &'a [u8]) -> Self {
-        Self {
-            oid_value: untrusted::Input::from(oid),
+        match oid {
+            Self::CLIENT_AUTH => Self::ClientAuth,
+            Self::SERVER_AUTH => Self::ServerAuth,
+            _ => Self::Other(oid),
+        }
+    }
+
+    /// Get the OID value of this [`KeyPurposeId`].
+    fn oid_value(&self) -> untrusted::Input<'a> {
+        match self {
+            Self::ClientAuth => untrusted::Input::from(Self::CLIENT_AUTH),
+            Self::ServerAuth => untrusted::Input::from(Self::SERVER_AUTH),
+            Self::Other(oid) => untrusted::Input::from(oid),
         }
     }
+
+    /// id-kp-serverAuth   OBJECT IDENTIFIER ::= { id-kp 1 }
+    ///
+    /// id-pkix            OBJECT IDENTIFIER ::= { 1 3 6 1 5 5 7 }
+    /// id-kp              OBJECT IDENTIFIER ::= { id-pkix 3 }
+    pub const SERVER_AUTH: &'static [u8] = &oid!(1, 3, 6, 1, 5, 5, 7, 3, 1);
+
+    /// id-kp-serverAuth   OBJECT IDENTIFIER ::= { id-kp 2 }
+    ///
+    /// id-pkix            OBJECT IDENTIFIER ::= { 1 3 6 1 5 5 7 }
+    /// id-kp              OBJECT IDENTIFIER ::= { id-pkix 3 }
+    pub const CLIENT_AUTH: &'static [u8] = &oid!(1, 3, 6, 1, 5, 5, 7, 3, 2);
 }
 
 impl fmt::Display for KeyPurposeId<'_> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "KeyPurposeId(")?;
-        for (i, byte) in self.oid_value.as_slice_less_safe().iter().enumerate() {
-            if i > 0 {
-                write!(f, ".")?;
-            }
-            write!(f, "{byte}")?;
-        }
-        write!(f, ")")
+        write!(f, "KeyPurposeId::{:?}", self)
     }
 }
 
 impl PartialEq<Self> for KeyPurposeId<'_> {
     fn eq(&self, other: &Self) -> bool {
-        public_values_eq(self.oid_value, other.oid_value)
+        self.oid_value().as_slice_less_safe() == other.oid_value().as_slice_less_safe()
     }
 }
 
 impl Eq for KeyPurposeId<'_> {}
 
-// id-pkix            OBJECT IDENTIFIER ::= { 1 3 6 1 5 5 7 }
-// id-kp              OBJECT IDENTIFIER ::= { id-pkix 3 }
-
-// id-kp-serverAuth   OBJECT IDENTIFIER ::= { id-kp 1 }
-const EKU_SERVER_AUTH: &[u8] = &oid!(1, 3, 6, 1, 5, 5, 7, 3, 1);
-
-// id-kp-clientAuth   OBJECT IDENTIFIER ::= { id-kp 2 }
-const EKU_CLIENT_AUTH: &[u8] = &oid!(1, 3, 6, 1, 5, 5, 7, 3, 2);
-
 fn loop_while_non_fatal_error<'a, V: IntoIterator + 'a>(
     default_error: Error,
     values: V,
@@ -757,8 +773,8 @@ mod tests {
     #[test]
     fn eku_key_purpose_id() {
         assert!(
-            ExtendedKeyUsage::RequiredIfPresent(KeyPurposeId::new(EKU_SERVER_AUTH))
-                .key_purpose_id_equals(KeyPurposeId::new(EKU_SERVER_AUTH).oid_value)
+            ExtendedKeyUsage::RequiredIfPresent(KeyPurposeId::ServerAuth)
+                .key_purpose_id_equals(KeyPurposeId::Other(KeyPurposeId::SERVER_AUTH).oid_value())
         )
     }
 

@djc djc force-pushed the eku-not-found-context branch 3 times, most recently from 4ba3e6b to aa888cd Compare April 10, 2025 15:10
@djc djc force-pushed the eku-not-found-context branch 3 times, most recently from 010c129 to d1f591d Compare April 13, 2025 10:18
@djc djc force-pushed the eku-not-found-context branch 3 times, most recently from 9bf263b to a864b85 Compare April 13, 2025 11:37
@djc djc requested review from ctz and cpu April 22, 2025 08:18
@djc
Copy link
Member Author

djc commented Apr 22, 2025

I made a bunch of changes here, and I feel like this is in a good state now. Want to do another round of review (perhaps in conjunction with rustls/rustls#2426)?

Copy link
Member

@ctz ctz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@djc djc force-pushed the eku-not-found-context branch from a864b85 to 19f17b1 Compare April 22, 2025 13:45
@djc djc added this pull request to the merge queue Apr 22, 2025
Merged via the queue into main with commit 9cf30f6 Apr 22, 2025
63 of 64 checks passed
@djc djc deleted the eku-not-found-context branch April 22, 2025 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants