Skip to content

[TLS 1.3] OCSP Stapling#3112

Merged
reneme merged 3 commits intomasterfrom
tls13/ocsp_stapling
Dec 30, 2022
Merged

[TLS 1.3] OCSP Stapling#3112
reneme merged 3 commits intomasterfrom
tls13/ocsp_stapling

Conversation

@reneme
Copy link
Copy Markdown
Collaborator

@reneme reneme commented Dec 9, 2022

Pull Request Dependencies

Description

This introduces active support for OCSP stapling for certificate-based authentication of both TLS 1.3 servers and clients.

I refactored the existing Certificate_Status_Request extension that was already used in TLS 1.2 but changed its semantics in TLS 1.3. Relevant RFC sections are referenced in the code. TL;DR: In TLS 1.3 Certificate_Status_Request is used as an extension to individual CertificateEntry structures in the Certificate handshake message. Though, the extension's body is equal to the Certificate_Status handshake message used in TLS 1.2 🤯

New Callback: TLS::Callbacks::tls_provide_cert_chain_status()

... that allows applications to provide stapled OCSP responses for all (or some) certificates added to the Certificate handshake message. To the best of my understanding, previous versions of TLS allowed stapled OCSP responses for the leaf certificate, only.

By default, this callback invokes the old tls_provide_cert_status() callback: mimicking the behavior of TLS 1.2 and stapling an OCSP response for the leaf certificate, only.

BoGo

Note that this enables several BoGo tests that are not necessarily related to OCSP stapling. In fact, the relevant tests for OCSP stapling cannot be enabled yet: They all depend on session resumption that is future work. However, the initial handshakes (with OCSP stapling) were successful, the tests fail due to the inability to resume the session later on.

@reneme reneme force-pushed the tls13/ocsp_stapling branch from 00cb110 to 9988a27 Compare December 9, 2022 13:55
@reneme reneme force-pushed the tls13/ocsp_stapling branch from 9988a27 to 40a663d Compare December 9, 2022 14:23
@reneme reneme mentioned this pull request Dec 9, 2022
7 tasks
@reneme reneme added this to the Botan 3.0.0 milestone Dec 9, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 9, 2022

Codecov Report

Base: 88.00% // Head: 87.95% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (26c074c) compared to base (7793d70).
Patch coverage: 85.16% of modified lines in pull request are covered.

❗ Current head 26c074c differs from pull request most recent head 3ad1a6b. Consider uploading reports for the commit 3ad1a6b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3112      +/-   ##
==========================================
- Coverage   88.00%   87.95%   -0.05%     
==========================================
  Files         600      600              
  Lines       67055    66871     -184     
  Branches     6696     6687       -9     
==========================================
- Hits        59011    58819     -192     
- Misses       5219     5227       +8     
  Partials     2825     2825              
Impacted Files Coverage Δ
src/bogo_shim/bogo_shim.cpp 72.99% <0.00%> (-0.06%) ⬇️
src/lib/tls/tls_client.cpp 83.92% <ø> (ø)
src/lib/tls/tls_session.cpp 85.71% <0.00%> (ø)
src/cli/tls_client.cpp 60.24% <42.85%> (ø)
src/lib/tls/tls_extensions.cpp 82.01% <50.00%> (ø)
src/lib/tls/msg_cert_verify.cpp 90.36% <60.00%> (ø)
src/lib/tls/msg_client_hello.cpp 81.21% <65.00%> (+2.68%) ⬆️
src/lib/tls/tls13/msg_encrypted_extensions.cpp 76.92% <66.66%> (ø)
src/lib/tls/tls13/msg_certificate_req_13.cpp 73.17% <73.68%> (ø)
src/lib/tls/tls_extensions_cert_status_req.cpp 79.24% <78.84%> (-4.09%) ⬇️
... and 75 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 force-pushed the tls13/ocsp_stapling branch from 40a663d to 26c074c Compare December 15, 2022 10:07
Copy link
Copy Markdown
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

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

I just reviewed the final commits

buf.push_back(0);
buf.push_back(0);
buf.push_back(0);
TODO: look into this more deeply
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Outstanding issue here? I don't understand what this TODO refers to

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I was wondering about the hard-coded serialization and wanted to find out what is going on here. For reference, this is now:

         // Serialization is hard-coded as we don't support advanced features
         // of this extension anyway.
         return
            {
            1,    // status_type = ocsp
            0, 0, // empty responder_id_list
            0, 0, // no extensions
            };

@reneme reneme mentioned this pull request Dec 16, 2022
14 tasks
@reneme reneme force-pushed the tls13/ocsp_stapling branch 2 times, most recently from a3012ae to 3ad1a6b Compare December 30, 2022 11:41
This extension got redefined in TLS 1.3 and is now attached
to individual CertificateEntries in the Certificate handshake
message. This prepares the Certificate_Status_Request class
using a pimpl construction.
@reneme reneme force-pushed the tls13/ocsp_stapling branch from 3ad1a6b to 706670f Compare December 30, 2022 12:22
@reneme reneme merged commit b18049f into master Dec 30, 2022
@randombit randombit deleted the tls13/ocsp_stapling branch December 30, 2022 20:10
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.

3 participants