Skip to content
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

[WIP]: Move to pure python code for parsing PEMs. #1390

Closed
wants to merge 51 commits into from

Conversation

alex
Copy link
Member

@alex alex commented Oct 8, 2014

OMFG. Not final.

Implements:

  • the core "PEM" parser logic
  • PEM decryption
  • Handling of the ASN.1 DER for ECDSA Private Keys

Not done:

  • Parsing of a bunch of other key types
  • non-md5 based PBES

Some notes:

  • I don't understand the error handling around ASN.1 well enough yet, which
    makes me pretty uncomfortable given the sensitivity of this stuff.
  • PyASN.1's API is kind of miserable, but it does appear to be complete
  • There's some not-great error handling in (at least) the EC number loading in
    the OpenSSL backend
  • Bits vs. Bytes for key_size is the worst :-(
  • If we're comitted to doing this, it's going to be a decent sized project.
    We'll have to keep the old key loading logic around and port one key type at
    a time.
  • I don't know if I read the RFC closely enough :-( There's all kinds of random
    notes in various places, and I assume if I ignore any one of those the
    security of the system is screwed.

FEEDBACK PLEASE!!! :-)

@jenkins-cryptography
Copy link

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

@jenkins-cryptography
Copy link

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

@jenkins-cryptography
Copy link

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

@jenkins-cryptography
Copy link

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

@jenkins-cryptography
Copy link

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

@jenkins-cryptography
Copy link

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

)


def bytes_to_int(b):
Copy link
Member

Choose a reason for hiding this comment

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

Is this some fancy ASN.1 bytes to int thing? It doesn't look particularly special and we already have a utility method that does bytes to int.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where do we have that method? We have some code in "bn_to_int", but it only
does py3k, py2k goes via hex.

On Sat, Oct 11, 2014 at 2:12 AM, Alex Stapleton notifications@github.com
wrote:

In cryptography/hazmat/primitives/serialization.py:

  •            namedValues=namedval.NamedValues(
    
  •                ("ecPrivkeyVer1", 1),
    
  •            )
    
  •        ),
    
  •    ),
    
  •    namedtype.NamedType("privateKey", univ.OctetString()),
    
  •    namedtype.OptionalNamedType("parameters", _ECParameters().subtype(
    
  •        implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 0),
    
  •    )),
    
  •    namedtype.OptionalNamedType("publicKey", univ.BitString().subtype(
    
  •        implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 1),
    
  •    )),
    
  • )

+def bytes_to_int(b):

Is this some fancy ASN.1 bytes to int thing? It doesn't look particularly
special and we already have a utility method that does bytes to int.


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

"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

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm misunderstanding the purpose of this method, can't we just use six.bytes2int()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think six.bytes2int exists, there's six.byte2int (singular!),
but that's basically just chr.

On Sat, Oct 11, 2014 at 8:20 AM, Terry Chia notifications@github.com
wrote:

In cryptography/hazmat/primitives/serialization.py:

  •            namedValues=namedval.NamedValues(
    
  •                ("ecPrivkeyVer1", 1),
    
  •            )
    
  •        ),
    
  •    ),
    
  •    namedtype.NamedType("privateKey", univ.OctetString()),
    
  •    namedtype.OptionalNamedType("parameters", _ECParameters().subtype(
    
  •        implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 0),
    
  •    )),
    
  •    namedtype.OptionalNamedType("publicKey", univ.BitString().subtype(
    
  •        implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 1),
    
  •    )),
    
  • )

+def bytes_to_int(b):

Unless I'm misunderstanding the purpose of this method, can't we just use
six.bytes2int()?


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

"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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, I remembered it wrong. :)

@alex
Copy link
Member Author

alex commented Oct 13, 2014

Let's talk about PKCS#12. It's a thing. That's unfortunate, but we kind of have to live with it. Specifically, it's the KDF used with PKCS#8 for some keys. I started to implement it. I'm pretty sure I'm Implementing Crypto (tm). Since we all said we weren't going to do that, I'm kind of inclined to stop.

How to do it then?

  • We could not support keys with these algorithms, I actually can't find support for the algorithm I'm currently looking at (OID 1.2.840.113549.1.12.1.3) in Botan or PyCrypto.
  • Or we could expose PKCS#12 as a KDF in our kdf module (maybe privately for now) and reuse the OpenSSL version?

What do folks think?

@jenkins-cryptography
Copy link

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

@jenkins-cryptography
Copy link

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

@ronf
Copy link

ronf commented Oct 14, 2014

There is a PKCS#12-specific KDF involved, but why would you need to implement that in Python? OpenSSL understands these encodings already, so shouldn't you be able to let it do the decryption?

Is the issue that you're trying to support non-ASCII pass-phrases and OpenSSL isn't giving you a way to pass in the already-encoded UTF-16 bytes to use as the pass-phrase, and so you can't pass the encrypted version of the key to OpenSSL?

FWIW, the pure-Python KDF for PKCS#12 I implemented was only about 30 lines of code, but you're right that it's technically "crypto" code.

Without this support, the following PKCS#8 ciphers would not be supported for PBE version 1:
SHA1-2DES, SHA1-RC2-40, SHA1-RC2-128, SHA1-RC4-40, and SHA1-RC4-128

@jenkins-cryptography
Copy link

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

@jenkins-cryptography
Copy link

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

@jenkins-cryptography
Copy link

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

@jenkins-cryptography
Copy link

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

@jenkins-cryptography
Copy link

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

Conflicts:
	setup.py
	src/cryptography/hazmat/primitives/serialization.py
@jenkins-cryptography
Copy link

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

@jenkins-cryptography
Copy link

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

@alex
Copy link
Member Author

alex commented Dec 25, 2014

retest this please

@jenkins-cryptography
Copy link

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

@alex
Copy link
Member Author

alex commented Dec 25, 2014

retest this please

@jenkins-cryptography
Copy link

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

@alex
Copy link
Member Author

alex commented Dec 25, 2014

retest this please

@jenkins-cryptography
Copy link

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

@alex
Copy link
Member Author

alex commented Dec 25, 2014

retest this please

@jenkins-cryptography
Copy link

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

Conflicts:
	src/cryptography/hazmat/primitives/serialization.py
@jenkins-cryptography
Copy link

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

@reaperhulk
Copy link
Member

I think we should close this and if we choose to revisit it in the future a new PR can be opened. Let me know if you disagree @alex :)

@reaperhulk reaperhulk closed this Sep 30, 2015
@alex
Copy link
Member Author

alex commented Sep 30, 2015

That's fine.

On Wed, Sep 30, 2015 at 12:23 AM, Paul Kehrer notifications@github.com
wrote:

I think we should close this and if we choose to revisit it in the future
a new PR can be opened. Let me know if you disagree @alex
https://github.com/alex :)


Reply to this email directly or view it on GitHub
#1390 (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

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

Successfully merging this pull request may close these issues.

None yet

6 participants