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

add CertificateChainPolicy to ssl options #71961

Merged
merged 4 commits into from
Jul 12, 2022
Merged

add CertificateChainPolicy to ssl options #71961

merged 4 commits into from
Jul 12, 2022

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jul 11, 2022

This is replacement for #69908
fixes #71191 #59979 #35839 #59944 #40423

This adds API approved in #71191 so callers of SslStream can specify custom X509ChainPolicy to specify custom trust, AIA download behavior or other options available in X509ChainPolicy.

Unlike original PoC there is no attempt to enforce consistency with other validation fragments. When custom policy is specified it will be used exclusively instead of fragments we currently use to construct it it internally.

TODO: This changes only SslStream. I will follow-up with PR for QUIC once the code there is stable. (#71783 done)

@wfurt wfurt added this to the 7.0.0 milestone Jul 11, 2022
@wfurt wfurt requested review from vcsjones, bartonjs and a team July 11, 2022 18:28
@wfurt wfurt self-assigned this Jul 11, 2022
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jul 11, 2022

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

Issue Details

This is replacement for #69908
fixes #71191 #59979 #35839 #59944 #40423

This adds API approved in #71191 so callers of SslStream can specify custom X509ChainPolicy to specify custom trust, AIA download behavior or other options available in X509ChainPolicy.

Unlike original PoC there is no attempt to enforce consistency with other validation fragments. When custom policy is specified it will be used exclusively instead of fragments we currently use to construct it it internally.

TODO: This changes only SslStream. I will follow-up with PR for QUIC once the code there is stable. (#71783 done)

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security
Milestone: 7.0.0

bartonjs
bartonjs reviewed Jul 11, 2022
View reviewed changes
src/libraries/System.Net.Security/src/System/Net/Security/SslClientAuthenticationOptions.cs Outdated
Comment on lines 80 to 81
/// </summary>
public X509ChainPolicy? ValidationPolicy { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Properties are supposed to have both a summary and a value tag.

https://github.com/dotnet/dotnet-api-docs/wiki/Property-Value

Suggested change
/// </summary>
public X509ChainPolicy? ValidationPolicy { get; set; }
/// </summary>
/// <value>
/// An optional customized policy for certificate chain validation.
/// The default is <see langword="null" />.
/// </value>
public X509ChainPolicy? ValidationPolicy { get; set; }

bartonjs
bartonjs reviewed Jul 11, 2022
View reviewed changes
src/libraries/System.Net.Security/src/System/Net/Security/SslServerAuthenticationOptions.cs Outdated
/// Specifies X509ChainPolicy to use for remote certificate
/// validation. If set, CertificateRevocationCheckMode and SslCertificateTrust is ignored.
/// </summary>
public X509ChainPolicy? ValidationPolicy { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

All of the feedback from the API docs for the client version also applies here.

bartonjs
bartonjs reviewed Jul 11, 2022
View reviewed changes
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs Outdated
Comment on lines 951 to 955
if (_sslAuthenticationOptions.ValidationPolicy.VerificationTimeIgnored)
{
chain.ChainPolicy.CustomTrustStore.AddRange(trust._store.Certificates);
// Update VerificationTime to 'Now' unless explicitly set for whatever reason.
chain.ChainPolicy.VerificationTime = DateTime.Now;
}
Copy link
Member

@bartonjs bartonjs Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This (VerificationTime acting as DateTime.Now when VerificationTimeIgnored is set) should be an implementation detail of X509Chain, not acted upon here.

Copy link
Member

Choose a reason for hiding this comment

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

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Chain.cs

Line 136 in c9feedf

chainPolicy.VerificationTime,

Change

-chainPolicy.VerificationTime,
+chainPolicy.VerificationTimeIgnored ? DateTime.Now : VerificationTime,

bartonjs
bartonjs reviewed Jul 11, 2022
View reviewed changes
...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509ChainPolicy.cs Outdated

public X509ChainPolicy Clone()
{
return (X509ChainPolicy)MemberwiseClone();
Copy link
Member

Choose a reason for hiding this comment

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

I want this written by hand.

Suggested change
return (X509ChainPolicy)MemberwiseClone();
X509ChainPolicy clone = new X509ChainPolicy
{
DisableCertificateDownloads = DisableCertificateDownloads,
_revocationMode = _revocationMode,
_revocationFlag = _revocationFlag,
_verificationFlags = _verificationFlags,
_trustMode = _trustMode,
_verificationTime = _verificationTime,
VerificationTimeIgnored = VerificationTimeIgnored,
UrlRetrievalTimeout = UrlRetrievalTimeout,
};
if (_applicationPolicy?.Count > 0)
{
clone.ApplicationPolicy.AddRange(_applicationPolicy);
}
if (_certificatePolicy?.Count > 0)
{
...
}
if (_customTrustStore?.Count > 0)
{
...
}
}

And then in tests, Assert.NotSame(orig.ApplicationPolicy, clone.ApplicationPolicy); and Assert.Equal(orig.ApplicationPolicy, clone.ApplicationPolicy); (and similar for the other collections)

wfurt and others added 3 commits July 12, 2022 20:34
@wfurt @bartonjs
Update src/libraries/System.Net.Security/src/System/Net/Security/SslC…
0b80589 
…lientAuthenticationOptions.cs

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
@wfurt @bartonjs
Apply suggestions from code review
0a7075b 
Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
@wfurt
feedback from review
88e609f
bartonjs
bartonjs reviewed Jul 12, 2022
View reviewed changes
...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509ChainPolicy.cs
Comment on lines +153 to +161
foreach (var item in _applicationPolicy)
{
clone.ApplicationPolicy.Add(item);
}
}

if (_certificatePolicy?.Count > 0)
{
foreach (var item in _certificatePolicy)
Copy link
Member

Choose a reason for hiding this comment

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

These shouldn't be var, and there should be some tests for clone, but those can get done as a followup if the tests are all green.

bartonjs
bartonjs approved these changes Jul 12, 2022
View reviewed changes
Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

Some minor issues, but nothing worth missing the branch over.

@bartonjs bartonjs added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jul 12, 2022
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 12, 2022
@ghost
Copy link

ghost commented Jul 12, 2022 •
edited by bartonjs
Loading

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@bartonjs bartonjs mentioned this pull request Jul 12, 2022
Closed
2 tasks
@bartonjs
Copy link
Member

(Waiting on sending the mail until we actually merge)

@bartonjs bartonjs added cryptographic-docs-impact new-api-needs-documentation and removed new-api-needs-documentation needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Jul 12, 2022
@bartonjs bartonjs merged commit 388dd6c into dotnet:main Jul 12, 2022
@bartonjs bartonjs mentioned this pull request Jul 13, 2022
Merged
@wfurt
Copy link
Member Author

wfurt commented Jul 13, 2022

Thanks @bartonjs. Is this breaking because of the time change in X509ChainPolicy? If so, are you going to write the description of the behavior change?

@wfurt wfurt changed the title add ValidationPolicy to ssl options add CertificateChainPolicy to ssl options Jul 13, 2022
@bartonjs
Copy link
Member

Is this breaking because of the time change in X509ChainPolicy?

Yep.

If so, are you going to write the description of the behavior change?

Did that prior to merging it in 😄

@stephentoub
Copy link
Member

ChainPolicy.ExtraStore is written into by GetRemoteCertificate as part of establishing connections. If the caller now provides an X509ChainPolicy instance with this feature, doesn't that mean we're then storing into that shared ExtraStore each time? Which would have implications on thread-safety (multiple connections all calling GetRemoteCertificate concurrently and adding to this non-thread-safe collection concurrently), memory leaks (unbounded additions to that ExtraStore each time a new connection is created / the remote is validated), and potentially security issues with the cert from one connection being then used as part of validation / chain building for another connection?

@stephentoub
Copy link
Member

@bartonjs pointed out to me offline that we always clone the policy and ensure the clone is only used once.

@wfurt wfurt mentioned this pull request Jul 26, 2022
Closed
@rzikm rzikm mentioned this pull request Jul 29, 2022
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Aug 17, 2022
@bartonjs bartonjs added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Aug 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@bartonjs bartonjs bartonjs approved these changes

@vcsjones vcsjones Awaiting requested review from vcsjones

Assignees

@wfurt wfurt

Labels
area-System.Net.Security breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration new-api-needs-documentation
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

Add X509ChainPolicy to SslOptions
3 participants
@wfurt @bartonjs @stephentoub