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

Simple ECDSA support #359

Merged
merged 7 commits into from
Sep 30, 2019
Merged

Simple ECDSA support #359

merged 7 commits into from
Sep 30, 2019

Conversation

tomato42
Copy link
Member

@tomato42 tomato42 commented Sep 23, 2019

Support for ECDSA authentication from server side

(first part of the full ECDSA support, #52, reworking of #196 to make review easier)


This change is Reviewable

@tomato42 tomato42 added the enhancement new feature to be implemented label Sep 23, 2019
@tomato42 tomato42 self-assigned this Sep 23, 2019
@lgtm-com
Copy link

lgtm-com bot commented Sep 23, 2019

This pull request introduces 5 alerts and fixes 1 when merging 2087e80 into 401712c - view on LGTM.com

new alerts:

  • 3 for Unused import
  • 1 for Unused local variable
  • 1 for 'import *' may pollute namespace

fixed alerts:

  • 1 for Membership test with a non-container

@tomato42 tomato42 force-pushed the simple-ecdsa-support branch 4 times, most recently from ebacb8e to 1fd2cc1 Compare September 23, 2019 19:29
@lgtm-com
Copy link

lgtm-com bot commented Sep 23, 2019

This pull request fixes 1 alert when merging 1fd2cc1 into 401712c - view on LGTM.com

fixed alerts:

  • 1 for Membership test with a non-container

@lgtm-com
Copy link

lgtm-com bot commented Sep 24, 2019

This pull request fixes 1 alert when merging 2b6c6da into 401712c - view on LGTM.com

fixed alerts:

  • 1 for Membership test with a non-container

@lgtm-com
Copy link

lgtm-com bot commented Sep 25, 2019

This pull request fixes 1 alert when merging 0199b2d into 401712c - view on LGTM.com

fixed alerts:

  • 1 for Membership test with a non-container

Copy link
Collaborator

@ansasaki ansasaki left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1, 5 of 5 files at r2, 4 of 4 files at r3, 1 of 1 files at r4, 4 of 4 files at r5, 3 of 3 files at r6, 9 of 9 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tomato42)


tlslite/utils/python_key.py, line 112 at r5 (raw file):

    def _parse_ecc_ssleay(data):
        """
        Parse binary structure of the old SSLeay file fromat used by OpenSSL.

s/fromat/format/

allow the parsePEM to parse both RSA and ECDSA key files
support for ECDSA authentication without client certificates
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: 13 of 26 files reviewed, 1 unresolved discussion (waiting on @ansasaki)


tlslite/utils/python_key.py, line 112 at r5 (raw file):

Previously, ansasaki (Anderson Sasaki) wrote…

s/fromat/format/

Done.

@lgtm-com
Copy link

lgtm-com bot commented Sep 30, 2019

This pull request fixes 1 alert when merging d44800d into 401712c - view on LGTM.com

fixed alerts:

  • 1 for Membership test with a non-container

Copy link
Collaborator

@ansasaki ansasaki left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 13 files at r8, 3 of 3 files at r9, 9 of 9 files at r10.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@ansasaki
Copy link
Collaborator

To be honest, in this review I couldn't check all the changes (for example the hardcoded keys, cipher suites allowed for each TLS version, etc.). I'm relying on the results of the added tests, which seems to cover the changes.

@tomato42
Copy link
Member Author

To be honest, in this review I couldn't check all the changes (for example the hardcoded keys, cipher suites allowed for each TLS version, etc.). I'm relying on the results of the added tests, which seems to cover the changes.

that's the idea: the tests should show that the code works :)

the proper test coverage will come with tlsfuzzer/tlsfuzzer#543 and related issues

thanks!

@tomato42 tomato42 merged commit 513e165 into master Sep 30, 2019
@tomato42 tomato42 deleted the simple-ecdsa-support branch September 30, 2019 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new feature to be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants