PoC: support custom X509ChainPolicy in SslStream #69908

wfurt · 2022-05-27T17:45:59Z

Related to #59979.
Allowing users to provide custom policy give option to disable AIA or influence other aspects of validation.

we would copy provided policy when we capture other properties in SslAuthenticationOptions when AuthenticateAs*() is called.
Unless VerificationTime was set, it would resolve to DateTime.Now e.g. same behavior as now.
If custom policy is set, SslStream would ignore X509RevocationMode and Trust e.g. complete policy takes precedence. (similar how we prioritize SslStreamCertificateContext over X509Certificate) Then X509ChainPolicy is not provided, we would construct it same way as now from the bits.
If ApplicationPolicy is not set, we would always create it and set EKU. This one deviates from above. The may goal is to preserve right behavior for cases when only custom trust is desired, like:

X509ChainPolicy policy = new X509ChainPolicy();

policy.CustomTrustMode = CustomRootTrust;

policy.TrustStore.Add(s_ourPrivateRoot);

// whatever else they want to set.



SslStreamClientOptions clientOpts = ...;

clientOpts.ChainPolicy = policy;



SslStreamServerOptions serverOpts = ...;

serverOpts.ChainPolicy = policy;

If ApplicationPolicy is set, we would preserve it, assuming it is what caller really wants.

The Duplicate creates copy of _extraStore because this is place where certificates from the TLS session go e.g. may be unique for each TLS session. I was wondering about the CustomTrustStore but I felt we don't need extra copy and allocations.

Please comment as you see fit. This is mostly to illustrate the intention and behavior.

ghost · 2022-05-27T17:46:04Z

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 · 2022-05-27T17:46:05Z

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

Issue Details

Related to #59979.
Allowing users to provide custom policy give option to disable AIA or influence other aspects of validation.

we would copy provided policy when we capture other properties in SslAuthenticationOptions when AuthenticateAs*() is called.
Unless VerificationTime was set, it would resolve to DateTime.Now e.g. same behavior as now.
If custom policy is set, SslStream would ignore X509RevocationMode and Trust e.g. complete policy takes precedence. (similar how we prioritize SslStreamCertificateContext over X509Certificate) Then X509ChainPolicy is not provided, we would construct it same way as now from the bits.
If ApplicationPolicy is not set, we would always create it and set EKU. This one deviates from above. The may goal is to preserve right behavior for cases when only custom trust is desired, like:

X509ChainPolicy policy = new X509ChainPolicy();

policy.CustomTrustMode = CustomRootTrust;

policy.TrustStore.Add(s_ourPrivateRoot);

// whatever else they want to set.



SslStreamClientOptions clientOpts = ...;

clientOpts.ChainPolicy = policy;



SslStreamServerOptions serverOpts = ...;

serverOpts.ChainPolicy = policy;

If ApplicationPolicy is set, we would preserve it, assuming it is what caller really wants.

The Duplicate creates copy of _extraStore because this is place where certificates from the TLS session go e.g. may be unique for each TLS session. I was wondering about the CustomTrustStore but I felt we don't need extra copy and allocations.

Please comment as you see fit. This is mostly to illustrate the intention and behavior.

Author:	wfurt
Assignees:	wfurt
Labels:	`area-System.Net.Security`
Milestone:	-

bartonjs · 2022-05-27T21:38:37Z

...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509ChainPolicy.cs


        public TimeSpan UrlRetrievalTimeout { get; set; }

+        public X509ChainPolicy Duplicate()
+        {
+            var dup = (X509ChainPolicy)this.MemberwiseClone();


Everything should just be cloned manually. All of the collection values need to be cloned to avoid delayed or concurrent modification (e.g. you're potentially adding to ApplicationPolicy, which could be an empty, non-null collection).

could be done. I did not want to duplicate things we don't really need to save allocations.

If it's going to be a public API and Duplicate doesn't duplicate everything, I think that would be somewhat odd. I think most callers would expect a deep copy, and not a shallow copy.

Things that would need to be copied:

internal OidCollection? _applicationPolicy; internal OidCollection? _certificatePolicy; internal X509Certificate2Collection? _extraStore; internal X509Certificate2Collection? _customTrustStore;

We know we need to copy _extraStore. So the number of allocations we would save are three class objects. Since X509Certificate2 and Oid are (mostly) immutable, I don't think the individual objects themselves need to be copied, just the collections.

Three allocations for a "behaves as expected" API seems acceptable to me, and the copies would only need to be done if not null.

Though I wonder if we should be deep copying everything anyway. This location is modifying _customTrustStore

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs

Lines 980 to 983 in 29277bc

if (trust._store != null)

{

chain.ChainPolicy.CustomTrustStore.AddRange(trust._store.Certificates);

}

and this modified application policy:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs

Lines 966 to 969 in 29277bc

if (chain.ChainPolicy.ApplicationPolicy.Count == 0)

{

// Authenticate the remote party: (e.g. when operating in server mode, authenticate the client).

chain.ChainPolicy.ApplicationPolicy.Add(_sslAuthenticationOptions.IsServer ? s_clientAuthOid : s_serverAuthOid);

Since those are not copied by Duplicate, would those modifications be observed by callers?

The alternative, of course, is not ship this as a public API if we are sensitive to the object allocations.

I was thinking about it. I had offline chat with @bartonjs and this PR is out to flesh out possible behavior to API proposed in #59979 (comment) . The policy is not in center but we should make sure we can achieve reasonable behavior. (and solve the underlying use cases (listed only one here))

bartonjs · 2022-05-27T21:39:45Z

src/libraries/System.Net.Security/src/System/Net/Security/SslServerAuthenticationOptions.cs

@@ -74,5 +74,10 @@ public EncryptionPolicy EncryptionPolicy
        /// Use extreme caution when changing this setting.
        /// </summary>
        public CipherSuitesPolicy? CipherSuitesPolicy { get; set; }
+
+        /// <summary>
+        /// Sepcifies specific policy for verifying remote cretrificate.


Suggested change

/// Sepcifies specific policy for verifying remote cretrificate.

/// Specifies the policy for verifying the remote certificate.

bartonjs · 2022-05-27T21:42:50Z

src/libraries/System.Net.Security/src/System/Net/Security/SslAuthenticationOptions.cs

+                if (CertificateContext.Trust != null && sslServerAuthenticationOptions.ChainPolicy != null &&
+                    sslServerAuthenticationOptions.ChainPolicy.TrustMode != X509ChainTrustMode.CustomRootTrust)
+                {
+                    throw new InvalidOperationException("Unable to use custom trust without custom root"); ;


I think the intent here is that if you specified a trust value and a policy that you have to be in custom root trust on the policy... but it doesn't verify that they're the same contents.

Unless it's going to verify they're the same set, I'd just ignore it. Trust would still control the trust list in the handshake (if opted in), but if there's also a policy then it'll do what the policy says. (That also solves the problem of what to do about future potential trust modes, like CustomAnchors (stop at intermediates that are in the collection, don't have to be roots) or SystemOrCustomRootTrust (which isn't very likely))

That would be ok. We can define what the behavior is.

rzikm · 2022-05-30T07:58:40Z

src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs

@@ -2908,6 +2908,7 @@ public sealed partial class X509ChainPolicy
        public System.TimeSpan UrlRetrievalTimeout { get { throw null; } set { } }
        public System.Security.Cryptography.X509Certificates.X509VerificationFlags VerificationFlags { get { throw null; } set { } }
        public System.DateTime VerificationTime { get { throw null; } set { } }
+        public System.Security.Cryptography.X509Certificates.X509ChainPolicy Duplicate() { throw null; }


Why not name it Clone? is this some specific convention?

Clone would be fine. I don't care that much about the name. The big question is is if we want to add public method here (like I did) or if we hide it and make it visible only to runtime.

I thought that since we have MemberwiseClone then it is more common to use Clone instead of Duplicate or other names, but I don't have too strong opinion on this.

rzikm · 2022-05-30T07:59:13Z

...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509ChainPolicy.cs

+        {
+            get
+            {
+                return _verificationTime.Equals(default(DateTime)) ? DateTime.Now : _verificationTime;


what about DateTime.UtcNow?

I used DateTime.Now because it is used by the Reset. I can change that if we want to.

UtcNow is more efficient AFAIK, so unless it would change behavior I think changing it would be good idea.

I would consider using a nullable DateTime? instead of DateTime.MinValue as a null-sentinel.

That would be great but DateTime is struct. I originally thought about adding internal 'bool' to track if the time was set or not. The use case here is single policy that is used over time for each ssl session. So unless set explicitly, we would like to default to what ever now it is when new session is created.

There are a couple of things here, and I glossed over them.

There's a question of should the default be "when Build is called" or "when the Policy object was built". We've seen from people needing to work around it that people who save policy objects long term thought the default was "when Build is called".

Using a "have you called the property setter" secret internal state makes things a bit weird, since there's no way to return to that state after the fact.

We can't change the property to be a nullable

There's a question of should the getter be honest and say it's a sentinel value, and the if goes into X509Chain.Build.

DateTime.Now vs DateTime.UtcNow: It was previously Now, and Windows needs it (IIRC) in local time, so Now probably makes more sense... but I didn't comment about previously this because this is a proof of concept, not something ready to merge.

Duplicate vs Clone: I don't have strong opinions. Pick a name, and we'll let API Review decide which one is best.

I don't have a problem with the public concept of cloning the object, but I think it should be a semi-shallow clone: The collections are unique, the objects in them are not. (So we don't clone any X509Certificate2 instances or Oid instances, they're both mostly-read-only anyways).

The clone can certainly turn empty collections back into null ones.

wfurt · 2022-05-31T07:03:09Z

Let me know @bartonjs if there are conceptual issues. We can sort out details during reviews but I would lie to write API proposal ready for API review.

vcsjones · 2022-05-31T14:43:56Z

...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509ChainPolicy.cs


        public TimeSpan UrlRetrievalTimeout { get; set; }

+        public X509ChainPolicy Duplicate()
+        {
+            var dup = (X509ChainPolicy)this.MemberwiseClone();


If it's going to be a public API and Duplicate doesn't duplicate everything, I think that would be somewhat odd. I think most callers would expect a deep copy, and not a shallow copy.

Things that would need to be copied:

internal OidCollection? _applicationPolicy; internal OidCollection? _certificatePolicy; internal X509Certificate2Collection? _extraStore; internal X509Certificate2Collection? _customTrustStore;

We know we need to copy _extraStore. So the number of allocations we would save are three class objects. Since X509Certificate2 and Oid are (mostly) immutable, I don't think the individual objects themselves need to be copied, just the collections.

Three allocations for a "behaves as expected" API seems acceptable to me, and the copies would only need to be done if not null.

Though I wonder if we should be deep copying everything anyway. This location is modifying _customTrustStore

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs

Lines 980 to 983 in 29277bc

if (trust._store != null)

{

chain.ChainPolicy.CustomTrustStore.AddRange(trust._store.Certificates);

}

and this modified application policy:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs

Lines 966 to 969 in 29277bc

if (chain.ChainPolicy.ApplicationPolicy.Count == 0)

{

// Authenticate the remote party: (e.g. when operating in server mode, authenticate the client).

chain.ChainPolicy.ApplicationPolicy.Add(_sslAuthenticationOptions.IsServer ? s_clientAuthOid : s_serverAuthOid);

Since those are not copied by Duplicate, would those modifications be observed by callers?

vcsjones · 2022-05-31T14:47:58Z

...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509ChainPolicy.cs

@@ -109,7 +132,7 @@ public void Reset()
            _revocationFlag = X509RevocationFlag.ExcludeRoot;
            _verificationFlags = X509VerificationFlags.NoFlag;
            _trustMode = X509ChainTrustMode.System;
-            VerificationTime = DateTime.Now;
+            _verificationTime = default;


This would be a breaking change, correct? Previous behavior: _verificationTime would be whatever DateTime.Now is when Reset or .ctor was created, while now VerificationTime is "now" every time it is accessed until it has been explicitly set.

I don't think this is a bad change, I just would want to understand why, and, consider what might break of consumers.

vcsjones · 2022-05-31T14:55:14Z

...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509ChainPolicy.cs

+        {
+            get
+            {
+                return _verificationTime.Equals(default(DateTime)) ? DateTime.Now : _verificationTime;


I would consider using a nullable DateTime? instead of DateTime.MinValue as a null-sentinel.

vcsjones · 2022-05-31T14:57:27Z

...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509ChainPolicy.cs


        public TimeSpan UrlRetrievalTimeout { get; set; }

+        public X509ChainPolicy Duplicate()
+        {
+            var dup = (X509ChainPolicy)this.MemberwiseClone();


The alternative, of course, is not ship this as a public API if we are sensitive to the object allocations.

wfurt · 2022-05-31T18:08:27Z

Based on the feedback, one option would be to unseal X509ChainPolicy Then the SslStream (and QUIC) should do whatever need to be done to efficiently process n ew sessions. if we end up with public clone/duplicate method I agree it would be strange to make it optimize for just TLS processing.

bartonjs · 2022-05-31T19:20:46Z

one option would be to unseal X509ChainPolicy

No. 😄

ghost · 2022-07-07T17:02:28Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

custom chain policy

29277bc

wfurt added the area-System.Net.Security label May 27, 2022

wfurt requested review from vcsjones, bartonjs and rzikm May 27, 2022 17:45

wfurt self-assigned this May 27, 2022

ghost added the new-api-needs-documentation label May 27, 2022

bartonjs reviewed May 27, 2022

View reviewed changes

rzikm reviewed May 30, 2022

View reviewed changes

vcsjones reviewed May 31, 2022

View reviewed changes

wfurt marked this pull request as draft June 7, 2022 16:20

ghost closed this Jul 7, 2022

wfurt mentioned this pull request Jul 11, 2022

add CertificateChainPolicy to ssl options #71961

Merged

karelz added this to the 7.0.0 milestone Jul 19, 2022

ghost locked as resolved and limited conversation to collaborators Aug 18, 2022

This pull request was closed.

	if (trust._store != null)
	{
	chain.ChainPolicy.CustomTrustStore.AddRange(trust._store.Certificates);
	}

	if (chain.ChainPolicy.ApplicationPolicy.Count == 0)
	{
	// Authenticate the remote party: (e.g. when operating in server mode, authenticate the client).
	chain.ChainPolicy.ApplicationPolicy.Add(_sslAuthenticationOptions.IsServer ? s_clientAuthOid : s_serverAuthOid);

	/// Sepcifies specific policy for verifying remote cretrificate.
	/// Specifies the policy for verifying the remote certificate.

PoC: support custom X509ChainPolicy in SslStream #69908

PoC: support custom X509ChainPolicy in SslStream #69908

Uh oh!

Conversation

wfurt commented May 27, 2022

Uh oh!

ghost commented May 27, 2022

Uh oh!

ghost commented May 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzikm May 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wfurt commented May 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wfurt commented May 31, 2022

Uh oh!

bartonjs commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 7, 2022

Uh oh!

Uh oh!

rzikm May 30, 2022 •

edited

Loading

bartonjs commented May 31, 2022 •

edited

Loading