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

Add support for session resumption when using client certificates #447

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eh-steve
Copy link

Description

Previously, if a client submitted a certificate, any session resumption would be ignored in order to mitigate indefinite extension attacks as per https://curl.se/docs/CVE-2016-5419.html, but this prevented improving performance under mutual authentication use cases.

Now if RequireAndVerifyClientCert/VerifyClientCertIfGiven are used, the certificate expiry time is recorded in the session struct, allowing the user provided session store to decide what to do with "expired" sessions.

Revocations are still not handled via this mechanism, and the old behaviour can be preserved using the new config PeerCertDisablesSessionResumption

@daenney
Copy link
Member

daenney commented Apr 14, 2022

This is going to need some tests.

@eh-steve eh-steve force-pushed the session-store-support-cert-verification-expiry branch 2 times, most recently from 2bbb298 to 2561eb0 Compare April 14, 2022 20:28
@codecov
Copy link

codecov bot commented Apr 16, 2022

Codecov Report

Merging #447 (5ce50c0) into master (2a699e1) will increase coverage by 0.05%.
The diff coverage is 88.37%.

@@            Coverage Diff             @@
##           master     #447      +/-   ##
==========================================
+ Coverage   75.81%   75.87%   +0.05%     
==========================================
  Files          96       96              
  Lines        5586     5599      +13     
==========================================
+ Hits         4235     4248      +13     
  Misses       1019     1019              
  Partials      332      332              
Flag Coverage Δ
go 75.89% <88.37%> (+0.05%) ⬆️
wasm 66.22% <87.50%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
config.go 100.00% <ø> (ø)
handshaker.go 75.95% <ø> (ø)
state.go 88.61% <ø> (ø)
flight4handler.go 77.60% <57.14%> (-0.68%) ⬇️
crypto.go 51.26% <75.00%> (+0.62%) ⬆️
conn.go 80.86% <100.00%> (+0.02%) ⬆️
flight5handler.go 78.24% <100.00%> (+0.23%) ⬆️
pkg/crypto/selfsign/selfsign.go 78.46% <100.00%> (+1.04%) ⬆️
internal/net/dpipe/dpipe.go 94.44% <0.00%> (-2.23%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a699e1...5ce50c0. Read the comment docs.

@eh-steve
Copy link
Author

eh-steve commented Apr 16, 2022

I can’t really tell from the logs whether the CI is failing due to my changes… if it’s just the commit message linting, I was going to squash everything

@daenney
Copy link
Member

daenney commented Apr 17, 2022

Yeah, it's just the commit message. All the other checks passed.

Allow mutual authentication (RequireAndVerifyClientCert/
VerifyClientCertIfGiven)to work with session resumption by
recording the certificate expiry time in the session struct
@eh-steve eh-steve force-pushed the session-store-support-cert-verification-expiry branch from 2561eb0 to 61c44b8 Compare April 19, 2022 08:34
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.

None yet

2 participants