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

Controlling TRUSTED_ISSUERS in SslStream (platform differences between Unix & Windows) #23514

Closed
ayende opened this issue Sep 11, 2017 · 22 comments
Labels
area-System.Net.Security bug tenet-compatibility Incompatibility with previous versions or .NET Framework
Milestone

Comments

@ayende
Copy link
Contributor

ayende commented Sep 11, 2017

This is related to https://github.com/dotnet/corefx/issues/17226 and aspnet/KestrelHttpServer#1802

On Windows, when using SslStream and requiring client certificates using AuthenticateAsServer an empty list of trusted issuers will be sent. This can be seen when running: penssl s_client -connect 192.168.1.11:8080

The results on Windows include

No client certificate CA names sent

On Linux, however, the server will send a list of all the trusted issuers on the system. This is actually handled by CoreFX directly, in this method:
https://github.com/dotnet/corefx/blob/0fd3d2c5bcb1a83e7ebd8025e4810e11e231ce03/src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs#L323

The code there is explicitly adding the registered certificates. On some machines, this can be a VERY big list. However, the calling code has no control over that and has to accept the machine state.

This is problematic, and a solution that will allow us to control what are the trusted issuers would be much better for us.

The underlying reason is that currently a user may specify a certificate, but get an error that no such certificate was used, which is really confusing and require to dig into which is the trusted issuer, what is going on and a lot of other annoyances.

There is also now way for us to inject any behavior into the system to control it.
Another overload on SslStream that would allow configuring the ssl context directly before use would be much better, but anything that would allow us to avoid sending the full issuers list all the time would be sufficent.

This seems to impact quite a few people and it is the cause of very hard to diagnose issues.

@Drawaes
Copy link
Contributor

Drawaes commented Sep 11, 2017

#23157

The above issue is a redesign of the SSLStream api to allow for easy edition of such knobs and levers. In light of that (and if it goes through which I believe it should) how would you like the config point to look?

It might also be important with SNI to be able to provide a map of host -> calist as different hostnames might want different certs.

@ayende
Copy link
Contributor Author

ayende commented Sep 11, 2017

It would be easiest if we could just get an extension point, similar to the current ones with userCertificateSelectionCallback and userCertificateValidationCallback. If we can have another one that will give us something like userTrustedIssuersSelectionCallback that would handle both scenarios, no?

@Drawaes
Copy link
Contributor

Drawaes commented Sep 11, 2017

The issue is around if you want to have a different list per SNI host. OpenSSL will let you have a callback but as far as I can tell (and those at MS have yet go say otherwise) you need an upfront list for SChannel.

Eitherway I agree some way of configuring it is required as Http/2 disallows renegotiation so you need the correct client cert in the initial handshake or client certs will be completely useless.

@ayende
Copy link
Contributor Author

ayende commented Sep 11, 2017

The current problem doesn't relate to SNI or renegotiation or OpenSSL. It is the framework making all of these decisions, the problem is that there is no way to get there.

@Drawaes
Copy link
Contributor

Drawaes commented Sep 11, 2017

I get that :) My point was merely we need to put in context of SNI and consider that as part of the design.

@Drawaes
Copy link
Contributor

Drawaes commented Sep 11, 2017

Also if you slot this in tmwith the SNI change which needs an API review as well you might get it quicker. As both need an API review ;)

@ayende
Copy link
Contributor Author

ayende commented Sep 26, 2017

Another thing to add here. When testing, we noticed that we had 21KB of data in the SSL handshake just to send the TRUSTED_ISSUERS, I don't think that this is expected.

@Drawaes
Copy link
Contributor

Drawaes commented Sep 26, 2017

No considering the trusted CA extension is a list of either names (x509 names) or cert hashes (fingerprints) even with the small encryption and header overhead you would be looking at 48 ca' s minimum.

@ayende
Copy link
Contributor Author

ayende commented Sep 27, 2017

I'm running the same command on the same server in both Linux and Windows:

openssl s_client -connect 127.0.0.1:8080

In Windows, I get:
SSL handshake has read 1486 bytes and written 443 bytes

So that is roughly one packet to read the information.

In Linux, I get:

SSL handshake has read 21939 bytes and written 891 bytes

I attached the raw output of the command to this message.

Note that the Linux machine is a plain Ubuntu machine without any special configuration

windows.txt
linux2.txt

@ayende
Copy link
Contributor Author

ayende commented Sep 28, 2017

Would is be possible to add a flag to disable this behavior?
I'm thinking about just add a check for the presence of an environment variable that a user can set to skip this.

@ayende
Copy link
Contributor Author

ayende commented Oct 8, 2017

Is there any progress here? The UpdateCAListFromRootStore seems to be a very different behavior for the CoreCLR between Windows and Linux, and it adds significantly to the complexity of anyone who want to use client certificates from Linux.

@ayende
Copy link
Contributor Author

ayende commented Oct 16, 2017

Hi, any response on this?

@bartonjs
Copy link
Member

@ayende When the Linux version was written it was matching behavior with Windows. If Windows has since changed behaviors (which may have "already happened" but in a newer OS than was being baselined) then we may want to change the Linux behavior, but the reason that we did it was for consistency.

@ayende
Copy link
Contributor Author

ayende commented Oct 22, 2017

As far as I can see, (tested on Win 10 & Win 2016), this is the default Windows behavior.
Is there anything that needs to be done to create a PR for removing this behavior?

@karelz
Copy link
Member

karelz commented Nov 30, 2017

@Priya91 thinks this is in crypto APIs, not in SslStream.

@bartonjs
Copy link
Member

https://github.com/dotnet/corefx/blob/a77d5aae4790689b86a9910ca981825b72daa9a8/src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs#L99 (AllocateSslContext).

It's definitely SslStream-specific code. It was written to match SslStream from Windows Server 2012 R2 (IIRC), and Windows 10 seems to have changed the default behavior within SChannel, which results in changing how SslStream works here on Windows.

If Networking/SslStream wants to match Windows 10 (after confirming with security feature processes that the change is the correct one to make) just delete the method call, and the method (and any otherwise abandoned methods). (Or leave things as is, make it an API option, force Windows 10 to behave like prior OSes, et cetera)

@karelz
Copy link
Member

karelz commented Nov 30, 2017

I think matching Windows 10 is reasonable. @Priya91 @wfurt thoughts?
@ayende if you want to submit the PR, please go ahead. Thanks!

@karelz karelz closed this as completed Nov 30, 2017
@karelz karelz reopened this Nov 30, 2017
@Priya91
Copy link
Contributor

Priya91 commented Dec 1, 2017

@ayende Do you know what scenarios will be broken if this change is made? Matching windows 10 behavior seems reasonable, given that this was added to maintain consistency with windows in the first place. Would you like to submit a PR to remove this?

@davidsh
Copy link
Contributor

davidsh commented Dec 1, 2017

Do you know what scenarios will be broken if this change is made?

We should very careful before changing this.

The default behavior in SCHANNEL for Windows Server 2012 and later was changed to not send TRUSTED_ISSUERS lists by default. This was done because frequently such lists are huge and they overflow the packet size limitations of TLS.

See:
https://technet.microsoft.com/en-us/library/dn786429(v=ws.11).aspx

@Drawaes
Copy link
Contributor

Drawaes commented Dec 1, 2017

That shouldn't break TLS and if it does something is wrong with the implementation. There are "max fragment sizes" but a message (eg handshake message) should be able to be broken into any number of these

he record layer fragments information blocks into TLSPlaintext
records carrying data in chunks of 2^14 bytes or less. Client
message boundaries are not preserved in the record layer (i.e.,
multiple client messages of the same ContentType MAY be coalesced
into a single TLSPlaintext record, or a single message MAY be
fragmented across several records).

But actually reading your link... windows seems to have a bad implementation..... that's a pain... pity there is no managed implementation and you are limited by SChannel.

@ayende
Copy link
Contributor Author

ayende commented Dec 1, 2017

@karelz I can send the PR, yes, will be here soon

@davidsh
Copy link
Contributor

davidsh commented Mar 15, 2019

This can be closed now that PR dotnet/corefx#25642 has been merged

@davidsh davidsh closed this as completed Mar 15, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Security bug tenet-compatibility Incompatibility with previous versions or .NET Framework
Projects
None yet
Development

No branches or pull requests

7 participants