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.

Sign up for GitHub

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

Jump to bottom

Enable server-side OCSP stapling on Linux #69833

Merged
merged 7 commits into from
Jun 2, 2022
Merged

Enable server-side OCSP stapling on Linux #69833

merged 7 commits into from
Jun 2, 2022

Conversation

bartonjs
Copy link
Member

@bartonjs bartonjs commented May 25, 2022 •
edited
Loading

A few code reorganization notes:

  • The certificate asset download code from System.Security.Cryptography has been moved to a partial class in src/libraries/Common/src/System/Net/Http/X509ResourceClient.cs to be shared with System.Net.Security
    • It's partial so that CertificateAssetDownloader's tracing can be plugged in via partial methods.
  • OpenSSL Interop.OCSP.cs got split into Interop.OCSP.cs (only using SafeOcspResponseHandle) and Interop.OCSP.Chain.cs (also using SafeX509StoreCtxHandle)

With this change, SslStreamCertificateContext.Create on Linux will do a background
fetch of an OCSP response to staple to the Server Certificate in the Server Hello
portion of the handshake. This fetch is skipped if the context is built with offline:true.

Contexts that live for a long time and see regular activity should always have a response
ready to deliver to clients that ask for one. Particularly idle contexts that are retrieving
OCSP responses that have particularly short lifetimes may stop stapling because they may end up
not getting the activity they need to initiate a new request before the current response expires.

Contributes to #33377.

@bartonjs bartonjs added area-System.Net.Security os-linux Linux OS (any supported distro) labels May 25, 2022
@bartonjs bartonjs added this to the 7.0.0 milestone May 25, 2022
@bartonjs bartonjs requested a review from wfurt May 25, 2022 23:52
@ghost ghost assigned bartonjs May 25, 2022
@ghost
Copy link

ghost commented May 25, 2022

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

Issue Details

A few code reorganization notes:

  • The certificate asset download code from System.Security.Cryptography has been moved to a partial class in src/libraries/Common/src/System/Net/Http/X509ResourceClient.cs to be shared with System.Net.Security
    • It's partial so that CertificateAssetDownloader's tracing can be plugged in via partial methods.
  • OpenSSL Interop.OCSP.cs got split into Interop.OCSP.cs (only using SafeOcspResponseHandle) and Interop.OCSP.Chain.cs (also using SafeX509StoreCtxHandle)

With this change, SslStreamCertificateContext.Create on Linux will do a background
fetch of an OCSP response to staple to the Server Certificate in the Server Hello
portion of the handshake. This fetch is skipped if the context is built with offline:true.

Contexts that live for a long time and see regular activity should always have a response
ready to deliver to clients that ask for one. Particularly idle contexts that are retrieving
OCSP responses that have particularly short lifetimes may stop stapling because they may end up
not getting the activity they need to initiate a new request before the current response expires.

Author: bartonjs
Assignees: -
Labels:

area-System.Net.Security, os-linux
Milestone: 7.0.0

wfurt
wfurt reviewed May 26, 2022
View reviewed changes
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.

generally looks good to me.
We seems to fail the build on some configurations.
Quic failures are known and unrelated.

wfurt
wfurt approved these changes May 27, 2022
View reviewed changes
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

@bartonjs
Copy link
Member Author

Looks like there's some build configuration where time_t is long instead of long long, and that's messing with the export as int64_t. Looking into it.

stephentoub
stephentoub reviewed May 31, 2022
View reviewed changes
src/libraries/Common/src/System/Net/Http/X509ResourceClient.cs
((IDisposable)responseMessage).Dispose();

redirections++;
if (redirections > MaxRedirections)
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember why we do all this custom redirect handling rather than using the built-in support. Do you?

Copy link
Member

@vcsjones vcsjones May 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Because we cannot permit HTTP -> HTTPS redirects. The scheme has to remain HTTP. #45010 has more background, and #48221 has the first fix for it.

Copy link
Member

@stephentoub stephentoub May 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We should really consider adding #45364, @karelz. This is an awful lot of custom logic required.

Copy link
Member

Choose a reason for hiding this comment

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

@stephentoub are you suggesting in .NET 7.0?
I think we might be able to make it happen. Let's bring it up in next meeting.

Copy link
Member

Choose a reason for hiding this comment

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

are you suggesting in .NET 7.0?

If we have time.

stephentoub
stephentoub reviewed May 31, 2022
View reviewed changes
src/libraries/Common/src/System/Text/Base64UrlEncoding.cs Outdated

namespace System.Text
{
internal static class Base64UrlEncoding
Copy link
Member

Choose a reason for hiding this comment

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

We've had multiple requests for this functionality, e.g. #1658. How close is this to what's needed for that? I'm wondering if we should use this as an opportunity to get it into the right shape and move that issue to api-ready-for-review so we can get it out in 7.

stephentoub
stephentoub reviewed May 31, 2022
View reviewed changes
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs

if (!_staplingForbidden)
{
// Create the task, let the download finish in the background.
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on why it's ok for this to be fire-and-forget?

Copy link
Member Author

Choose a reason for hiding this comment

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

Consistent with other platforms, the server side of OCSP stapling is bestish effortish. On Windows they equivalently fire off an async download, and whenever there's a new session and the download has completed they attach it... if it didn't complete (or it failed), then it isn't.

stephentoub
stephentoub reviewed May 31, 2022
View reviewed changes
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs
if (task.IsCompletedSuccessfully)
{
return task.Result;
}
Copy link
Member

Choose a reason for hiding this comment

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

Same here... why is it ok that this is fire-and-forget?

stephentoub
stephentoub reviewed May 31, 2022
View reviewed changes
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs
{
Task<byte[]?>? pending = _pendingDownload;

if (pending is not null && !pending.IsFaulted)
Copy link
Member

Choose a reason for hiding this comment

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

This never needs to transition from non-null back to null?

Copy link
Member Author

Choose a reason for hiding this comment

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

_pendingDownload goes back to null if it completes successfully. On... line.... that got deleted.

stephentoub
stephentoub reviewed May 31, 2022
View reviewed changes
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs
{
pending = _pendingDownload;

if (pending is null || pending.IsFaulted)
Copy link
Member

Choose a reason for hiding this comment

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

If it's faulted, will someone somewhere have accessed its Exception or otherwise observed what the error was?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. If it faulted (most likely due to a timeout or network error) we'll just let another one spin up.

@bartonjs bartonjs merged commit 6a8f16e into dotnet:main Jun 2, 2022
@jeffhandley jeffhandley mentioned this pull request Jun 3, 2022
Closed
@maraf maraf mentioned this pull request Jun 9, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 1, 2022
@bartonjs bartonjs deleted the linux_server_ocsp_stapling branch August 4, 2022 15:51
@bartonjs bartonjs added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Aug 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vcsjones vcsjones vcsjones left review comments

@stephentoub stephentoub stephentoub left review comments

@karelz karelz karelz left review comments

@wfurt wfurt wfurt approved these changes

Assignees

@bartonjs bartonjs

Labels
area-System.Net.Security needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration os-linux Linux OS (any supported distro)
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@bartonjs @vcsjones @stephentoub @karelz @wfurt