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

ECDSA support #196

Merged
merged 11 commits into from
Nov 13, 2019
Merged

ECDSA support #196

merged 11 commits into from
Nov 13, 2019

Conversation

tomato42
Copy link
Member

@tomato42 tomato42 commented Oct 23, 2017

  • parsing ECDSA certificates
  • verifying ECDSA signatures on Server Key Exchange
  • negotiating ECDSA ciphers as a client
  • loading private keys
  • signing Server Key Exchange messages with ECDSA
  • negotiating ECDSA ciphers as a server
  • handling ECDSA in Certificate Verify (as client and server)
  • handling ECDSA in TLS 1.1 and earlier (signatures use SHA1, not SHA1+MD5)
  • using m2crypto for ECDSA (can be moved to a separate issue, should autodetect if the version of m2crypto/openssl does support ECC)
  • verify that sha1+ecdsa is selected if client didn't advertise signature algorithms but did advertise ecdsa ciphersuites in TLSv1.2
  • verify that sha1+ecdsa and sha224+ecdsa is ignored for signature algorithms in TLS 1.3 (it's disallowed there)
  • select signature algorithm based on public key type in TLS 1.3 (sha256 can be used with P-256 only, etc.)
  • support for starting server with multiple certificates/keys and selecting good one based on client preference, supported cipher suites or signature algorithms
  • verify that server advertised support for ecdsa_sign in CertificateRequest (or rsa_sign if we have a RSA certificate)
  • test coverage for all of the above

fixes #52

merged as #359, #360, #361, #363


This change is Reviewable

@tomato42 tomato42 added the enhancement new feature to be implemented label Oct 23, 2017
@tomato42 tomato42 force-pushed the ecdsa-support branch 2 times, most recently from 6f2a74f to c47c9df Compare October 25, 2017 15:07
@tomato42 tomato42 changed the title ECDSA support WIP ECDSA support Oct 25, 2017
@tomato42 tomato42 changed the title WIP ECDSA support [WIP] ECDSA support Nov 1, 2017
@tomato42

This comment has been minimized.

@tomato42 tomato42 added the complex Issues that require good knowledge of tlslite-ng internals or cryptography label Apr 12, 2018
@tomato42

This comment has been minimized.

@tomato42

This comment has been minimized.

@tomato42 tomato42 changed the title [WIP] ECDSA support ECDSA support Apr 1, 2019
@tomato42

This comment has been minimized.

@tomato42

This comment has been minimized.

@tomato42

This comment has been minimized.

@tomato42

This comment has been minimized.

@tomato42

This comment has been minimized.

@tomato42 tomato42 changed the title ECDSA support [WIP] ECDSA support Apr 5, 2019
@tomato42

This comment has been minimized.

@tomato42 tomato42 changed the title [WIP] ECDSA support ECDSA support Nov 5, 2019
@tomato42 tomato42 self-assigned this Nov 5, 2019
@lgtm-com

This comment has been minimized.

@tomato42
Copy link
Member Author

tomato42 commented Nov 6, 2019

This is the basic code to make it usable, but it's not really feature complete: certificate selection won't work in TLS 1.1 or earlier. It will give certificate with curve not supported by client. Those issues are tracked in #366 – fixing it would require significant reworking of negotiation and I'd rather close other outstanding PRs than work on polishing this stuff. Fixing #366 should make the code in tlsconnection.py much nicer.

While I did add configuration for virtual hosts, I didn't implement it, just provided a draft for the API. Continuation of that work is in #368.

it is also pure-python, while we could use m2crypto to speed it up, this was moved to #367

@ueno
Copy link
Collaborator

ueno commented Nov 13, 2019


tlslite/tlsconnection.py, line 3118 at r6 (raw file):

        sni_ext = clientHello.getExtension(ExtensionType.server_name)
        if sni_ext:
            name = sni_ext.hostNames[0].decode('ascii', 'strict')

would it make sense to catch the exception?

@ueno
Copy link
Collaborator

ueno commented Nov 13, 2019


tlslite/x509.py, line 55 at r9 (raw file):

        return self.bytes == other.bytes

    def __ne__(self, other):

Is this needed? The documentation says that the default implementation diverts to __eq__.

Copy link
Collaborator

@ueno ueno left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 3 of 3 files at r9, 2 of 2 files at r10, 1 of 1 files at r11.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tomato42)

Copy link
Member Author

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ueno)


tlslite/tlsconnection.py, line 3118 at r6 (raw file):

Previously, ueno (Daiki Ueno) wrote…

would it make sense to catch the exception?

no, it was already validated as decodable before


tlslite/x509.py, line 55 at r9 (raw file):

Previously, ueno (Daiki Ueno) wrote…

Is this needed? The documentation says that the default implementation diverts to __eq__.

LGTM complained until I added them, maybe it's needed for earlier versions?

@tomato42 tomato42 merged commit a15122a into master Nov 13, 2019
@tomato42 tomato42 deleted the ecdsa-support branch November 13, 2019 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complex Issues that require good knowledge of tlslite-ng internals or cryptography enhancement new feature to be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ECDSA support
2 participants