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

[TLS 1.3] Client Authentication in the main Handshake #2957

Merged
merged 6 commits into from
Oct 12, 2022
Merged

Conversation

hrantzsch
Copy link
Collaborator

@hrantzsch hrantzsch commented Apr 13, 2022

Pull Request Dependencies

Before merging this, we should first review merge:

Description

This implements:

  • client authentication during the main handshake
  • coalescing of multiple handshake messages (which is required for the RFC 8448 client auth test) into a single (encrypted) record

Post-handshake authentication is left for future work. Rationale: A server may always ask for client authentication during the handshake. Post-handshake auth can be disabled by the client, by not negotiating the "post_handshake_auth" extension in the Client Hello.

TODO

  • We should explicitly support the "signature_algorithm_cert" extension
    The client can choose to not use this extension. Hence its support becomes crucial when implementing the server only.

@lgtm-com
Copy link

lgtm-com bot commented Apr 19, 2022

This pull request introduces 1 alert when merging 348a5bb into 8976bf2 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@reneme reneme marked this pull request as ready for review April 19, 2022 12:01
@reneme reneme changed the title [TLS 1.3] Client Auth [TLS 1.3] Client Authentication in the main Handshake Apr 19, 2022
@lgtm-com
Copy link

lgtm-com bot commented Apr 19, 2022

This pull request introduces 1 alert when merging c22912f into 8976bf2 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@reneme reneme force-pushed the dev/tls-13 branch 4 times, most recently from 00b35a6 to 2e01455 Compare April 27, 2022 11:21
@reneme reneme force-pushed the tls13/client_auth branch 2 times, most recently from f98334d to 3abfc13 Compare April 27, 2022 12:16
@lgtm-com
Copy link

lgtm-com bot commented Apr 27, 2022

This pull request introduces 1 alert when merging 3abfc13 into 45b74cc - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@lgtm-com
Copy link

lgtm-com bot commented May 18, 2022

This pull request introduces 1 alert when merging 9549e06 into bae64de - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@lgtm-com
Copy link

lgtm-com bot commented May 27, 2022

This pull request introduces 1 alert when merging 3ed31d4 into bae64de - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@lgtm-com
Copy link

lgtm-com bot commented May 27, 2022

This pull request introduces 1 alert when merging 86bacc1 into bae64de - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@reneme reneme changed the base branch from dev/tls-13 to master July 5, 2022 08:09
@reneme
Copy link
Collaborator

reneme commented Jul 5, 2022

Rebased and retargeted to master.

reneme and others added 3 commits July 6, 2022 14:32
Co-Authored-By: Hannes Rantzsch <hannes.rantzsch@nexenio.com>
After handshake messages are created they are dropped into this method for
book keeping and compile-time message type checking. Afterwards messages are
further processed and eventually sent. Hence, the old method name was fairly
misleading.

Co-Authored-By: Hannes Rantzsch <hannes.rantzsch@nexenio.com>
This allows for client authentication using certificates in the main handshake.
Post-handshake authentication is out of scope of this commit

Co-authored-by: René Meusel <rene.meusel@nexenio.com>
@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2022

Codecov Report

Base: 92.57% // Head: 92.55% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (ffe1c30) compared to base (987c7af).
Patch coverage: 92.68% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2957      +/-   ##
==========================================
- Coverage   92.57%   92.55%   -0.02%     
==========================================
  Files         596      600       +4     
  Lines       69729    70073     +344     
  Branches     6613     6625      +12     
==========================================
+ Hits        64552    64858     +306     
- Misses       5144     5182      +38     
  Partials       33       33              
Impacted Files Coverage Δ
src/lib/tls/tls_extensions.cpp 91.40% <75.00%> (-0.68%) ⬇️
src/lib/tls/tls13/msg_certificate_req_13.cpp 80.00% <80.00%> (ø)
src/tests/test_tls_rfc8448.cpp 91.26% <92.20%> (+0.97%) ⬆️
src/lib/tls/msg_cert_verify.cpp 94.25% <93.54%> (-1.21%) ⬇️
src/lib/tls/tls13/tls_client_impl_13.cpp 92.50% <97.56%> (+1.14%) ⬆️
src/bogo_shim/bogo_shim.cpp 88.85% <100.00%> (+0.03%) ⬆️
src/lib/tls/msg_cert_req.cpp 89.85% <100.00%> (ø)
src/lib/tls/tls12/tls_client_impl_12.cpp 91.54% <100.00%> (ø)
src/lib/tls/tls12/tls_handshake_state.cpp 86.13% <100.00%> (ø)
src/lib/tls/tls12/tls_server_impl_12.cpp 88.41% <100.00%> (ø)
... and 44 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@reneme reneme added this to the Botan 3.0.0 milestone Sep 20, 2022
src/lib/tls/msg_cert_verify.cpp Outdated Show resolved Hide resolved
src/lib/tls/msg_cert_verify.cpp Outdated Show resolved Hide resolved
src/lib/tls/tls13/msg_certificate_req_13.cpp Outdated Show resolved Hide resolved
src/lib/tls/tls13/tls_channel_impl_13.h Outdated Show resolved Hide resolved
@reneme reneme merged commit 6b36f30 into master Oct 12, 2022
@randombit randombit deleted the tls13/client_auth branch December 1, 2022 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants