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

SNI API for SslStream #23797

Closed
Drawaes opened this issue Oct 10, 2017 · 26 comments
Closed

SNI API for SslStream #23797

Drawaes opened this issue Oct 10, 2017 · 26 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Security
Milestone

Comments

@Drawaes
Copy link
Contributor

Drawaes commented Oct 10, 2017

Rationale

The SNI extension allows for multiple hostnames to be served from a single IP address/port combo. This is needed for Kestrel to be able to be a true web facing internet server using TLS.

The client side already supports sending a hostname so requires no change. The serverside should have a callback that gives the correct certificate/chain depending on a host name or returns null if none are available.

If no certificate is returned the handshake should fail.

This requires the ALPN Api to be completed first.

API Shape

namespace System.Net.Security {
public class SslServerAuthenticationOptions
{
    public ServerCertificateSelectionCallback ServerCertificateSelectionCallback { get; set; }
}

public delegate System.Security.Cryptography.X509Certificates.X509Certificate ServerCertificateSelectionCallback(object sender, string hostName);
}

Additional API information:

  • When host name could not be found the hostName value will be null when callback is called
  • hostName will never be an empty string
  • When caller returns null certificate we will throw AuthenticationException

Open questions:

  • ReadOnlySpan<byte> vs. string for hostName - for server API it might be appropriate to make callback with ReadOnlySpan<byte> - this way server does not have to make two allocations on each request (1 for UTF8 conversion and 1 for IDN conversion) - server could potentially do conversion beforehand and then do byte compare - for client we chose hostName but in the client case it is less important and should be more user friendly
    • If we choose ReadOnlySpan<byte>, should we add API to convert to string? (and where)

References

ALPN/Optionbag Dependent issue #23157 and PR dotnet/corefx#24389

/cc @Priya91 @Tratcher @geoffkizer @benaadams
EDIT (@krwq): updated API proposal, added open questions

@Tratcher
Copy link
Member

Something that came up during the ALPN design was that you may want to use other information from the handshake as inputs into cert selection such as ALPN. We should see how much information it's viable to surface during such a callback.

Side note: putting 'Sni' in the property and delegate names is unnecessary.

@Drawaes
Copy link
Contributor Author

Drawaes commented Oct 10, 2017

ALPN "might" be able to be surfaced, that in itself will be touch and go. For most of the libs SNI is the first callback that you then "switch in" an SslContext (in OpenSsl anyway) which then processes the ClientHello. So you might struggle to get ALPN, anything more than that I expect is a pipe dream (unless you had a managed TLS implementation 🤣

@Drawaes
Copy link
Contributor Author

Drawaes commented Oct 10, 2017

Updated top issue to drop Sni prefix

@PeterSmithRedmond
Copy link

It looks like the API is trying to give as little information as possible to the callback instead of the rich information that we could provide.

  • The ALPN RFC explicitly gives "picking different certificates" as a reason for ALPN

If we create a mini-class today with the SNI and ALPN (which will be null for many implementations), we solve the underlying problem. The API would then look more like this:

class SslInformation
{
public IList ApplicationProtocolList { get; set;}
public string ServerNameIndication {get; set; }
}

And then simply pass the class in to the authentication
public delegate (X509Certificate certificate, X509CertificateCollection chain) ServerCertficateCallback(SslInformation value);

It might be nice if we also provided the stream that we're fiddling with -- that way a server that wants different certs based on the port (for example) could cast the Stream to NetworkStream and then pull out the stream information as needed.

Two strange things are popping up in the API

  1. The SslServerAuthenticationOptions includes a cert but not a cert chain. This callback, however, provides both a cert and a cert chain. What's the rationale for having the cert chain in only one place?

  2. It also seems a little odd that we have a cert value in SslServerAuthenticationOptions and that the ServerCertficateCallback doesn't simply update that value.

@Drawaes
Copy link
Contributor Author

Drawaes commented Oct 11, 2017

Yes I agree on the first point IF we can validate we can get that information from the underlying providers (we might find our hands are tied here).

The Underlying stream might be a good idea, although for the port number you can have different SSLStreams... not 100% sure about that one have to think on it.

The chains, well I am not 100% sure why you can't supply the chain in the other one, might have to ask the people who designed it, but I would like to supply the chain ;)

Last point, because the "Options" would normally be cached and used for every connection. So say Kestrel would make one instance of those options and pass it into every Ssl Connection that it makes, changing it suddenly would be just nasty.

@Drawaes
Copy link
Contributor Author

Drawaes commented Oct 12, 2017

After some research, no can do on the ALPN by the way... not on OpenSsl anyway

image

openssl/openssl#819

Alpn is triggered AFTER SNI. Which makes sense anyway, you don't switch certificates due to the protocol, you might switch protocols you want to support however due to the hostname. So I think the original design stands in this case.

@PeterSmithRedmond
Copy link

The ALPN spec explicitly gives "picking a cert" as a use case. We shouldn't hamstring our designs just because one of the underlying implementations can't do it today.

Adding a mini-class with appropriate information (which might be null in the first implementation) is just prudent API design.

@Tratcher
Copy link
Member

Even if the final ALPN selection hasn't been calculated yet the client's ALPN list should be available, correct?

@Drawaes
Copy link
Contributor Author

Drawaes commented Oct 13, 2017

Are you sure without parsing the messages in managed code? Also has anyone checked if Schannel or securetransport can do it? Because while I agree for "1 provider" let's not hamstring it. But if non can do it then it's pointless.

@PeterSmithRedmond
Copy link

Both the SNI and APLN are sent as part of the initial ClientHello, The certificate is sent back as part of the ServerHello. There shouldn't be any reason why all that information isn't theoretically available. Since these two TLS extensions clearly and unambiguously intended to be used for server selection, we should have a slightly extensible API.

I'm not asking for much -- just that instead of passing in a single string, we pass in a structure with one string and (possibly) a list of client ALPN value.

@PeterSmithRedmond
Copy link

PeterSmithRedmond commented Oct 13, 2017

QQ about SNI. It's clear that we've been talking about the SNI as a single name, and that seems to be the obvious 99% case right now. But looking at the RFC, it looks like the actual SNI value is a list of names and (also theoretically) holds a byte which says what "kind" of name it is.

(No surprise, the only kind of name allowed is a DNS name)

Am I misreading the RFC? And if so, if we want to conform to the RFC, shouldn't the single string be a list of strings?


Thanks to Drawas -- I AM misreading the RFC. The text prohibits more than one hostname of any type, and there's only one type, and therefore there is only one hostname. One string is plenty.

@Drawaes
Copy link
Contributor Author

Drawaes commented Oct 13, 2017

Yes it theoretically is a list of strings so could be represented as such.

I get that it would be nice, and I understand it should be possible, however .nets policy is not to do TLS parsing (I would love it if there was a managed version) so unless something can actually supply this info (OpenSsl, SChannel etc) it's a bit misleading to have something that will never be populated.

I think the answer isn't No and infact I want it but someone needs to find out if it's possible.

@Drawaes
Copy link
Contributor Author

Drawaes commented Oct 13, 2017

Actually scrap that. I remember why it was a single string now. You can ONLY have host names at the moment (and at all to be honest, because TLS 1.3 is coming down the line this will not be back changed I would bet money on it, it's now 13 years old and unchanged).

The RFC clearly states

The ServerNameList MUST NOT contain more than one name of the same
name_type.

Therefore we are back to a single string

@karelz
Copy link
Member

karelz commented Nov 30, 2017

@Priya91 is the API ready for review?

@Priya91
Copy link
Contributor

Priya91 commented Dec 1, 2017

@Drawaes

The chains, well I am not 100% sure why you can't supply the chain in the other one, might have to ask the people who designed it, but I would like to supply the chain ;)

Why is the chain information useful? How would the handshake use this information from the sni?

@Tratcher @PeterSmithRedmond Regarding providing the client ALPN list, as far as I researched neither schannel nor openssl has any server-side apis that gives the list of protocols sent by client. This information may be obtained by hacking the apis during the alpn phase of the handshake, which may result in memory corruption, etc, but by that time, sni is completed. Is there a way to implement this correctly with the existing stack?

Couple of observations about the api itself

  1. Should the delegate definition be put on SslStream class instead of SslServerAuthenticationOptions, since this type is a feature of the SslStream itself, and not of options bag, which just has data to configure the sslstream. The other callbacks, RemoteCertificateValidationCallback and LocalCertificateSelectionCallback also exist on SslStream.

  2. The callback should also include an object sender, to pass down the sslstream object itself, in case the user wants to use any information from previous handshake steps during sni selection. Also if the underlying native stack does alpn before sni, the sni api can get alpn information by doing ((SslStream)object).NegotiatedApplicationProtocol

I'm against the idea of using structure to send down data to the callback, it adds unnecessary complexity. The only information that is required for doing sni is the targetHost as per the RFC. I would prefer keeping the contract simple and relevant on inputs and outputs.

@Drawaes
Copy link
Contributor Author

Drawaes commented Dec 1, 2017

I agree with the two observations and have made that change.

For the chains, how does one normally get a chain into an X509 cert, there are two ways I know of, load it from a "store" or load it from a PFX, is there another way? You can load a DER but it doesn't have the chain right?

I am worried that in a "dynamic" situation it will be hard to construct a chain on the fly say on Linux. If there is another way then I am cool with that and would be interested in hearing it.

@Priya91
Copy link
Contributor

Priya91 commented Dec 1, 2017

@Drawaes Currently we only take a server certificate from the user for sslstream as server, and the underlying stacks collect the chains from the respective stores. In case of openssl, it has it's own store, on windows everything is there in a single location. Do any of these native stacks provide knobs to also configure the cert chain? My question is by having the sni return chain information, what are we going to do with it?

@Drawaes
Copy link
Contributor Author

Drawaes commented Dec 1, 2017

OpenSsl has

int SSL_CTX_set0_chain(SSL_CTX *ctx, STACK_OF(X509) *sk);

@Priya91
Copy link
Contributor

Priya91 commented Dec 2, 2017

Ok sounds good, can you also confirm how it works with Schannel and SecureTransport..

@Drawaes
Copy link
Contributor Author

Drawaes commented Dec 2, 2017

I know nothing about osx, and if I get time I will look through the windows sdk, but again the documentation on MSDN for that is subpar... (like really old)

@Drawaes
Copy link
Contributor Author

Drawaes commented Dec 2, 2017

Here you go looks like OSX can take a single certificate or a chain with the "main" certificate being the first item in the array if you are sending a chain, it seems to imply that it will lookup the "store" only if the chain isn't provided (I assume that is the way its used today)

Setting the certificate or certificates is mandatory for server connections, but is optional for clients. Specifying a certificate for a client enables SSL client-side authentication. You must place in certRefs[0] a SecIdentityRef object that identifies the leaf certificate and its corresponding private key. Specifying a root certificate is optional; if it’s not specified, the root certificate that verifies the certificate chain specified here must be present in the system wide set of trusted anchor certificates.

https://developer.apple.com/documentation/security/1392400-sslsetcertificate

@Drawaes
Copy link
Contributor Author

Drawaes commented Dec 2, 2017

SChannel seems to take an array of certs as well, but it appears that each is a "separate" end certificate and its not for the "chain", but I can't be 100% sure.

But then the way you load the certificate into the temporary store, I would image you need to load the chain inbehind them. So it won't be as pretty as OSX or OpenSSL but it should be doable

@muratg
Copy link

muratg commented Mar 6, 2018

cc @shirhatti @halter73

@terrajobst
Copy link
Member

terrajobst commented Mar 27, 2018

Video

@krwq, please make sure that deciding on the cert via a callback is viable, i.e. you don't have to pass a mapping of host name to cert up front on any of the platforms we support with .NET Core. Otherwise, the API shape won't work.

When caller returns null certificate we will throw AuthenticationException

Alternative design would be that null means use the default certificate (SslServerAuthenticationOptions.ServerCertificate). That might be more convenient without forcing the delegate to allocate a closure.

@davidfowl @halter73, preferences?

@Tratcher
Copy link
Member

@terrajobst The current design works pretty well for ASP.NET, I've already submitted a PR aspnet/KestrelHttpServer#2425.

It's easy to explain that you set ServerCertificate XOR ServerCertificateSelector. I don't see a need for fallback behavior.

@krwq
Copy link
Member

krwq commented Mar 30, 2018

Fixed with dotnet/corefx#28278

@krwq krwq closed this as completed Mar 30, 2018
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@dotnet dotnet 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
api-approved API was approved in API review, it can be implemented area-System.Net.Security
Projects
None yet
Development

No branches or pull requests

9 participants