X509 and X509Req extensions #1790
Conversation
bkstein
commented
Jan 13, 2023
bkstein
commented
- This is part 3/4 of the split Added pkcs7 support (this PR was split, do not merge) #1598.
- It contains implementations for creating and reading X509 certs and requests. This handles X509 extensions and attributes.
- X509_PURPOSE #1788 and Preparing openssl-sys for PKCS7 and X509 extensions #1789 are prerequisites for this PR.
…o kletterstein/x509
|
@Skepfyr @sfackler I rebased this branch, as merge conflicts showed up. As this is not the first rebase, I kindly ask, if this work is still welcome. This PR was free of conflicts for quite a while, but naturally, this doesn't hold for ever. The rebases always take me some hours until the CI passes again. So this is a call for review and a polite reminder, that there is a PKCS #7 related PR, too. |
| @@ -27,6 +27,7 @@ bitflags = "1.0" | |||
| cfg-if = "1.0" | |||
| foreign-types = "0.3.1" | |||
| libc = "0.2" | |||
| num_enum = "0.5" | |||
There was a problem hiding this comment.
You don't appear to use num_enum anywhere?
| @@ -135,6 +135,10 @@ fn main() { | |||
| println!("cargo:rustc-cfg=libressl291"); | |||
| } | |||
|
|
|||
| if version >= 0x3_01_00_00_0 { | |||
There was a problem hiding this comment.
This looks like a duplicate of line 46 to me? In fact this whole block looks like a duplicate of the block on line 19? What caused you to add this? I suspect this duplication accidentally snuck in a while ago.
| @@ -84,77 +86,106 @@ impl fmt::Display for Asn1GeneralizedTimeRef { | |||
| } | |||
| } | |||
|
|
|||
| /// The type of an ASN.1 value. | |||
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | |||
| pub struct Asn1Type(c_int); | |||
There was a problem hiding this comment.
I understand the goal to name things more accurately but this is a breaking change right? I think that's probably fine but will require some more thought.
| pub const OBJECT: Asn1TagValue = Asn1TagValue(ffi::V_ASN1_OBJECT); | ||
| /// ASN.1 ObjectDescriptor | ||
| pub const OBJECT_DESCRIPTOR: Asn1TagValue = Asn1TagValue(ffi::V_ASN1_OBJECT_DESCRIPTOR); | ||
| /// Hmmm... |
There was a problem hiding this comment.
Hmm? X.680 defines external as part of ASN.1, I'm not sure what it's for but it's there (and interestingly Object Descriptors appear to be related to external types)
| pub const BMPSTRING: Asn1TagValue = Asn1TagValue(ffi::V_ASN1_BMPSTRING); | ||
|
|
||
| /// Returns the integer representation of `Padding`. | ||
| #[allow(clippy::trivially_copy_pass_by_ref)] |
There was a problem hiding this comment.
Why work around clippy rather than just taking self?
| @@ -1248,6 +1248,7 @@ mod tests { | |||
| } | |||
|
|
|||
| #[test] | |||
| #[cfg(not(ossl300))] | |||
There was a problem hiding this comment.
It seems surprising to me that this PR needs to add this, why is it not affecting everything else?
| #[corresponds(X509_get_ext_d2i)] | ||
| pub fn key_usage(&self) -> Option<&Asn1BitStringRef> { | ||
| unsafe { | ||
| let ptr = ffi::X509_get_ext_d2i( |
There was a problem hiding this comment.
For consistency with the OpenSSL API we're now implementing these APIs with a single extension function, like this: https://docs.rs/openssl/latest/openssl/x509/struct.X509Revoked.html#method.extension
| /// Get an item from the list of `ASN1_TYPE`s. | ||
| pub fn typ(&self, idx: isize) -> Result<&Asn1TypeRef, ErrorStack> { | ||
| unsafe { | ||
| let type_ptr = cvt_p(ffi::X509_ATTRIBUTE_get0_type(self.as_ptr(), idx as c_int))?; |
There was a problem hiding this comment.
This must test the length of the stack first so it doesn't do an out-of-bounds read.
| /// Returns 0 if equal. | ||
| #[corresponds(X509_NAME_cmp)] | ||
| #[allow(clippy::unnecessary_cast)] | ||
| pub fn cmp_with(&self, other: &X509NameRef) -> i32 { |
There was a problem hiding this comment.
This already (now?) exists as try_cmp.
| } | ||
| } | ||
|
|
||
| pub fn add_attribute_by_nid(&mut self, nid: Nid, value: &str) -> Result<(), ErrorStack> { |
There was a problem hiding this comment.
This pair of functions worries me, are we going to need wrappers for each type of value? Should they actually be taking an Asn1Type or Asn1String (I'm not familiar with attributes).
