-
-
Notifications
You must be signed in to change notification settings - Fork 740
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
Add two methods to the PKCS7 API #2111
Conversation
bd957d7
to
106c563
Compare
ec165c0
to
4fc1713
Compare
4fc1713
to
74859f5
Compare
74859f5
to
b7d6066
Compare
openssl/src/pkcs7.rs
Outdated
@@ -281,11 +296,37 @@ impl Pkcs7Ref { | |||
Ok(stack) | |||
} | |||
} | |||
|
|||
// Return the type of a PKCS#7 structure as an Asn1Object | |||
pub fn type_oid(&self) -> Option<&Asn1ObjectRef> { |
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.
Should this just be called type
?
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.
Can't use that here, unfortunately, because type
is a strict keyword.
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.
It could be r#type
instead to use the raw identifier syntax, but yeah -- I think bare type
won't work.
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.
You can go with a raw ident, but type_
is a bit easier to work with.
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.
Fixed (changed to type_
)
openssl/src/pkcs7.rs
Outdated
} | ||
|
||
// Retrieve all the certificates from a PKCS#7 structure used for signed data | ||
pub fn signed_data_certificates(&self) -> Result<Option<&StackRef<X509>>, ErrorStack> { |
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.
I think I'd rather go "1 step" here - instead having a pub fn signed(&self) -> Option<&Pkcs7SignedRef>
method.
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.
Wouldn't this mean that users of this library would have to use unsafe code to extract the stack of certificates from the Pkcs7SignedRef
? Since they would have to do the extra step that this function now does:
.and_then(|x| x.cert.as_mut())
.and_then(|x| StackRef::<X509>::from_const_ptr_opt(x));
which is unsafe.
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.
I assume @sfackler was suggesting that Pkcs7SignedRef
should have safe methods for accessing this data.
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.
Got it. Fixed, now the stack of certificates can be accessed in two steps:
let signed_certificates = pkcs7.signed().and_then(|x| x.certificates());
1ef2608
to
5630689
Compare
5630689
to
3d69fe9
Compare
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.
Will hold off on merging for @sfackler to have a look.
Sorry for the delay! LGTM |
This PR adds two new methods:
Pkcs7Ref::type_
returns the type of the PKCS#7 object (signed, envelope, etc)Pkcs7Ref::signed
returns the PKCS7_SIGNED member of a signed PKCS#7 object (as aPkcs7SignedRef
)It also adds a
Pkcs7SignedRef::certificates
method, to access the stack of certificates of a signed PKCS7 object.The motivation behind adding these methods is that
cryptography
is migrating its PKCS7 backend implementation from Python to Rust.Here is the Python code that would be replaced by Rust, which manually access the data structures that this PR exposes through the API.
One thing I'm not sure about (in
certificates()
) is my handling of the Stack as a reference, and the ownership status of the certificates inside of it, so any suggestions/fixes are welcome.