API: SSLStream Allow Configuration of CipherSuites

[krwq]

Forking off the main thread: https://github.com/dotnet/corefx/issues/24588 to stick to APIs

namespace System.Net.Security
{
    public sealed class CipherSuitesPolicy
    {
        public CipherSuitesPolicy(IEnumerable<TlsCipherSuite> allowedCipherSuites);
        public IEnumerable<TlsCipherSuite> AllowedCipherSuites { get; }
    }
    public class SslServerAuthenticationOptions
    {
        public CipherSuitesPolicy CipherSuitesPolicy { get; set; }
    }
    public class SslClientAuthenticationOptions
    {
        public CipherSuitesPolicy CipherSuitesPolicy { get; set; }
    }
}

Depends on: https://github.com/dotnet/corefx/issues/34867 (approved)

[davidsh]

Only disabling, because it's the only solution which can work on all platforms

How does one disable specific ciphers on Windows? Is it via an API? That would be necessary in order for this to work on Windows. And does it require admin permissions or not?

Inclusion of ciphers is usually via registry settings. So, I was wondering if this API is actually feasible on Windows.

[blowdart]

Enabling/Disabling is generally done with suites, combinations of ciphers, exchange and signature algorithms, rather than the component parts. The names of the suites are the same in OpenSSL and Windows. I wonder if it's better to stick to this, rather than let people believe that they can be treated as their component parts.

There also needs to be the ability to order suites, so the strongest are preferred first, falling back to the less desirable ones.

[bartonjs]

API Feedback:

• Unless you return an ISet that isn't a public type, someone will cast it the day you ship and changing the implementation is a runtime breaking change. So the API feedback will be "return the concrete type you are, unless you've nicely used an uncastable type".
• EcDsa => ECDsa
• The CipherAlgorithmType values will need to match what SChannel says they are, since the Windows PAL just blindly copies values out of SChannel. Something needs to be decided for "non-Windows" values (if they didn't reserve them).
• EcDhe => ECDiffieHellman (If the suffix "E" is to distinguish it from static ECDH, then presumably we'd need a DiffieHellmanEphemeral for the IFC DH version)

[krwq]

How does one disable specific ciphers on Windows? Is it via an API?

@davish, yes, Windows has added an API in RS5 for disabling ciphers (not ciphersuites), also older versions have something resembling that API. You have to use SCH_CREDENTIALS structure which takes list of TLS_PARAMETERS which restrict ciphers.

Enabling/Disabling is generally done with suites, combinations of ciphers, exchange and signature algorithms, rather than the component parts

@blowdart, I agree that would be ideal as it is the most flexible option - it is what the spec defines but API on Windows does not allow to do this - only disabling ciphers (almost exactly as this API proposal with couple of more options). We can later add platform specific API for disabling specific ciphersuites on Linux/OSX and throw PNSE on Windows. After giving it some thought before I think disabling ciphers is also good or better as you can express: "I do not trust RC4", "I do not trust ECDsa" rather than "I do not trust ECDsa but only if it is used along with ...."

There also needs to be the ability to order suites, so the strongest are preferred first, falling back to the less desirable ones.

Window currently does not support it. I think OpenSSL & OSX do. If we pick that only disabling is possible as in this proposal we do let OS choose for us.

return the concrete type you are, unless you've nicely used an uncastable type

I actually made it settable so I do not really care if someone does or does not cast it. I'd rather mention in the docs to not rely on a specific type returned by this API but if this is the practice we do everywhere else we can change it with i.e. HashSet or something else (or implement ISet as internal type and return that to avoid casting).

CipherAlgorithmType values will need to match what SChannel

I'll try to match them where possible, for other values as I've proposed we can choose something which will unlikely be used such as 0x70000000 and higher - we can choose any other arbitrary value

EcDhe => ECDiffieHellman (If the suffix "E" is to distinguish it from static ECDH, then presumably we'd need a DiffieHellmanEphemeral for the IFC DH version)

This was intentional, OpenSSL currently doesn't have any ECDH which are not ephermal (at least on the version I've checked) - I don't mind using the full name but initially I thought about EllipticCurveDiffieHellmanEphermal and that seemed a bit long - I guess using EC makes sense and shortens it significantly

[blowdart]

If the Windows API doesn't allow this then the Windows API is wrong. It's how it's done in the registry globally, it's how it's done in ngenix and apache, and Windows needs a slap. Can you map the suite strings to the underlying primitives instead for a consistent API in .NET?

[krwq]

@blowdart, I think it should be doable (specifying ciphersuites) but I still think some kind of filtering is preferable in most of the cases, I can't think of a case where you would want to disable ECDsa in one context but not in another - you either trust it or not.

I can see that specifying prefered order would have to be with full ciphersuites but for disabling I'm not convinced.

[blowdart]

I think it's more a case of an API being as expected, and no-one configures HTTPS on an algorithm alone, they're always combinations.

[krwq]

@blowdart I think the configuration with ciphers is less error prone and much easier but I can see how API which is consistent between different implementations of SslStream is useful but I do not think we should necessarily follow that convention.

[vcsjones]

Some thoughts.

1. How does one prioritize the curve on the key exchange curve when using ECDHE, or is that out-of-scope here? e.g. I want to prioritize X25519 before p256, for example. Should there be an ExchangeAlgorithmCurve that is an enumeration?

2. I agree that disallowing piecemeal algorithms is odd. An ordered allowlist seems much more preferable to a disallow list though. Ideally, there is a CipherSuite type that is just a wrapper around a ushort that contains the IANA value or something like that.

The names of the suites are the same in OpenSSL and Windows

Mostly, but they differ in exact formatting if it is decided that strings are accepted as input. e.g.

Windows: "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256"
OpenSSL: "ECDHE-ECDSA-AES128-GCM-SHA256"

No "TLS" prefix and "WITH" in OpenSSL, and different separators. If a string of suites are accepted, It'd have to be normalized for the right platform somehow.

[krwq]

1. @vcsjones it's a good point but unfortunately out of scope for now (good next step though)
2. If we are going to do that it will likely be enum with all the values (although our naming conventions don't quite work well with these names)

[mattzink]

Just supporting disabling, without the ability to order the remaining the ciphers, makes this unusable for any use case I can think of. I believe ordering must be a requirement.

[krwq]

Hey, I have updated suggestion based on feedback - please let me know what you think:

Changes:

• removed CipherSuitesPolicy.DisallowedSignatureAlgorithms (and enum) - it doesn't work well with TLS 1.3 ciphersuites and is possible to disable it with SslClientAuthenticationOptions.RemoteCertificateValidationCallback
  • Open question: TLS 1.3 spec allows to use DHE or ECDHE for key exchange, OpenSSL currently only supports ECDHE. How will disabling key exchange cipher work once there are more options?
• CiphersuitesPolicy => CipherSuitesPolicy (capitalization)
• Added: CipherSuitesPolicy.DisallowedCipherSuites - it will take an ushort so that platforms which support taking spec values are not restricted by our internal list but we will provide spec values as enum TlsCipherSuites : ushort
  • Notes: this will throw PNSE on Windows - see option below for detecting that
• Added: CipherSuitesPolicy.DisallowedCipherSuitesSupported - to be able to check if current platform supports disallowing by specific ciphersuite

Not added/changed yet:

• CipherSuitesPolicy.AllowedCipherSuites - this would allow for ordering and whitelisting
  • Open question: current issue is that TLS 1.3 and < TLS 1.3 ciphersuites must not be used together (https://tools.ietf.org/html/rfc8446#appendix-B.4) - OpenSSL does have two separate APIs for TLS1.3 and older. How would ordering work if you specify TLS1.3 and non-TLS1.3 ciphersuites? Should this be two separate APIs or you must specify TLS1.3 before older ciphersuites (throw exception when that's not the case)
• Enum values/names for ExchangeAlgorithmType and CipherAlgorithmType will change

[krwq]

@mattzink why do you think so? Do you not believe in OS ability to do it correctly? How would that work between TLS1.3 ciphersuites and older versions ciphersuites? (see below)

[krwq]

Another changelist:

• Renamed DisallowedCipherSuitesSupported => CipherSuitesControlSupported (name TBD, not sure how to call this)
• Added AllowedCipherSuites - this also allows ordering. If not set (== null) it defaults to OS defaults
  • as @bartonjs has pointed offline is that if client and server are both TLS1.3 the spec says that you should use TLS1.3 and thus effectively reorders the priority so that TLS1.3 ciphersuites are fist - this removes the problem of what to do when someone specifies i.e. TLS1.2 ciphersuites before TLS1.3 ciphersuites
• Fix typo: DisallowedCipherSuitesSupported was supposed to be an ISet

[vcsjones]

Ciphersuites generated from IANA

Looks good, but I hope there are plans to pare back some of the old things that should not be in there like Kerberos, NULL, anon, etc. If someone REALLY wanted to use bad suites and knew what they were doing they could cast a value to the enum like (CipherSuite)0 if they wanted to use NULL_NULL.

Along the same lines, it seems somewhat scary that the default value for the enum is also the least secure. e.g. if someone wrote this:

public void Foo(CipherSuite suite = default) => throw null;

I think most TLS implementations disallow this anyway, but it also makes a better case for not putting TLS_NULL_WITH_NULL_NULL in the enum at all and implementations can throw if the enum value's value 0 since there is zero reason to use it today.

[bartonjs]

@vcsjones 0 is a legal value for any enum, so CipherSuite suite = default would compile even if a zero-value isn't defined.

I think it's much easier to say that we'll regularly regenerate the list from the IANA registry, but making a policy around what to exclude gets weird (and is usually a thing we can only do once... making the exclusions look weird in 5 years).

[vcsjones]

@bartonjs

Yes that is why my rational was to throw if the enum is 0, too:

throw if the enum value's value 0 since there is zero reason to use it today.

Sorry if I wasn't clear, but what I am advocating for is the enum having no usable default. Don't define anything for zero and APIs should throw if they are given a 0 value, or default.

[krwq]

@vcsjones EncryptionPolicy controls the behavior for NULL ciphers so it won't get added by accident anyway. I'll make sure to respect that when you set AllowedCipherSuites

[krwq]

another change:

• DisallowedDigestAlgorithms -> DisallowedMacs

this is to disambiguate between MAC vs hash used for key derivation

[krwq]

To keep the thread updated - I have prototyped two solutions here:

• API proposed above
• Only allow list of cipher suites

My previous assumptions about disallow list of ciphers being better was broken in testing - really error prone from user and library perspective and comes with the future maintenance cost of having to know everything about every cipher suite. My current stance is that allow list of cipher suites is much less error prone and more intuitive and I believe that's what the design should look like.

[terrajobst]

Video

Going through the builder feels odd; especially because it doesn't itself represent a collection. It seems more natural to do something like:

public sealed class CipherSuitesPolicy
{
    public CipherSuitesPolicy(IEnumerable<TlsCipherSuite> allowedCipherSuites);

    public IEnumerable<TlsCipherSuite> AllowedCipherSuites { get; }
}

It was proposed to apply hide the AllowedCipherSuites property from other types; we don't believe that's the way to give guidance. Rather, we should document the API, provide relevant guidance developers are likely searching for (e.g. "How to properly configure SSL with TLS"). We should also invest in an analyzer.

[krwq]

Note: OpenSsl 1.1.1 or OSX is required. Windows is not supported at the moment.