-
Notifications
You must be signed in to change notification settings - Fork 172
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
instead of looking of NIDs and then using X509V3_EXT_nconf_nid, #141
Conversation
The ruby-core mailing list or this GitHub issue tracker is the right place for questions about ruby-openssl. From your email on openssl-users: Like Bob Moskowitz who has been posting about IDevID, I have also been creating certificates with custom/private extensions in aid of creating IDevIDs. I'm one of the authors of both: https://datatracker.ietf.org/doc/draft-ietf-anima-bootstrapping-keyinfra/ and https://datatracker.ietf.org/doc/draft-ietf-anima-voucher/ I want to create certificates with custom/private extensions programatically, and I found it very difficult to do using the current ruby-openssl modules. I was sure that it must be possible from the C-API, and found that this following change to ruby's interface worked well for me: https://github.com/ruby/openssl/pull/141 } I haven't found a seperate openssl-ruby list as yet, so please { } excuse the BCC, and as this is not a security issue, I haven't { } used that address. Please redirect me. { The critical change being: - ext = X509V3_EXT_nconf_nid(conf, ctx, nid, RSTRING_PTR(valstr)); + ext = X509V3_EXT_nconf(conf, ctx, RSTRING_PTR(oid), RSTRING_PTR(value)); Because EXT_nconf does all the nid lookups, and processes the generics itself, no point in the ruby-ssl code limiting itself to OIDs predefined in objects.h by it's use of nid lookups directly. ** I'm asking the Openssl team if I'm using the API reasonably ** ** Clearly, I have some possible garbage that has leaked in ** My code looks like: @idevid.add_extension(ef.create_extension( "subjectAltName", sprintf("otherName:1.3.6.1.4.1.46930.1;UTF8:%s", self.sanitized_eui64), false)) which let me put a private extension having my private hardware number into the SAN. That works just great, I think. I have in fact extracted the result in metdtls code in my constrained device a few months ago. After my changes above, I could now also do: (46930 being my PEN) @idevid.add_extension(ef.create_extension( "1.3.6.1.4.1.46930.2", "ASN1:UTF8String:http://www.sandelman.ca", false)) which would insert a MASAURL (using a PEN until we get an id-pe OID allocated) as described in: https://tools.ietf.org/html/draft-ietf-anima-bootstrapping-keyinfra-07#section-2.2 Of concern is that when I look at the resulting certificate: dooku-[fountain/spec/certs](2.3.0) mcr 10006 %openssl x509 -noout -text -in 12-00-00-66-4D-02.crt Certificate: ... X509v3 Subject Alternative Name: othername: 1.3.6.1.4.1.46930.2: ..http://www.sandelman.ca Looking at a hexdump I see "0x0c" and "0x17" prior to the http, but maybe it's a length or something.... I wondered if there was garbage or a UTF-8 BOM or something inserted.. so, I pointed asn1parse at the result, and I see: 400:d=5 hl=2 l= 9 prim: OBJECT :1.3.6.1.4.1.46930.2 411:d=5 hl=2 l= 25 prim: OCTET STRING [HEX DUMP]:0C17687474703A2F2F7777772E73616E64656C6D616E2E6361 So the 0xc and 0x17 are really there. Clearly, it's not being take as an UTF8String (because I would expect to see UTF8STRING as the type if it were), yet the ASN1:UTF8String is in fact being processed somehow. If you want to see the whole certificate result, as sample/test data to my Registrar: https://github.com/mcr/fountain/blob/master/spec/certs/12-00-00-66-4D-02.crt -- ] Never tell me the odds! | ipv6 mesh networks [ ] Michael Richardson, Sandelman Software Works | network architect [ ] [email protected] http://www.sandelman.ca/ | ruby on rails [ |
The critical change being: - ext = X509V3_EXT_nconf_nid(conf, ctx, nid, RSTRING_PTR(valstr)); + ext = X509V3_EXT_nconf(conf, ctx, RSTRING_PTR(oid), RSTRING_PTR(value)); Because EXT_nconf does all the nid lookups, and processes the generics itself, no point in the ruby-ssl code limiting itself to OIDs predefined in objects.h by it's use of nid lookups directly. NIDs can be added at run time with For whatever reason,
@idevid.add_extension(ef.create_extension( "1.3.6.1.4.1.46930.2", "ASN1:UTF8String:http://www.sandelman.ca", false)) [...] 400:d=5 hl=2 l= 9 prim: OBJECT :1.3.6.1.4.1.46930.2 411:d=5 hl=2 l= 25 prim: OCTET STRING [HEX DUMP]:0C17687474703A2F2F7777772E73616E64656C6D616E2E6361 So the 0xc and 0x17 are really there. Clearly, it's not being take as an UTF8String (because I would expect to see UTF8STRING as the type if it were), yet the ASN1:UTF8String is in fact being processed somehow. It's working as expected. The ASN.1 type definition of Extension is:
The leading "\x0c\x17" is the BER tag and the length of the UTF8String encapsulated in the 'extnValue'.
|
Thank you so much for the reply.
I will comment in the issue as requested, but I'll do so in email so that I
can CC the openssl-users list.
Kazuki Yamaguchi <[email protected]> wrote:
The ruby-core mailing list or this GitHub issue tracker is the right
place for questions about ruby-openssl.
mcr> Of concern is that when I look at the resulting certificate:
mcr> dooku-[fountain/spec/certs](2.3.0) mcr 10006 %openssl x509 -noout -text
mcr> -in 12-00-00-66-4D-02.crt Certificate: ... X509v3 Subject Alternative
mcr> Name: othername: 1.3.6.1.4.1.46930.2: ..http://www.sandelman.ca
mcr> Looking at a hexdump I see "0x0c" and "0x17" prior to the http, but
mcr> maybe it's a length or something.... I wondered if there was garbage or
mcr> a UTF-8 BOM or something inserted.. so, I pointed asn1parse at the
mcr> result, and I see:
ky> NIDs can be added at run time with OpenSSL::ASN1::ObjectId.register
ky> (which calls OBJ_create()), but yes, this should be fixed.
I did not find a way to call OBJ_create() from ruby. Is there one?
Many OpenSSL FAQs suggest you need to hack objects.h and recompile, which is
clearly a PITA if you are trying to live above distribute ruby binaries, so I
was looking for another way.
ky> For whatever reason, OpenSSL::X509::ExtensionFactory#create_ext has
ky> accepted long names which aren't handled by the non-generic extensions
ky> path of X509V3_EXT_nconf(). For compatibility I guess it will be like
ky> this...
Ah, that's why it uses that way.
I'll add that code to my tree, and update the pull request.
Are there regression tests which cover that?
I was hoping travis would tell me about such failures that I didn't know
about :-)
ky> It's working as expected. The ASN.1 type definition of Extension is:
ky> -- contains the DER encoding of an ASN.1 value
ky> The leading "\x0c\x17" is the BER tag and the length of the UTF8String
ky> encapsulated in the 'extnValue'.
okay, so "openssl x509 -text" is failing to decode that then.
# @value="http://www.sandelman.ca">
Awesome!
…--
] Never tell me the odds! | ipv6 mesh networks [
] Michael Richardson, Sandelman Software Works | network architect [
] [email protected] http://www.sandelman.ca/ | ruby on rails [
|
OpenSSL::ASN1::ObjectId.register (in ext/openssl/ossl_asn1.c) calls.
No, sadly. It would be helpful if you could add some to test/test_x509ext.rb. |
I have revised the code:
|
I will investigate adding a regress test case for this, can you give me an example of the nid lookup that would not have worked without the ln2nid() call? |
The changes in ext/openssl/ossl_x509ext.c look good. Thanks.
For example...
|
@mcr do you mind rebasing on master and explaining where we are at with this PR? |
Just say that I haven't lost track of this.
I just had less time/energy while travelling than I thought I would.
|
It seems like we can now look up `X509V3_EXT`s directly by their name, instead of having to go through the NID system. - With inspiration from (open and unmerged for 4 years) ruby/openssl#141, use `X509V3_EXT_nconf` whenever the NID is *not* known at build time - As a result, `new_from_X509V3_EXT_METHOD()` now takes a string instead of an NID as the second parameter - Relax failure test over unsupported CRL extensions (so as to OK on the new error message)
I'm sorry it took so long to reply. This patch is still relevant and the changes in ext/openssl looks good to me. Let me add some test cases. |
d1c9078
to
f638f2e
Compare
The test case test_error_data utilizes the error message generated by X509V3_EXT_nconf_nid(). The next commit will use X509V3_EXT_nconf(), which generates a slightly different error message. Let's adapt the check to it.
OpenSSL::X509::ExtensionFactory#create_ext and #create_extensions accepts both sn (short names) and ln (long names) for registered OIDs. This is different from the behavior of the openssl command-line utility which accepts only sn in openssl.cnf keys. Add a test case to check this.
instead of looking of NIDs and then using X509V3_EXT_nconf_nid, instead just pass strings to X509V3_EXT_nconf, which has all the logic for processing dealing with generic extensions also process the oid through ln2nid() to retain compatibility. [rhe: tweaked commit message and added a test case]
I had to add a cast from |
Given how insecure OpenSSL 1.0.x is, do we really care? |
The openssl/ruby currently maintains the old OpenSSL versions even when these are the end of life (EOL). openssl/.github/workflows/test.yml Lines 78 to 82 in fcda6cf
As a real use case, Red Hat Enterprise Linux (RHEL) 7's latest version RHEL 7.9 has OpenSSL version 1.0.2k in my investigation. As RHEL 7's end of life is in June 2024, according to this announcement. I assume that some people may use the openssl gem in the environment until the EOL, June 2024. |
I personally don't believe OpenSSL 1.0.2 support is a strict requirement for us, but we should keep master branch compatible with 1.0.2 until we officially drop it. |
CI is happy with |
Thank you so much! |
Nice work team! |
instead just pass strings to X509V3_EXT_nconf, which has all the logic for
processing dealing with generic extensions.
This is not yet right; it may insert two additional bytes to the value, and does not deal with the critical correctly. This pull request is for discussion.