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

Recover from failed OCSP download. #96448

Merged
merged 2 commits into from
Jan 10, 2024
Merged

Recover from failed OCSP download. #96448

merged 2 commits into from
Jan 10, 2024

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Jan 3, 2024

Fixes #96770

When performing OCSP stapling by server and we get invalid response from the OCSP server (either because we don't get any data back or parsing the OCSP response fails), the server will never try to fetch OCSP again because the SslStreamCertificateContext._pendingDownload field still contains the previous task. As a consequence, the server will stop sending OCSP staples after the failure.

This PR moves the _pendingDownload = null assignment to a place where it is guaranteed to execute.

This has a potential pitfall when the given OCSP server is unhealthy (and consistently returning bad response), then we would potentially issue many requests to it. But I understand that if server does not send OCSP staple then clients may fetch the staple themselves, so I am not 100% sure if mitigation from our side changes much. @bartonjs, @vcsjones, what do you think? If we should mitigate it, what mechanism do you suggest? exponential back-off, or maybe even a simple rate-limiting of max 1 request per x minutes?

@ghost
Copy link

ghost commented Jan 3, 2024

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

When performing OCSP stapling by server and we get invalid response from the OCSP server (either because we don't get any data back or parsing the OCSP response fails), the server will never try to fetch OCSP again because the SslStreamCertificateContext._pendingDownload field still contains the previous task.

This PR moves the _pendingDownload = null assignment to a place where it is guaranteed to execute.

This has a potential pitfall when the given OCSP server is unhealthy (and consistently returning bad response), then we would potentially issue many requests to it. But I understand that if server does not send OCSP staple then clients may fetch the staple themselves, so I am not 100% sure if mitigation from our side changes much. @bartonjs, @vcsjones, what do you think? If we should mitigate it, what mechanism do you suggest? exponential back-off, or maybe even a simple rate-limiting of max 1 request per x minutes?

Author: rzikm
Assignees: -
Labels:

area-System.Net.Security

Milestone: -

@rzikm rzikm added this to the 9.0.0 milestone Jan 3, 2024
@danmoseley
Copy link
Member

Very naive question - do servers ever request these from other servers, and if so, so they use this code? Just curious whether this is a case where Kestrel may have its own policy.

@rzikm
Copy link
Member Author

rzikm commented Jan 4, 2024

Very naive question - do servers ever request these from other servers, and if so, so they use this code? Just curious whether this is a case where Kestrel may have its own policy.

if you mean if this is used in "server-to-server" scenarios, then yes. Technically, the initiator of the connection still acts the role of a client. and the receiver of the connection (the machine acting the server role) will run this code and send an OCSP staple if OCSP stapling was enabled (by SslStreamCertificateContext.Create with right parameters).

Clients are requesting OCSP staple from the server by adding a specific extension to the ClientHello when initiating the connection. Right now, there is no API to control this extension from .NET (not even sure if it is present on all underlying platforms).

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@wfurt
Copy link
Member

wfurt commented Jan 5, 2024

BTW do we have test gap? With 5s delay we should be able to craft a test, right?

@rzikm
Copy link
Member Author

rzikm commented Jan 10, 2024

The testing is not going to be straightforward because most of the code is private. I filed #96791 to track test coverage and will look into it some more once we fix the more urgent issues.

@rzikm rzikm merged commit a3775a4 into dotnet:main Jan 10, 2024
111 checks passed
rzikm added a commit to rzikm/dotnet-runtime that referenced this pull request Jan 11, 2024
* Recover from failed OCSP check.

* Add 5s back-off after failed OCSP querry
rzikm added a commit to rzikm/dotnet-runtime that referenced this pull request Jan 11, 2024
* Recover from failed OCSP check.

* Add 5s back-off after failed OCSP querry
wfurt pushed a commit that referenced this pull request Jan 16, 2024
* Add entire issuer chain to trusted X509_STORE when stapling OCSP_Response (#96792)

* Add entire issuer chain to trusted X509_STORE when validating OCSP_Response

* Code review feedback

* More code review feedback

* Update src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>

* Fix compilation

* Always include root certificate

---------

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>

* Recover from failed OCSP download. (#96448)

* Recover from failed OCSP check.

* Add 5s back-off after failed OCSP querry

* Don't shorten OCSP expriation on failed server OCSP fetch (#96972)

* Don't shorten OCSP expriation on failed server OCSP fetch

* Code review feedback

---------

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
rzikm added a commit that referenced this pull request Jan 16, 2024
* Recover from failed OCSP download. (#96448)

* Recover from failed OCSP check.

* Add 5s back-off after failed OCSP querry

* Do not OCSP staple invalid OCSP responses

* Add entire issuer chain to trusted X509_STORE when validating OCSP_Response

Code review feedback

More code review feedback

Update src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>

Fix compilation

Always include root certificate

* Fix compilation

* Don't shorten OCSP expriation on failed server OCSP fetch (#96972)

* Don't shorten OCSP expriation on failed server OCSP fetch

* Code review feedback

---------

Co-authored-by: Kevin Jones <kevin@vcsjones.com>
Co-authored-by: Carlos Sánchez López <1175054+carlossanlop@users.noreply.github.com>
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
* Recover from failed OCSP check.

* Add 5s back-off after failed OCSP querry
@github-actions github-actions bot locked and limited conversation to collaborators Feb 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SslStreamCertificateContext stops fetching OCSP staples after one request failure
5 participants