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

Post handshake auth #551

Merged
merged 11 commits into from
Jan 8, 2020
Merged

Post handshake auth #551

merged 11 commits into from
Jan 8, 2020

Conversation

tomato42
Copy link
Member

@tomato42 tomato42 commented May 21, 2019

Description

Implement support and add simple test case for post_handshake_auth extension

Motivation and Context

depends on: tlsfuzzer/tlslite-ng#350, #543, tlsfuzzer/tlslite-ng#379, tlsfuzzer/tlslite-ng#380 and #501
fixes #296

filed #622 to handle the one remaining issue from codeclimate

Checklist

  • I have read the CONTRIBUTING.md document and my PR follows change requirements therein
  • the changes are also reflected in documentation and code comments
  • all new and existing tests pass (see Travis CI results)
  • test script checklist was followed for new scripts
  • new test script added to tlslite-ng.json and tlslite-ng-random-subset.json
  • new and modified scripts were ran against popular TLS implementations:
  • required version of tlslite-ng updated in requirements.txt and README.md
  • test case includes interaction with KeyUpdate
  • test case includes interaction with NewSessionTicket (the test should verify that server sends at least one ticket)
  • support for servers that reject PHA with no client certificate with an alert

This change is Reviewable

@tomato42 tomato42 added enhancement new feature to be implemented new test script will require creation of a new connection script labels May 21, 2019
@tomato42
Copy link
Member Author

This pull request introduces 1 alert when merging ec6d045 into 79936b8 - view on LGTM.com

new alerts:

  • 1 for Too few arguments in formatting call

Comment posted by LGTM.com

@tomato42
Copy link
Member Author

This pull request introduces 1 alert when merging 97c092c into 79936b8 - view on LGTM.com

new alerts:

  • 1 for Too few arguments in formatting call

Comment posted by LGTM.com

@tomato42
Copy link
Member Author

This pull request introduces 1 alert when merging c3924a1 into 79936b8 - view on LGTM.com

new alerts:

  • 1 for Too few arguments in formatting call

Comment posted by LGTM.com

@tomato42 tomato42 added the blocked waiting for some other issue to be resolved label Nov 11, 2019
@tomato42 tomato42 self-assigned this Nov 25, 2019
@tomato42 tomato42 removed the blocked waiting for some other issue to be resolved label Nov 25, 2019
@tomato42 tomato42 force-pushed the post-handshake-auth branch 3 times, most recently from 3925cf6 to 6a76be8 Compare December 2, 2019 20:26
@tomato42 tomato42 force-pushed the post-handshake-auth branch 2 times, most recently from f930435 to ba6c85a Compare December 4, 2019 19:18
@tomato42 tomato42 changed the title [WIP] Post handshake auth Post handshake auth Jan 2, 2020
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.

Could you document somewhere the options I should use to be able to test openssl server PHA using the added expect script?

Reviewed 5 of 8 files at r1, 4 of 4 files at r4, 3 of 3 files at r5, 1 of 1 files at r6, 3 of 3 files at r7, 3 of 3 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 2 of 2 files at r11, 1 of 1 files at r12, 1 of 1 files at r13, 3 of 3 files at r14.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @codeclimate[bot])

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.

I didn't add it as it doesn't require any special options, just the key and certificates are needed for openssl

Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @ansasaki and @codeclimate[bot])

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.

Ok, but what options should I use to run the test-tls13-post-handshake-auth.py script? I'm asking because I couldn't make the tests to pass against openssl. Using --pha-as-reply I get 4 tests passing and 2 failing. Is this expected?

Reviewed 1 of 1 files at r15.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @codeclimate[bot])

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.

did you specify also -k and -c options? --pha-as-reply should be the only thing needed:

$ PYTHONPATH=. python scripts/test-tls13-post-handshake-auth.py -k /tmp/client/key.pem -c /tmp/client/cert.pem  --pha-as-reply
sanity ...
OK

post-handshake authentication with no client cert ...
OK

post-handshake authentication with KeyUpdate ...
OK

malformed signature in PHA ...
OK

post-handshake authentication ...
OK

sanity ...
OK

Basic post-handshake authentication test case
Check if server will accept PHA, check if server rejects invalid
signatures on PHA CertificateVerify, etc.
version: 1

Test end
successful: 6
failed: 0

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @codeclimate[bot])

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.

Ok, after using a client certificate trusted by the server, it worked fine.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @codeclimate[bot])

@tomato42 tomato42 merged commit 1448f0d into master Jan 8, 2020
@tomato42 tomato42 deleted the post-handshake-auth branch January 8, 2020 19:12
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 new test script will require creation of a new connection script
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TLS 1.3 post handshake client authentication
2 participants