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

[API Proposal]: Flag to disable TLS Resume/Session Caching in SslStream #78305

Closed
jborean93 opened this issue Nov 14, 2022 · 28 comments · Fixed by #86047
Closed

[API Proposal]: Flag to disable TLS Resume/Session Caching in SslStream #78305

jborean93 opened this issue Nov 14, 2022 · 28 comments · Fixed by #86047
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Security help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@jborean93
Copy link
Contributor

jborean93 commented Nov 14, 2022

Background and motivation

Currently an SslStream will reuse a session for the same target host which causes problems when you need distinct sessions for the same host. For example I've written a CredSSP authentication handler for Linux which uses an internal TLS stream to wrap the authentication tokens. This works ok when creating the first connection to a target host but if creating a second connection, say in a new thread, the server TLS implementation is unable to handle it properly causing an authentication failure.

I see there are currently three ways to avoid caching:

  • Use a unique hostname, this causes issues with validating the certificate CN
  • Use the env var DOTNET_SYSTEM_NET_SECURITY_DISABLETLSRESUME, this is process wide
  • Use the AppContext setting System.Net.Security.DisableTlsResume, also process wide

The last two options aren't ideal due to it being process wide and it could affect unrelated code. The value of this setting is also cached so unless it was set before any SslStream was authenticated, it won't apply anymore.

API Proposal

namespace System.Net.Security;

public class SslClientAuthenticationOptions
{
    public bool DisableTlsResume { get; set; } = false;
}

Add a bool property to SslClientAuthenticationOptions that the caller can opt into for any SslStream they are going to authenticate.

This bool can then be checked at AllocateSslHandle as part of the check whether to use the cached content

bool cacheSslContext = !DisableTlsResume && sslAuthenticationOptions.EncryptionPolicy == EncryptionPolicy.RequireEncryption && sslAuthenticationOptions.CipherSuitesPolicy == null;

API Usage

Stream stream = ...;

SslStream tls = new(stream)
SslClientAuthenticationOptions options = new()
{
    TargetHost = "target",
    DisableTlsResume = true,
};
tls.AuthenticateAsClient(options);

Alternative Designs

No response

Risks

There will most likely be a performance hit when disabling TLS resume but as this is an opt-in behaviour rather than the default it's up to the caller to justify whether this is needed.

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

ghost commented Nov 14, 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

Currently an SslStream will reuse a session for the same target host which causes problems when you need distinct sessions for the same host. For example I've written a CredSSP authentication handler for Linux which uses an internal TLS stream to wrap the authentication tokens. This works ok when creating the first connection to a target host but if creating a second connection, say in a new thread, the server TLS implementation is unable to handle it properly causing an authentication failure.

I see there are currently three ways to avoid caching:

  • Use a unique hostname, this causes issues with validating the certificate CN
  • Use the env var DOTNET_SYSTEM_NET_SECURITY_DISABLETLSRESUME, this is process wide
  • Use the AppContext setting System.Net.Security.DisableTlsResume, also process wide

The last two options aren't ideal due to it being process wide and it could affect unrelated code. The value of this setting is also cached so unless it was set before any SslStream was authenticated, it won't apply anymore.

API Proposal

namespace System.Net.Security;

public class SslClientAuthenticationOptions
{
    public bool DisableTlsResume { get; set; } = false;
}

Add a bool property to SslClientAuthenticationOptions that the caller can opt into for any SslStream they are going to authenticate.

API Usage

Stream stream = ...;

SslStream tls = new(stream)
SslClientAuthenticationOptions options = new()
{
    TargetHost = "target",
    DisableTlsResume = true,
};
tls.AuthenticateAsClient(options);

Alternative Designs

No response

Risks

There will most likely be a performance hit when disabling TLS resume but as this is an opt-in behaviour rather than the default it's up to the caller to justify whether this is needed.

Author: jborean93
Assignees: -
Labels:

api-suggestion, area-System.Net.Security

Milestone: -

@jborean93
Copy link
Contributor Author

jborean93 commented Nov 14, 2022

I do need to look at what Windows and macOS does here to see if they also do caching and how it would be possible to avoid it.

Still even if it’s OpenSSL only there is already precedent for the client options to be platform specific with CipherSuitesPolicy.

@rzikm
Copy link
Member

rzikm commented Nov 14, 2022

On Windows the TLS Resume is achieved by reusing the SafeFreeCredentials instance when calling the Win API. We should be able to avoid TLS Resume by simply bypassing the SslSessionCache and using fresh creds each time.

@jborean93
Copy link
Contributor Author

Thanks for confirming, I've done a test of my CredSSP code where this is a problem and I can confirm Linux and Windows is affected. The macOS interop doesn't seem to use TLS resume or at least didn't break the authentication like OpenSSL/Schannel does.

@wfurt
Copy link
Member

wfurt commented Nov 14, 2022

The proposal is reasonable to me. There may be way how to disable it also on Windows via API call or we may make it part of the cache lookup.

I would be also curious what there would be functional impact @jborean93. It is like this on Windows since ever and for Linux I see it mainly as performance boost then anything else.

@jborean93
Copy link
Contributor Author

I'm honestly not sure what the performance impact would be but as it would be opt-in behaviour it's really the caller who would need to decide if it's desired for their work. For me in the CredSSP scenario I have no choice and need it to never reuse a session.

@wfurt
Copy link
Member

wfurt commented Nov 14, 2022

So again why not @jborean93? It should make no functional difference. If it does, we should investigate IMHO.

@jborean93
Copy link
Contributor Author

The Schannel provider that CredSSP uses doesn't support TLS resumption. Each connection must have their own security context with their own session with wrapping and unwrapping. I unfortunately cannot control this as it's hidden deep within SSPI and the CredSSP dlls and how it manages the TLS connection.

@awakecoding
Copy link

@wfurt I've implemented CredSSP myself in FreeRDP years ago, and TLS session resumption is not supported, it really has to be disabled. It's a restriction imposed by the CredSSP protocol itself.

@awakecoding
Copy link

"TLS session resumption is not supported" from [MS-CSSP]:

image

@wfurt
Copy link
Member

wfurt commented Nov 14, 2022

I understand that it may not be supported. This should be all optional e.g. Client would offer it (or not) and Server has option to resume or not.

For Windows we already cache and resume the security context - regardless of the resume.

@jborean93
Copy link
Contributor Author

The trouble is the client doesn't have a way to offer it or not, it just does and CredSSP side stops sending tokens back. This proposal is to add a flag to dotnet to force it not to offer it.

@rzikm
Copy link
Member

rzikm commented Nov 15, 2022

That sounds almost like a possible bug in CredSSP, shouldn't it just ignore the resumption token and continue with a full handshake?

@rzikm
Copy link
Member

rzikm commented Nov 15, 2022

Triage: API looks reasonable and implementable. Should not be difficult to implement once API change is approved (are you interested in making a PR? @jborean93). Not critical for 8.0.

@rzikm rzikm added this to the Future milestone Nov 15, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Nov 15, 2022
@rzikm rzikm added untriaged New issue has not been triaged by the area owner api-ready-for-review API is ready for review, it is NOT ready for implementation help wanted [up-for-grabs] Good issue for external contributors and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Nov 15, 2022
@awakecoding
Copy link

That sounds almost like a possible bug in CredSSP, shouldn't it just ignore the resumption token and continue with a full handshake?

I can only speculate as to the reason why the Microsoft server implementations of CredSSP don't ignore session resumption from the client, but my guess is that since the same TLS listener can lead to non-CredSSP code paths, the TLS server doesn't know ahead of time if CredSSP is going to be used, and therefore can't pro-actively ignore TLS session resumption. It is therefore the responsibility of the CredSSP client to disable TLS session resumption for the protocol to work. This has been the case in the CredSSP server since the very beginning, and Microsoft will likely never change that, so we just have to live with it.

Even then, there's no reason not to expose ways of enabling/disabling specific TLS features in .NET, because I doubt CredSSP is the only protocol picky about specific TLS features advertised by the client.

@jborean93
Copy link
Contributor Author

@rzikm I can certainly try but I've never made one for dotnet before. I'll give it a shot and see how I go.

@wfurt
Copy link
Member

wfurt commented Nov 15, 2022

Just to be clear @jborean93. We need to wait for approval and follow https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md

@jborean93
Copy link
Contributor Author

jborean93 commented Nov 21, 2022

Just as another use case where session caching is breaking a real life scenario is with WinRM Client Authentication over TLS 1.3. It works fine for the first authentication step but when starting a new connection to the same host dotnet will attempt to reuse the session which causes some issues with the WinRM service attempting to map the certificate back to some identity. I'm less sure of whether this is a feature unsupported on the WinRM service end or some misconfiguration on the client but disabling caching allows this scenario to work.

This is interesting though as the code does have a check on whether client authentication is in use but for whatever reason the session is still being reused.

@jborean93
Copy link
Contributor Author

@wfurt is there a typical timeframe for something like this to be reviewed and approved/rejected? I've got a pretty ugly workaround with reflection to get what I need https://github.com/jborean93/PSWSMan/blob/897bda13a4436652fc33964d9ea937acadd01261/src/Authentication/Tls.cs#L16-L84 but was hoping to have an official API for future versions so I don't use the internals.

@wfurt
Copy link
Member

wfurt commented Dec 5, 2022

This looks reasonably small if We can verify that we can control it on Windows via SSPI API. I'm not convinced that the caching is 100% sufficient. (and on Linux you can set System.Net.Security.DisableTlsResume AppContext to avoid reflection)
Even if we do it now(ish) 8.0 will probably ship some time next year fall.

@jborean93
Copy link
Contributor Author

jborean93 commented Dec 5, 2022

This looks reasonably small if We can verify that we can control it on Windows via SSPI API

Wouldn't the fix for Windows to not reuse the same credential object if resume was explicitly disabled.

I'm not convinced that the caching is 100% sufficient

I'm not sure what else I can do to convince you. I've given you 2 different scenarios where caching explicitly fails. Both are for Microsoft services where I have no control over the code. Sure the service should support this scenario but it doesn't and I can't do anything about that from a client except disable resume for the scenarios I know don't work. I'm not sure if MS are even working on improving the CredSSP considering TLS 1.3 has been available on Windows since Win 11 and Server 2022 but CredSSP doesn't even support it.

(and on Linux you can set System.Net.Security.DisableTlsResume AppContext to avoid reflection)

This only works globally and must be set before any connection has been made. So you loose the benefits of TLS resume where it's possible plus it may not even be possible if you are running in a process where a TLS connection was already made before your code was used.

I'm fine with the timeframe and waiting for 8.0 as I have a workaround using reflection for existing versions. I just don't want to rely on that reflection code going forward and would prefer an official and stable way to disable TLS resume for a specific SslStream. I'm also find with creating a PR to do the work but you've mentioned I should hold off until the API gets approved so I'm trying to see what the general timeframes of that would be.

@wfurt
Copy link
Member

wfurt commented Dec 5, 2022

I feel using the cache makes it more likely to resume but I'm not convinced avoiding it fully prevents that. I think we should explore SCH_CRED_DISABLE_RECONNECTS.

BTW ON Linux you can also set certificate selection callback and return null. It is implementation details of 7.0 so maybe as reliable as the reflection to internals but it feels less intimidating. (as done via public surface)

@jborean93
Copy link
Contributor Author

jborean93 commented Dec 5, 2022

I feel using the cache makes it more likely to resume but I'm not convinced avoiding it fully prevents that. I think we should explore SCH_CRED_DISABLE_RECONNECTS.

At least in my testing it's always worked for me.

As for SCH_CRED_DISABLE_RECONNECTS, while the docs could be wrong it does state it's a server only feature so probably won't work for a client.

Server only. If this flag is set, then full handshakes performed with this credential will not allow reconnects. A cache entry is created, so the session can be made resumable later by using the ApplyControlToken function.

When testing it directly with SSPI it doesn't fail so that's at least encouraging. Will have to try it out a bit more with an actual server to see what happens.

$cred = Get-SCHCredential -Flags SCH_CRED_DISABLE_RECONNECTS
$ctx = New-SecContext -Credential $cred

$stepParams = @{
    Context = $ctx
    Target = "google.com"
    ContextReq = 'ISC_REQ_SEQUENCE_DETECT, ISC_REQ_REPLAY_DETECT, ISC_REQ_CONFIDENTIALITY, ISC_REQ_ALLOCATE_MEMORY, ISC_REQ_STREAM'
    OutputBuffer = 'SECBUFFER_EMPTY'
}
$res = Step-InitSecContext @stepParams
$res

BTW ON Linux you can also set certificate selection callback and return null

That would break the client certificate auth scenario over TLS 1.3 that also doesn't work with TLS session resume (with the WSMan service).

@wfurt
Copy link
Member

wfurt commented Dec 9, 2022

it may not be clear: It is not the null, it is presence of the callback. You can return what ever certificate you need.

@jborean93
Copy link
Contributor Author

Thanks I can confirm that setting the callback does at least disable it for OpenSSL but unfortunately for Windows it does not work. It is a bit annoying that I either have to have a dummy X509Certificate2 instance to return or use null against the null reference checks but it might be worth it to at least get rid of reflection for the Linux side.

I've also hooked the call to AcquireCredentialsHandleW to ensure SCH_CRED_DISABLE_RECONNECTS is set in the SCH_CREDENTIALS.dwFlags field but it doesn't seem to affect anything on the client. Even with this flag present, Schannel still seems to try and reuse a session so I don't think this is going to be a valid way forward for this API proposal. It seems like getting a fresh new credential from AcquireCredentialsHandleW unless there is some other flag that could help.

@wfurt wfurt self-assigned this Mar 16, 2023
@wfurt wfurt modified the milestones: Future, 8.0.0 Mar 17, 2023
@bartonjs
Copy link
Member

bartonjs commented Mar 23, 2023

Video

  • We feel that the server side is warranted, too.
  • We flipped the property from Disallow (default false) to Allow (default true)
namespace System.Net.Security;

public partial class SslClientAuthenticationOptions
{
    public bool AllowTlsResume { get; set; } = true;
}

public partial class SslServerAuthenticationOptions
{
    public bool AllowTlsResume { get; set; } = true;
}

@jborean93
Copy link
Contributor Author

Thanks @wfurt for implementing this, I'll try and see if I can test the changes at some point but looking at the PR it should be good

@wfurt
Copy link
Member

wfurt commented Jun 29, 2023

That would be great @jborean93. You can grab daily build https://github.com/dotnet/installer or wait for Preview 8.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 29, 2023
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 help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants