Skip to content

X509Backend support in OpenSSL backend#1499

Merged
alex merged 35 commits intopyca:masterfrom
reaperhulk:x509-ossl-impl
Dec 17, 2014
Merged

X509Backend support in OpenSSL backend#1499
alex merged 35 commits intopyca:masterfrom
reaperhulk:x509-ossl-impl

Conversation

@reaperhulk
Copy link
Member

This PR adds a dependency on enum34. I am expecting some improvements to be needed in the docs along with potentially some comments required to explain the purpose of some of the tests.

TODO:

  • Resolve issue with EC public keys where curve name is not set. (resolved by raising error for now)

Depends on #1498.

@jenkins-cryptography
Copy link

Test FAILed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2581/

docs/x509.rst Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably say something like: "X.509 certificate are commonly used in protocols like TLS."

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2585/

@alex
Copy link
Member

alex commented Nov 27, 2014

You should be able to rebase the vector loader changes out now

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2586/

@jenkins-cryptography
Copy link

Test FAILed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2587/

@alex
Copy link
Member

alex commented Nov 27, 2014

Test failures relevant

On Thu Nov 27 2014 at 2:06:09 PM jenkins-cryptography <
notifications@github.com> wrote:

Test FAILed.
Refer to this link for build results:
https://jenkins.cryptography.io/job/cryptography-pr-experimental/2587/


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

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2589/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a bytes_to_bio utility method in the backend, should we have a bio_to_bytes too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah probably should. These methods are also present in #1503 (I pulled them from this PR and hoisted them into the backend itself). If that PR lands first I'll rebase this one against it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently BIO_get_mem_data is only useful for memory BIOs so I don't want to generically call the _read_mem_bio method bio_to_bytes. I am going to move them to live next to each other in the backend though.

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2600/

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2601/

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 75895e6 on reaperhulk:x509-ossl-impl into 745e512 on pyca:master.

docs/index.rst Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe fernet first? :-/ sorry

@alex
Copy link
Member

alex commented Dec 15, 2014

Besides the comment abotu the error message for NotImplementedError, I think this is good to go. Since this is a sizable chunk, I'd like another set of eyes: @dreid @lvh @public?

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2655/

@alex
Copy link
Member

alex commented Dec 15, 2014

Oop, sorry, one more thing: I think InvalidVersion shoudl have an attribute which contains the bogus value.

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2656/

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2658/

docs/x509.rst Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it some more, I'd like if all of these attributes / methods showed an example of accessing them, using the cert we parse above.

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2663/

@reaperhulk
Copy link
Member Author

What is the deal with me and over-indenting lately.

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2664/

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2665/

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 7522c6d on reaperhulk:x509-ossl-impl into 9305287 on pyca:master.

docs/x509.rst Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you slap a :type: int on this, and "Returns the raw version"

@alex
Copy link
Member

alex commented Dec 17, 2014

Ok. One last comment and then I'll merge.

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: https://jenkins.cryptography.io/job/cryptography-pr-experimental/2674/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shit I just realized this, but should this checking for a thing with an unnamed curve be in _evp_pkey_to_public_key? It seems like it could occur with regular key loading as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a good idea. Right now in our OpenSSL EC key classes we call _ec_key_curve_sn which asserts that EC_GROUP_get_curve_name isn't undef. We could hoist this check into EC key stuff for now. I'd like to put that in as a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok!

On Wed Dec 17 2014 at 1:15:28 PM Paul Kehrer notifications@github.com
wrote:

In src/cryptography/hazmat/backends/openssl/x509.py
#1499 (diff):

  •        pkey.type == self._backend._lib.EVP_PKEY_EC
    
  •    ):
    
  •        ec_cdata = self._backend._lib.EVP_PKEY_get1_EC_KEY(pkey)
    
  •        assert ec_cdata != self._backend._ffi.NULL
    
  •        ec_cdata = self._backend._ffi.gc(
    
  •            ec_cdata, self._backend._lib.EC_KEY_free
    
  •        )
    
  •        group = self._backend._lib.EC_KEY_get0_group(ec_cdata)
    
  •        assert group != self._backend._ffi.NULL
    
  •        nid = self._backend._lib.EC_GROUP_get_curve_name(group)
    
  •        if nid == self._backend._lib.NID_undef:
    
  •            raise NotImplementedError(
    
  •                "ECDSA certificates with unnamed curves are unsupported "
    
  •                "at this time"
    
  •            )
    

Might be a good idea. Right now in our OpenSSL EC key classes we call
_ec_key_curve_sn which asserts that EC_GROUP_get_curve_name isn't undef.
We could hoist this check into EC key stuff for now. I'd like to put that
in as a separate PR.


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

alex added a commit that referenced this pull request Dec 17, 2014
X509Backend support in OpenSSL backend
@alex alex merged commit 4d8de13 into pyca:master Dec 17, 2014
@reaperhulk reaperhulk deleted the x509-ossl-impl branch December 17, 2014 22:22
@felixonmars
Copy link

Should the dependency be omitted on Python 3.4+, or does enum34 works differently than the builtin one in Python 3.4?

@alex
Copy link
Member

alex commented Dec 18, 2014

We could skip installing. Right now it's just installed and ignored.

On Wed Dec 17 2014 at 10:19:32 PM Felix Yan notifications@github.com
wrote:

Should the dependency be omitted on Python 3.4+, or does enum34 works
differently than the builtin one in Python 3.4?


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

@felixonmars
Copy link

Thanks, I'm going to sed the line out on Arch for now.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Development

Successfully merging this pull request may close these issues.

8 participants