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

SslServerAuthenticationOptions API additions and suggestions #26210

Closed
baulig opened this issue May 17, 2018 · 12 comments
Closed

SslServerAuthenticationOptions API additions and suggestions #26210

baulig opened this issue May 17, 2018 · 12 comments
Labels
area-System.Net.Security enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@baulig
Copy link
Contributor

baulig commented May 17, 2018

I have a few questions and suggestions regarding Client Certificates, TLS Renegotiation and the new SslServerAuthenticationOptions API:

  1. What is the purpose of the EncryptionPolicy property - is anyone actually using this for anything?
    When would you set this to something other than RequireEncryption?

  2. Do we actually need SslServerAuthenticationOptions.AllowRenegotiation?
    Renegotiation is typically initiated by the server and not a client and there is also lack of support for it from some of the native backends - for instance, neither BoringSSL nor Apple's SecureTransport allow renegotiation requests from a client.
    The default value of it should also be changed to false when using any of the native backends where it's not supported.

  3. We should add something like this to SslServerAuthenticationOptions (possibly using X500DistinguishedName[] instead of string[]):

		public string[] ClientCertificateIssuers {
			get; set;
		}

There is SSLAddDistinguishedName() in Secure Transport and SSL_CTX_set_client_CA_list() in OpenSSL / BoringSSL that can be used in the native backend.

  1. There is no API to trigger TLS Renegotiation.

As part of my work on implementing Client Certificates in Mono, I added the following to a Mono-specific interface:

		bool CanRenegotiate {
			get;
		}

		Task RenegotiateAsync (CancellationToken cancellationToken);

How would you feel about adding something like that to SslStream?

The CanRenegotiate property is there because not all native backends support TLS Renegotiation (Secure Transport does, BoringSSL does not).

  1. Would it make sense to add something like this to SslClientAuthenticationOptions:
		public bool DisallowUnauthenticatedCertificateRequest {
			get; set;
		}

The reasoning is that setting ClientCertificates without also using LocalCertificateSelectionCallback will send the client certificates during the initial handshake, before the server has authenticated itself. This could be a potential privacy issue. This property would essentially require the server to renegotiate before client authentication can be used.

  1. There is no API to tell HttpListener to renegotiate. Are there any plans on adding something to it?

  2. There is HttpWebRequest.ClientCertificates, but no LocalCertificateSelectionCallback. So you cannot use the callback to distinguish between the initial handshake and renegotiation (see also point 5).

@karelz
Copy link
Member

karelz commented Sep 5, 2019

Let's review the list and decide which things we want to do and when ...

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@karelz karelz modified the milestones: 5.0, Future Feb 22, 2020
@mconnew
Copy link
Member

mconnew commented Mar 18, 2020

@baulig, you can trigger renegotiation in some situations using HttpListenerRequest.GetClientCertificate(). WCF does this on .NET Framework. When the listening port has been configured in HTTP.SYS to allow client certificates but not require one, it won't ask for one from the client in the initial TLS handshake. When WCF has decided the requesting URL needs a client certificate, it calls GetCertificate() which triggers an SSL renegotiation.

@mconnew
Copy link
Member

mconnew commented Mar 18, 2020

This is necessary for CoreWCF when using Kestrel.

@karelz
Copy link
Member

karelz commented Mar 18, 2020

@mconnew this issue has several things, can you be more specific what exactly is needed?

@mconnew
Copy link
Member

mconnew commented Mar 19, 2020

Specifically allow renegotiation. The scenario is having a server listen over https but not require a client certificate for the initial TLS handshake. Once CoreWCF has read the HTTP headers, it might decide it needs the client to provide a client certificate. At that point the server needs to request a TLS renegotiation asking for the client certificate. We build on top of ASP.NET Core and Kestrel uses SslStream so this is an indirect usage, but we still need this functionality to exist for ASP.NET Core to be able to provide us this capability.

@wfurt
Copy link
Member

wfurt commented Jun 10, 2020

I agree that 1. is pretty lame. However removal would be breaking change. I don't think it is worth to touch it.

For 2., this is mostly about the peer. The question is if we can trigger callback if peer asks and if we want to allow it. This may be corner case but removal would be breaking change as well.

What problem are you trying to solve with 3. @baulig ? @bartonjs may have some better insight to certificate details. It seems like most servers use altName and that is either host name (with wildcard) or IP.

  1. was discussed for a while but never high enough on priority list. Without it, it is also hard to test some SslStream behavior. It may be worth of creating separate issue with API proposal. Also note, that is not possible with TLS1.3. (and probably above)

  2. I don't have enough background to guess. Would that intern with other implementations?
    maybe @blowdart have some thoughts.

  3. no plans to invest in HttpListener AFAIK. We may take contributions but I'm not sure about it. Perhaps something for @KarlZ to comment on.

  4. I don't think we will touch HttpWebRequest for the same reasons as above.

@wfurt
Copy link
Member

wfurt commented Feb 19, 2021

  1. is essential duplicate of Developers using Kestrel can configure the list of CAs per-hostname #45456 and likely to get addresses in 6.0

Renegotiation does not exist in TLS1.3 so all this may come to past. I think wee should preserve existing behavior but not invest into new API. Aside from testing, I'm not sure how RenegotiateAsync would be used in practice without changing session parameters.

  1. can be solved with the callback, right @baulig? My preference would be to perhaps update documentation to make it clear instead of adding new API. I'm not sure if this would be feasible on Windows.

with this, I'm inclined to close this issue. Let me know @baulig and @mconnew if you have concerns.

cc: @aik-jahoda

@mconnew
Copy link
Member

mconnew commented Feb 19, 2021

We shouldn't be forcing HTTP/2 and TLS1.3 semantics down level to earlier HTTP and TLS protocols. Lack of being able to renegotiate prevents moving some existing WCF services to CoreWCF.

Some customers will have multiple endpoints on their service running on the same SSL port where one endpoint is using client certificate authentication, and another endpoint is using something else such as some HTTP authentication scheme. Without the ability to renegotiate, client side changes are needed to use different hostnames for the two endpoints, and they might not own the client side. It would also add a requirement for the client side to support SNI which isn't always the case. And it would add an extra cost to purchase a second service certificate for a second hostname.

@wfurt
Copy link
Member

wfurt commented Feb 19, 2021

What would you need from SslStream @mconnew ? As you mention, you can ask for client certificate and that would trigger the renegotiation when possible. Do you have example of functionality that works in .NET Framework but not in Core?

@mconnew
Copy link
Member

mconnew commented Feb 19, 2021

It's been a little while since I looked at the API and implementation and I just took another look as it looks like things have moved along since .NET Framework. I'm not seeing where asking for the client certificate would trigger renegotiation. SslStream.RemoteCertificate is a simply property getter and doesn't trigger anything. Am I missing something?

In .NET Framework, WCF uses either HttpListener or ASP.NET with IIS which handles the renegotiation. On Core the new CoreWCF is built on top of ASP.NET Core, and Kestrel is the primary web host. That uses SslStream to handle the SSL/TLS layer, and as that can't renegotiate after the headers are received, we can't provide functionality parity with .NET Framework.

@Tratcher
Copy link
Member

Tratcher commented Mar 9, 2021

I've opened a dedicated issue for (4), client cert negotiation. #49346

@wfurt
Copy link
Member

wfurt commented Feb 18, 2022

This is very old issue and I would like to drive to closure. Here is summary of proposed actions:

  1. What is the purpose of the EncryptionPolicy
    This is clearly dead. I opened mark EncryptionPolicy.NoEncryption as obsolete #65545 to obsolete it

  2. Do we actually need SslServerAuthenticationOptions.AllowRenegotiation
    I agree. Change default of SslServerAuthenticationOptions.AllowRenegotiation to be false #65547 tracks that now.

  3. We should add something like this to SslServerAuthenticationOptions (possibly using X500DistinguishedName[] instead of string[]):
    This was added in 6.0 as Add API to control trust during TLS handshake #54219. There is follow-up work in 7.0 to make it work for Linux and macOS

  4. There is no API to trigger TLS Renegotiation
    While there is no generic API for renegotiation, SslStream delayed client certificate negotiation #49346 @Tratcher mentioned was done in 6.0 and it allows to trigger it indirectly by asking for client certificate after handshake.

  5. Would it make sense to add something like this to SslClientAuthenticationOptions: DisallowUnauthenticatedCertificateRequest
    I think TLS spec is pretty clear that client should not send certificates unless server asked for it. If we do on what ever platform it is BUG imho and we should fix it without adding new property.

  6. There is no API to tell HttpListener to renegotiate.
    HttpListener is consider obsolete and we plan no improvements to it. New api SslStream delayed client certificate negotiation #49346 is available in Kestrel.

  7. There is HttpWebRequest.ClientCertificates , but no LocalCertificateSelectionCallback.
    HttpWebRequest is considered obsolete. There is no plan to plan to improve it.

multi-issues like this are hard to track and complete. I will close it and if there is any disagreement please open specific issue.
cc: @rzikm

@wfurt wfurt closed this as completed Feb 18, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2022
@karelz karelz modified the milestones: Future, 7.0.0 Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Security enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

6 participants