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

Add X509ChainPolicy to SslOptions #71191

Closed
wfurt opened this issue Jun 23, 2022 · 3 comments · Fixed by #71961
Closed

Add X509ChainPolicy to SslOptions #71191

wfurt opened this issue Jun 23, 2022 · 3 comments · Fixed by #71961
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Security
Milestone

Comments

@wfurt
Copy link
Member

wfurt commented Jun 23, 2022

Background and motivation

When TLS handshake is completed SslStream does peer certificate validation. This happens always on client and it can happen on server if client cert is provided. This process can for example download additional certificates over network and there is no way how caller can impact the processing. Only after al this is done custom delegate can be called for additional checks. This is inconvenient and it can possibly lead to performance or security issues.

SslStream currently creates X509ChainPolicy behind the curtain to valid peer's certificate.
The proposal is to add existing X509ChainPolicy to SslOptions so callers of SslStream can customize the validation process. If provided, it would be used exclusively and existing properties impacting validation (like CertificateRevocationCheckMode) will be ignored.

related to #59979, #35839, #59944, #40423

API Proposal

namespace System.Net.Security
{

    public class SslServerAuthenticationOptions
    {
        ....
+       X509ChainPolicy? ValidationPolicy;
    }
    public class SslClientAuthenticationOptions
    {
        ....
+       X509ChainPolicy? ValidationPolicy;
    }
}

API Usage

on client

X509ChainPolicy policy = new X509ChainPolicy();
policy.CustomTrustMode = CustomRootTrust;
policy.TrustStore.Add(s_ourPrivateRoot);
policy.UrlRetrievalTimeout = TimeSpan.FromSeconds(3);

SslStreamClientOptions options  = new SslStreamClientOptions();
options.TargetName = "myServer";
options.ValidationPolicy = policy;

var ssl = new SslStream(transportStream); 
ssl.AuthenticateAsClientAsync(options, cancellationToken);

on server preventing downloads:

X509ChainPolicy policy = new X509ChainPolicy();
policy.DisableCertificateDownload = true;
var options = new SslServerAuthenticationOptions();
options. ValidationPolicy = policy;

var ssl = new SslStream(transportStream); 
ssl.AuthenticateAsServerAsync(options, cancellationToken);

Alternative Designs

We could add specific properties to SslOptions and use them when creating similar to CertificateRevocationCheckMode. The problem is duplication as well as maintenance. We may add new options to X509ChainPolicy and it would be immediately available to callers of SslStream. If we keep adding discrete properties we will be always behind.

Risks

Current validation is hidden from callers and pretty simple. Fiddling with X509ChainPolicy is for advanced users and misconfiguring it can have security impact.

@wfurt wfurt added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 23, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 23, 2022
@ghost
Copy link

ghost commented Jun 23, 2022

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

Issue Details

Background and motivation

When TLS handshake is completed SslStream does peer certificate validation. This happens always on client and it can happen on server if client cert is provided. This process can for example download additional certificates over network and there is no way how caller can impact the processing. Only after al this is done custom delegate can be called for additional checks. This is inconvenient and it can possibly lead to performance or security issues.

SslStream currently creates X509ChainPolicy behind the curtain to valid peer's certificate.
The proposal is to add existing X509ChainPolicy to SslOptions so callers of SslStream can customize the validation process. If provided, it would be used exclusively and existing properties impacting validation (like CertificateRevocationCheckMode) will be ignored.

related to #59979, #35839, #59944, #40423

API Proposal

namespace System.Net.Security
{

    public class SslServerAuthenticationOptions
    {
        ....
+       X509ChainPolicy? ValidationPolicy;
    }
    public class SslClientAuthenticationOptions
    {
        ....
+       X509ChainPolicy? ValidationPolicy;
    }
}

API Usage

on client

X509ChainPolicy policy = new X509ChainPolicy();
policy.CustomTrustMode = CustomRootTrust;
policy.TrustStore.Add(s_ourPrivateRoot);
policy.UrlRetrievalTimeout = TimeSpan.FromSeconds(3);

SslStreamClientOptions options  = new SslStreamClientOptions();
options.TargetName = "myServer";
options.ValidationPolicy = policy;

var ssl = new SslStream(transportStream); 
ssl.AuthenticateAsClientAsync(options, cancellationToken);

on server preventing downloads:

X509ChainPolicy policy = new X509ChainPolicy();
policy.DisableCertificateDownload = true;
var options = new SslServerAuthenticationOptions();
options. ValidationPolicy = policy;

var ssl = new SslStream(transportStream); 
ssl.AuthenticateAsServerAsync(options, cancellationToken);


### Alternative Designs

We could add specific properties to SslOptions and use them when creating similar to `CertificateRevocationCheckMode`. The problem is duplication as well as maintenance. We may add new options to `X509ChainPolicy` and it would be immediately available to callers of SslStream. If we keep adding discrete properties we will be always behind. 

### Risks

Current validation is hidden from callers and pretty simple. Fiddling with `X509ChainPolicy` is for advanced users and misconfiguring it can have security impact.

<table>
  <tr>
    <th align="left">Author:</th>
    <td>wfurt</td>
  </tr>
  <tr>
    <th align="left">Assignees:</th>
    <td>-</td>
  </tr>
  <tr>
    <th align="left">Labels:</th>
    <td>

`api-suggestion`, `area-System.Net.Security`

</td>
  </tr>
  <tr>
    <th align="left">Milestone:</th>
    <td>-</td>
  </tr>
</table>
</details>

@wfurt wfurt removed the untriaged New issue has not been triaged by the area owner label Jun 23, 2022
@wfurt wfurt added this to the 7.0.0 milestone Jun 23, 2022
@wfurt wfurt added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 23, 2022
@bartonjs
Copy link
Member

bartonjs commented Jul 7, 2022

Video

  • The new properties should be properties, not fields.
  • We added some enhancements to X509ChainPolicy to increase the usability and maintainability here.
  • Consider an analyzer that warns if both ValidationPolicy and one of the older properties (such as CertificateRevocationCheckMode) are both assigned
namespace System.Net.Security
{
    public partial class SslServerAuthenticationOptions
    {
        public X509ChainPolicy? ValidationPolicy { get; set; }
    }
    public class SslClientAuthenticationOptions
    {
        public X509ChainPolicy? ValidationPolicy { get; set; }
    }
}

namespace System.Security.Cryptography.X509ChainPolicy
{
    public partial class X509ChainPolicy
    {
        public X509ChainPolicy Clone();
        // Breaking change: This defaults to true (and gets set to false if set_VerificationTime is called)
        public bool VerificationTimeIgnored { get; set; }
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 7, 2022
@billknye
Copy link

billknye commented Jul 7, 2022

It seems like maybe VerificationTimeIgnored is telling the chain policy not to do verification of the time, not to verify using DateTime.Now.

@wfurt wfurt self-assigned this Jul 11, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 11, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 12, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants