OpenSSL backend code for CRLs

[etrauschke]

This is the backend code for the CRL objects. Looks like a bunch of additional automatic tests have been added so let me make sure they clear first.

[etrauschke]

Looks like I have to change the test certs so that the 0.9.8 tests will not see a cert issuer extension marked as critical. Hopefully I can reuse some other cert for this, otherwise I might have to make a change to the vectors again.

[etrauschke]

It looks like I'll have to change the test cert crl_all_reasons.pem so that the cert_issuer extension is not critical. Otherwise I would have to skip a bunch of tests in the 0.9.8 environments.

[codecov-io]

Current coverage is 99.98%

Merging #2315 into master will not affect coverage as of 4e0e10c

@@            master   #2315   diff @@
======================================
  Files          120     120       
  Stmts        12247   12495   +248
  Branches      1350    1366    +16
  Methods          0       0       
======================================
+ Hit          12245   12493   +248
  Partial          2       2       
  Missed           0       0       

Review entire Coverage Diff as of 4e0e10c

Powered by Codecov. Updated on successful CI builds.

[etrauschke]

So I'm not sure what the coverage thing is complaining about, I guess it's because the for loop never runs completely through (intentionally).
I guess if that is really important I can change the line to something like this:

for r in crl.revoked_certificates.append(dummy_rev):

[etrauschke]

Ended up completely changing the logic of the reason flag test. Coverage seems happy now.

[alex]

Jenkins, retest this please.

[reaperhulk]

jenkins, retest this please

[etrauschke]

Anybody could have a look at this?

[reaperhulk]

This should be 1.1 now

[reaperhulk]

It doesn't look like your merge captured all the commits (it's definitely missing the PR you opened earlier).

[etrauschke]

Oh, please ignore this last push I merged my master and just did a push without specifying the branch so it updated this one unintentionally as well. I'm still testing my actual changes and will push once again once I('m done.

[etrauschke]

Now the PR is ready for review. If this gets a bit cluttered I can also create a new PR.

[reaperhulk]

I know this is in our current interface, but I'm wondering if it might be more consistent for us to just make CertificateRevocationList an iterable? That would look more like the way Extensions, SubjectAlternativeName, and several other x509 classes behave.

[etrauschke]

So you just want me to get rid of revoked_certificates() and replace it with an iter() and len() function? I can do that, I just tried to stay as close to the OpenSSL specification because I think this makes it easier for people to find out how to access the revoked certs.

[reaperhulk]

Yeah, I think that's consistent with our current approach for x509 objects.

[reaperhulk]

Let's also discuss the caching in a separate PR since it'd be awesome to land this and caching is orthogonal to the vast majority of the work here. (Sorry to make you do some extra work!)

[reaperhulk]

It looks like the certificateIssuer revoked certificate extension is also something we can handle in 0.9.8. It's true that calling X509V3_EXT_d2i on the ext doesn't work, but in this case we can do the following:

ext is an X509_EXTENSION * which has an ASN1_OCTET_STRING *value.
An ASN1_OCTET_STRING * is a typedef for the struct asn1_string_st which has int length and unsigned char *data.
We can bind GENERAL_NAMES *d2i_GENERAL_NAMES(GENERAL_NAMES **, const unsigned char **, long); which should allow us to use the ASN1_OCTET_STRING data/length to obtain the GENERAL_NAMES * that we need to pass to the _decode_general_names function.

[etrauschke]

@reaperhulk With the CRLExtensionOID I probably should have paid more attention when this change was made. This is unfortunate because it is basically wrong in the current form. They are not CRL extensions. Not sure if you would break anyones code yet since the backend support hasn't been in so the number of people using it is probably 0. However, I understand how complicated it is to change committed interfaces. Just one thing to keep in mind, if the backend code goes back, people might start using these extensions and then it gets even harder to change things around.

[etrauschke]

The plan with the certificateIssuer sounds pretty nifty but how would we integrate this with the X509ExtensionParser which does run X509V3_EXT_d2i for us. We would have to shoehorn some extra code in there which seems ugly. Maybe we can look at this again if we find additional extensions which require newer OpenSSL versions to be properly supported.

[etrauschke]

Before I forget it, I also changed the exception message for unsupported critical extensions so that it actually tells you that it is critical. Otherwise the message is confusing because the rule is that unknown exceptions are to be ignored unless they are marked critical.

[reaperhulk]

Right now there are no extensions that we parse conditionally on underyling OpenSSL version. I'd very much prefer to keep it that way since it's extremely difficult to explain to users how the "same" version of cryptography might exhibit significantly different behavior. At the very least we need to raise a warning.

It is a shame that the naming is wrong (and that's on me!), but we can transition away from that name with our standard deprecation strategy without too much pain. We can put in a PR to change the name after this PR, update the docs to all point at the new name, but then keep the old name for compatibility for the next 2 releases. It would then be completely dropped in the 1.3 release.

[etrauschke]

Ok, let me take a stab at the certIssuer custom parsing. I'll also remove the caching stuff for now and undo the naming change for CRLExtensionOID.

[etrauschke]

Found a way to make this work, let me know what you think.
I had to add a new CRL pem file to vectors which includes an empty/invalid certIssuer entry extension. This is necessary for code coverage since I can't use the generic bailout for an invalid extension anymore. Let me know if you want me to push that in a separate PR. Also, I had to add a new binding for *d2i_GENERAL_NAMES which you might also want in a separate PR. So just let me know and I'll do that.

[reaperhulk]

Yeah if you wouldn't mind dropping those in separate PRs we can get them landed immediately :)

[reaperhulk]

Can you walk me through the logic here? How was this extension encoded badly such that it should error? What makes the empty extension trigger the extension is invalid path?

[etrauschke]

What I did is I created a new custom extension which has the same OID as the certificate issuer extension but has a custom value. The way I did this is like that (in C using OpenSSL):

X509_REVOKED *r = X509_REVOKED_new();
int nid = OBJ_create("2.5.29.29", "Test", "Test Extension");
X509_REVOKED_add1_ext_i2d(r, nid, NULL, 0, 0);

Then when we get to _decode_cert_issuer(backend, ext) we get a NULL pointer as data_ptr_ptr (or something else which isn't a GENERAL_NAMES type) which goes into d2i_GENERAL_NAMES() and returns a NULL pointer for which I explicitly test.

If that is not good enough, I could also create this CRL so that it sets an integer instead of a NULL value. Everything which is not GENERAL_NAMES should trigger this. However, I think at that point we are fairly far out in terms of error cases, I expect this code to never be triggered in the real world. I just needed something to make coverage happy.

[reaperhulk]

I'm okay with this -- just wanted to make sure I understood the approach. Thanks!

[etrauschke]

Now that I think about this, I wonder if we should just silently ignore it. I mean for the other extensions we don't check if the values actually make sense and we can pass a NULL value to _decode_general_names(), which just leads to an empty GeneralNames object.

[reaperhulk]

What about just creating a bad extension (use an OID that doesn't make sense for the context, like EKU or KU)?

[reaperhulk]

Sorry, I owe you a review of this updated code again. Will get to it tonight.

[etrauschke]

Cool, thanks. I just wanted to see if we need to change the behavior again, because before we have decided on what should happen you might just waste your time reviewing code which needs to be changed anyway.

[reaperhulk]

Okay, looking at this more I'd prefer it to ignore it like a normal NULL. We should already be triggering this code path with another test so we shouldn't need a special test case at all now that you're using the ExtensionParser class, right?

[etrauschke]

Yes, for everything which goes through the regular processing with X509V3_EXT_d2i we already handle the NULL case and I haven't change any of that code. It would be just about the certIssuer extension parsing.

[reaperhulk]

Let's leave this as is with just an additional comment next to that value error that states it's present because we're parsing the extension ourselves and can't rely on the check in the extensionparser.

[reaperhulk]

I've restarted the CI job in #2417. Once that merges you can rebase this and add the two comments and I'll do a final review. Once again, thanks for sticking with this!

[reaperhulk]

The vector has now merged. If you add those two comments I'll do a final paranoia-based review to make sure I haven't missed anything and we can get this merged!

[reaperhulk]

PKCS10 defines CSRs -- is there anything about CRLs in there?

[reaperhulk]

I have a few small things I'd like to clean up but I'm satisfied that it's ready for merge now. Huge thanks @etrauschke. Be sure to send a PR adding yourself to AUTHORS.rst when you get a chance!

[coveralls]

Changes Unknown when pulling cee79f8 on etrauschke:crl_ossl_backend into ** on pyca:master**.