support CRLDistributionPoints in the OpenSSL backend #1920

reaperhulk · 2015-05-10T21:46:39Z

reaperhulk · 2015-05-10T21:46:58Z

(note: _build_general_names should be used to replace a bunch of boilerplate in a future PR once this merges)

alex · 2015-05-10T21:47:59Z

I'm not sure I understand this comment.

reaperhulk · 2015-05-10T21:49:03Z

If you mean the comment I just put on this, that's a note to myself because I added _build_general_names here but I can use that in like 4 other extensions we've already landed. Putting those changes in this PR would not be appropriate though.

alex · 2015-05-10T21:51:09Z

Can we do it in the reverse order, a PR to use that first?

On Sun, May 10, 2015 at 5:49 PM, Paul Kehrer notifications@github.com
wrote:

If you mean the comment I just put on this, that's a note to myself
because I added _build_general_names here but I can use that in like 4
other extensions we've already landed. Putting those changes in this PR
would not be appropriate though.

—
Reply to this email directly or view it on GitHub
#1920 (comment).

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

reaperhulk · 2015-05-10T21:55:56Z

Sure, #1921 (this will be rebased against that once it merges)

coveralls · 2015-05-10T22:13:53Z

Coverage remained the same at 100.0% when pulling 6c76f2b on reaperhulk:x509-ossl-cdp into cadbdc6 on pyca:master.

coveralls · 2015-05-10T22:13:53Z

Coverage remained the same at 100.0% when pulling 6c76f2b on reaperhulk:x509-ossl-cdp into cadbdc6 on pyca:master.

coveralls · 2015-05-10T22:13:54Z

Coverage remained the same at 100.0% when pulling 6c76f2b on reaperhulk:x509-ossl-cdp into cadbdc6 on pyca:master.

alex · 2015-05-10T22:34:25Z

@reaperhulk ready for that rebase

reaperhulk · 2015-05-11T01:35:34Z

This has been rebased

etrauschke · 2015-05-11T17:00:56Z

Ok, I was just about to adjust my changes to your last putback but i see you are already way ahead.
One thing, I didn't see you add any additional certs for test cases so it seems you found some for what you wanted to test. If you also want to test the relative name for the dist point, have a look at revocation_cert_valid.pem from my patch. These are kinda weird to create since for some reason the openssl CLI client won't allow you to create one with more than one fragment.
If you want I can also add the test myself.

reaperhulk · 2015-05-11T18:48:29Z

Yeah @etrauschke I based the work on your patch and expanded it from there. We do have a relative name test in here already (see line 1668).

etrauschke · 2015-05-11T21:30:10Z

Yes, but you don't test that decoding an actual certificate with a relative name actually works. So what you are testing is your abstraction, but not the code which pulls the information from the OpenSSL backend.

reaperhulk · 2015-05-11T21:43:01Z

I don't think I follow. The test (test_relativename_and_crl_issuer) loads a DER certificate with an encoded relativename, parses it, and then does an equality check. This should exercise the backend unless I'm completely misunderstanding.

etrauschke · 2015-05-11T21:45:00Z

Sorry, I'm an idiot. You're right, never mind.

alex · 2015-05-13T03:12:41Z

src/cryptography/hazmat/backends/openssl/x509.py

The conversion to _asn1_string_to_utf8 was lost.

alex · 2015-05-13T03:22:47Z

src/cryptography/hazmat/backends/openssl/x509.py

Hmm, should this use CRL_REASON_UNSPECIFIED and friends from OpenSSL?

We could; they're defined in the RFC though so the OpenSSL defines aren't as important in my opinion. If we did choose to do it we'd probably want to make a list and then iterate over that?

A series of if statements seems ok to me. I think I'd prefer if we used
OpenSSL's constants.

On Tue, May 12, 2015 at 11:38 PM, Paul Kehrer notifications@github.com
wrote:

In src/cryptography/hazmat/backends/openssl/x509.py
#1920 (comment):

reasons = []

# We will check each bit from RFC 5280

# ReasonFlags ::= BIT STRING {

# unused (0),

# keyCompromise (1),

# cACompromise (2),

# affiliationChanged (3),

# superseded (4),

# cessationOfOperation (5),

# certificateHold (6),

# privilegeWithdrawn (7),

# aACompromise (8) }

for bit in range(1, 9):

if self._backend._lib.ASN1_BIT_STRING_get_bit(

cdp.reasons, bit

):

We could; they're defined in the RFC though so the OpenSSL defines aren't
as important in my opinion. If we did choose to do it we'd probably want to
make a list and then iterate over that?

—
Reply to this email directly or view it on GitHub
https://github.com/pyca/cryptography/pull/1920/files#r30200533.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

If we want a series of if statements I'll have to put a new vector in that sets every bit and also one that tests the negative cases not covered by the existing vectors. Obviously doable, but required if we want to maintain coverage.

Ugh, right. Let me think.

On Tue, May 12, 2015 at 11:49 PM, Paul Kehrer notifications@github.com
wrote:

In src/cryptography/hazmat/backends/openssl/x509.py
#1920 (comment):

reasons = []

# We will check each bit from RFC 5280

# ReasonFlags ::= BIT STRING {

# unused (0),

# keyCompromise (1),

# cACompromise (2),

# affiliationChanged (3),

# superseded (4),

# cessationOfOperation (5),

# certificateHold (6),

# privilegeWithdrawn (7),

# aACompromise (8) }

for bit in range(1, 9):

if self._backend._lib.ASN1_BIT_STRING_get_bit(

cdp.reasons, bit

):

If we want a series of if statements I'll have to put a new vector in that
sets every bit and also one that tests the negative cases not covered by
the existing vectors. Obviously doable, but required if we want to maintain
coverage.

—
Reply to this email directly or view it on GitHub
https://github.com/pyca/cryptography/pull/1920/files#r30200778.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

It appears the OpenSSL constants map to the CRLReason enumeration in RFC 5280 and not the ReasonFlags (which are close, but not identical). So we can't actually use the OpenSSL constants here. For reference:

ReasonFlags ::= BIT STRING { unused (0), keyCompromise (1), cACompromise (2), affiliationChanged (3), superseded (4), cessationOfOperation (5), certificateHold (6), privilegeWithdrawn (7), aACompromise (8) }

CRLReason ::= ENUMERATED { unspecified (0), keyCompromise (1), cACompromise (2), affiliationChanged (3), superseded (4), cessationOfOperation (5), certificateHold (6), removeFromCRL (8), privilegeWithdrawn (9), aACompromise (10) }

OpenSSL's defines:

# define CRL_REASON_NONE -1 # define CRL_REASON_UNSPECIFIED 0 # define CRL_REASON_KEY_COMPROMISE 1 # define CRL_REASON_CA_COMPROMISE 2 # define CRL_REASON_AFFILIATION_CHANGED 3 # define CRL_REASON_SUPERSEDED 4 # define CRL_REASON_CESSATION_OF_OPERATION 5 # define CRL_REASON_CERTIFICATE_HOLD 6 # define CRL_REASON_REMOVE_FROM_CRL 8 # define CRL_REASON_PRIVILEGE_WITHDRAWN 9 # define CRL_REASON_AA_COMPROMISE 10

coveralls · 2015-05-13T04:37:54Z

Coverage remained the same at 100.0% when pulling 37ee3f7 on reaperhulk:x509-ossl-cdp into 0facc2a on pyca:master.

coveralls · 2015-05-13T04:37:55Z

Coverage remained the same at 100.0% when pulling 37ee3f7 on reaperhulk:x509-ossl-cdp into 0facc2a on pyca:master.

support CRLDistributionPoints in the OpenSSL backend

reaperhulk added the x509 label May 10, 2015

reaperhulk added this to the Ninth Release milestone May 10, 2015

reaperhulk force-pushed the x509-ossl-cdp branch from 6c76f2b to cf20858 Compare May 10, 2015 23:55

reaperhulk force-pushed the x509-ossl-cdp branch from cf20858 to 8b8ffd8 Compare May 13, 2015 01:58

alex reviewed May 13, 2015
View reviewed changes

reaperhulk force-pushed the x509-ossl-cdp branch from 8b8ffd8 to 37ee3f7 Compare May 13, 2015 03:14

alex reviewed May 13, 2015
View reviewed changes

reaperhulk mentioned this pull request May 13, 2015

more CDP vectors! #1941

Merged

reaperhulk added 2 commits May 13, 2015 08:55

support CRLDistributionPoints in the OpenSSL backend

9a10d59

change approach for parsing CDP reason flags

594a2ed

reaperhulk force-pushed the x509-ossl-cdp branch from 37ee3f7 to 594a2ed Compare May 13, 2015 13:55

reaperhulk mentioned this pull request May 13, 2015

add changelog entry for supporting x509 extensions #1946

Merged

alex added a commit that referenced this pull request May 13, 2015

Merge pull request #1920 from reaperhulk/x509-ossl-cdp

92fb537

support CRLDistributionPoints in the OpenSSL backend

alex merged commit 92fb537 into pyca:master May 13, 2015

reaperhulk deleted the x509-ossl-cdp branch May 13, 2015 22:57

github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2020

support CRLDistributionPoints in the OpenSSL backend #1920

support CRLDistributionPoints in the OpenSSL backend #1920

Uh oh!

Conversation

reaperhulk commented May 10, 2015

Uh oh!

reaperhulk commented May 10, 2015

Uh oh!

alex commented May 10, 2015

Uh oh!

reaperhulk commented May 10, 2015

Uh oh!

alex commented May 10, 2015

Uh oh!

reaperhulk commented May 10, 2015

Uh oh!

coveralls commented May 10, 2015

Uh oh!

coveralls commented May 10, 2015

Uh oh!

coveralls commented May 10, 2015

Uh oh!

alex commented May 10, 2015

Uh oh!

reaperhulk commented May 11, 2015

Uh oh!

etrauschke commented May 11, 2015

Uh oh!

reaperhulk commented May 11, 2015

Uh oh!

etrauschke commented May 11, 2015

Uh oh!

reaperhulk commented May 11, 2015

Uh oh!

etrauschke commented May 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 13, 2015

Uh oh!

coveralls commented May 13, 2015

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants