openssl
openssl copied to clipboard
ruby
PKCS7 unsigned/signed attributes support
Hi there!
I'm attempting to implement a SCEP server in Ruby to produce certificates for Apple devices as part of a device management solution. As part of this, Apple confirms that the signed PKCS7 response contains both a senderNonce and recipientNonce in the signedAttributes field of the PKCS7 message. micromdm/scep has an implementation of this process written in Go.
I've spent my weekend attempting to add support for PKCS7 attribute assignment to openssl-ruby, but I've hit a wall, hence the draft PR. Specifically, I have some compile errors (submitted below the fold) I've run into, and before continuing I thought it best to start a discussion about my approach to confirm I'm on the right track, and how to proceed.
I should note now that I'm very new to both C extension development & the inner workings of OpenSSL/X509/ASN.1, and I understand I may have taken an incorrect approach. Please don't hesitate to let me know how I can improve, and I would be more than happy to donate this pull request to someone more qualified so they can nurture this to completion. That said, I would really like to see the capability added to Ruby (especially as I'm not the only one asking for it), so I'm more than happy to continue work to get this PR into a mergeable state.
At this juncture, I would like a confirmation as to my approach, and the answers to the following questions:
- Is it okay to use
obj_to_asn1obj&ossl_asn1_get_valuefrom outside ofossl_asn1.c? This would solve my compile error, but given my unfamiliarity with C I'm not sure how to make this happen. I'm pretty confident I only need a header declaration, but I'm sensitive to unintentionally clashing with another function somewhere. - I need to unwrap a given "value" from my attribute from a Ruby-style argument into a
void *valuesuitable forASN1_TYPE_set(viaX509_ATTRIBUTE_createviaadd_attributeviaPKCS7_add_signed_attribute). We already do this with a long case statement inossl_asn1_get_asn1type— would it be okay to break this case statement out ofget_asn1type? I'm currently callingget_asn1typethen pulling the "value" from the returnedASN1_TYPE, which does not seem efficient or wise. - Am I correct in that a "tag" is the same as an "attrtype" for the purposes of
PKCS7_add_signed_attribute? It seems that the last two arguments toPKCS7_add_signed_attributeare provided by the single typedvaluein#add_signed_attribute(oid, value) - How can I confirm that an
ObjectIdis anObjectIdin C? - I've added the
OpenSSL::PKCS7::PARTIALconstant but neglectedPKCS7_STREAM, as I do not have a use case nor understand it. Is it acceptable to only continue withPARTIALsupport? - Because of OpenSSL's reliance on "numerical IDs" & the lack of connection in openssl-ruby between the numerical ID, we would need to raise an error when an un-registered OID is passed into any
add_attribute-style method. Is it okay to explain in this error what's necessary to fix the issue? The PKCS7 class is notably devoid of documentation, which I'm not sure is intentional or not.
Additionally I'm struggling to understand ASN.1 as a concept, especially coming from JSON/Protobuffers land where I have the ability to parse a structure into something with labels. If anyone has any further reading that could assist with this, I would be more than grateful.
Thank you for reading & maintaining this library, and I hope we can work together to get this pull request over the finish line.
Current compile errors
-> % rake compile
cd tmp/x86_64-darwin20/openssl/3.0.0
/usr/bin/make
compiling ../../../../ext/openssl/ossl_pkcs7.c
../../../../ext/openssl/ossl_pkcs7.c:1011:13: error: implicit declaration of function 'obj_to_asn1obj' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
a1obj = obj_to_asn1obj(ossl_asn1_get_value(oid)); // TODO: error check
^
../../../../ext/openssl/ossl_pkcs7.c:1011:28: error: implicit declaration of function 'ossl_asn1_get_value' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
a1obj = obj_to_asn1obj(ossl_asn1_get_value(oid)); // TODO: error check
^
../../../../ext/openssl/ossl_pkcs7.c:1011:11: warning: incompatible integer to pointer conversion assigning to 'ASN1_OBJECT *' (aka 'struct asn1_object_st *') from 'int' [-Wint-conversion]
a1obj = obj_to_asn1obj(ossl_asn1_get_value(oid)); // TODO: error check
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../ext/openssl/ossl_pkcs7.c:1025:11: error: implicit declaration of function 'ossl_asn1_tag' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
tag = ossl_asn1_tag(value); // TODO: error check
^
../../../../ext/openssl/ossl_pkcs7.c:1066:61: error: member reference type 'ASN1_TYPE *' (aka 'struct asn1_type_st *') is a pointer; did you mean to use '->'?
PKCS7_add_signed_attribute(p7si, nid, tag, value_as_type.value);
~~~~~~~~~~~~~^
->
../../../../ext/openssl/ossl_pkcs7.c:1066:48: error: passing 'union (anonymous union at /Users/rubynerd/.rbenv/versions/3.0.0/openssl/include/openssl/asn1.h:446:5)' to parameter of incompatible type 'void *'
PKCS7_add_signed_attribute(p7si, nid, tag, value_as_type.value);
^~~~~~~~~~~~~~~~~~~
/Users/rubynerd/.rbenv/versions/3.0.0/openssl/include/openssl/pkcs7.h:274:38: note: passing argument to parameter 'data' here
void *data);
^
1 warning and 5 errors generated.
make: *** [ossl_pkcs7.o] Error 1
rake aborted!
Command failed with status (2): [/usr/bin/make...]
testharness.rb
CERTIFICATE = OpenSSL::X509::Certificate.new(File.read("./scep.pem"))
PRIVATE_KEY = OpenSSL::PKey::RSA.new(File.read("./scep-key.pem"))
# fake response, would be from OpenSSL::PKCS7.encrypt
response = OpenSSL::ASN1::Sequence.new([
OpenSSL::ASN1::ObjectId.new('1.2.840.113549.1.7.2'),
])
puts "OpenSSL::PKCS7::PARTIAL: #{OpenSSL::PKCS7::PARTIAL}"
res = OpenSSL::PKCS7.sign(CERTIFICATE, PRIVATE_KEY, nil, [], OpenSSL::PKCS7::BINARY | OpenSSL::PKCS7::PARTIAL)
puts "res.signers: #{res.signers}"
# effectively:
# res.signers.first.add_signed_attribute(OpenSSL::ASN1::ObjectId.new('1.x.x'), OpenSSL::ASN1::OctetString.new(""))
res.finalize(response.to_der, OpenSSL::PKCS7::BINARY | OpenSSL::PKCS7::PARTIAL)
puts res.to_pem
Thank you for your time looking into this!
Unfortunately, the PKCS#7 module hasn't got much attention since it was initially implemented in ~2005 and it's still lacking core features and documentation. :(
signedAttrs/unsignedAttrs is specified in RFC 5652:
SignerInfo ::= SEQUENCE {
[...],
signedAttrs [0] IMPLICIT SignedAttributes OPTIONAL,
[...],
unsignedAttrs [1] IMPLICIT UnsignedAttributes OPTIONAL }
SignedAttributes ::= SET SIZE (1..MAX) OF Attribute
UnsignedAttributes ::= SET SIZE (1..MAX) OF Attribute
Attribute ::= SEQUENCE {
attrType OBJECT IDENTIFIER,
attrValues SET OF AttributeValue }
AttributeValue ::= ANY
[...]
attrValues is a set of values that comprise the attribute. The
type of each value in the set can be determined uniquely by
attrType. The attrType can impose restrictions on the number of
items in the set.
It seems that PKCS7_add_signed_attribute(si, nid, attrtype_of_value, value) is a specialized function to create an Attribute containing a single-valued SET as attrValues (and replace the current Attribute in signedAttrs with the same attrType=nid with the new one, if exists).
It's probably useful to write programs in C, but I suspect it would be simpler for us to implement a wrapper for the lower-level function PKCS7_set_signed_attributes() since we have OpenSSL::X509::Attribute to represent an Attribute object. It's currently used by OpenSSL::X509::Request, aka. PKCS#10. So we can use something like:
attrValue = OpenSSL::ASN1::Set.new([OpenSSL::ASN1::OctetString.new("data")])
si = OpenSSL::PKCS7::SignerInfo.new(..)
si.signed_attributes = [
OpenSSL::X509::Attribute.new("attrType", attrValue),
]
- Is it okay to use
obj_to_asn1obj&ossl_asn1_get_valuefrom outside ofossl_asn1.c? This would solve my compile error, but given my unfamiliarity with C I'm not sure how to make this happen. I'm pretty confident I only need a header declaration, but I'm sensitive to unintentionally clashing with another function somewhere.
They are not needed if we go with PKCS7_set_signed_attributes(), but - yes. Habitually ossl_ prefix is added to identifiers when exporting.
- I need to unwrap a given "value" from my attribute from a Ruby-style argument into a
void *valuesuitable forASN1_TYPE_set(viaX509_ATTRIBUTE_createviaadd_attributeviaPKCS7_add_signed_attribute). We already do this with a long case statement inossl_asn1_get_asn1type— would it be okay to break this case statement out ofget_asn1type? I'm currently callingget_asn1typethen pulling the "value" from the returnedASN1_TYPE, which does not seem efficient or wise.
OpenSSL::X509::Attribute#value= currently does this. The only other user of ossl_asn1_get_asn1type, ASN1#to_der, doesn't really need ASN1_TYPE either, so I think it should be refactored.
- Am I correct in that a "tag" is the same as an "attrtype" for the purposes of
PKCS7_add_signed_attribute? It seems that the last two arguments toPKCS7_add_signed_attributeare provided by the single typedvaluein#add_signed_attribute(oid, value)
I think so. PKCS7_add_signed_attribute()'s third parameter seems to be corresponding to the type of AttributeValue.
- How can I confirm that an
ObjectIdis anObjectIdin C?- I've added the
OpenSSL::PKCS7::PARTIALconstant but neglectedPKCS7_STREAM, as I do not have a use case nor understand it. Is it acceptable to only continue withPARTIALsupport?
I don't understand the difference. They were identical when it was first introduced. https://github.com/openssl/openssl/commit/60f20632e27d353e76e62b6746879e1d8e292e06
I think PARTIAL only is fine.
- Because of OpenSSL's reliance on "numerical IDs" & the lack of connection in openssl-ruby between the numerical ID, we would need to raise an error when an un-registered OID is passed into any
add_attribute-style method. Is it okay to explain in this error what's necessary to fix the issue? The PKCS7 class is notably devoid of documentation, which I'm not sure is intentional or not.
To my understanding, NIDs are mostly for convenience (so C programs can use NID_* macro) and there should be a way to pass ASN1_OBJECT instead, maybe as a lower-level function.
@rhenium so I've taken a stab at PKCS7::SignerInfo#signed_attributes= but unfortunately I've run into an issue: it simply isn't working. The code compiles and runs, but the resulting PKCS7 message doesn't contain any signed attributes.
I think something's going on with DupX509AttrPtr, and it's not correctly duplicating/retaining references, but I can't find an up_ref function for X509_ATTRIBUTE, and honestly I'm understanding why OpenSSL::PKCS7 doesn't have much in the way of documentation given the underlying OpenSSL code :sob:
Do you have any advice for how to debug this issue?
A problem is that OpenSSL::PKCS7#add_signer was not working correctly at all. https://github.com/ruby/openssl/pull/423
I've moved this to the backburner as I've changed languages for this project, but I think I've got the PKCS7 authenticated attribute setter working! Testing just a setter is quite questionable though, so I think for feature completeness this PR will need a getter for the attributes of a parsed message as well. I'll figure this one out when I've got some spare cycles 👍