-
Notifications
You must be signed in to change notification settings - Fork 65
Maintain context for key usage mismatch errors #337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
88559a5
to
ff5f073
Compare
Codecov ReportAttention: Patch coverage is
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. 🚀 New features to boost your workflow:
|
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, I think ideally for the error representation we could have an enum like: |
I was trying to figure out if I can replace the current Diff belowOne 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())
)
}
|
4ba3e6b
to
aa888cd
Compare
010c129
to
d1f591d
Compare
9bf263b
to
a864b85
Compare
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)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
a864b85
to
19f17b1
Compare
No description provided.