akkadotnet / akka.net

Canonical actor model implementation for .NET with local + distributed actors in C# and F#.
http://getakka.net
Other
4.69k stars 1.04k forks source link

Certificate validation doesn't work as expected if one party suppresses validation #5738

Open jonnydee opened 2 years ago

jonnydee commented 2 years ago

Version Information

Bug Description

I figured out that if two actor systems don't suppress certificate validation on each side they can only communicate with certificates derived from a common root CA trusted by the operating system (which is fine). However, if one party (let's call it Alice) has certificate validation enabled while another one (let's call it Eve) has certificate validation suppressed then Eve can connect to Alice using an arbitrary SSL certificate (i.e. issued by another root CA) and send messages to Alice (which apparently is a bug).

Steps to Reproduce

  1. Run an Actor System instance (Alice) with an SSL certificate configured and make sure certificate validation is NOT suppressed.
  2. Run another Actor System instance (Eve) configured with an SSL certificate issued by a different root CA then the one used for Alice's SSL certificate and make sure certificate validation is suppressed.
  3. Let an actor running within Actor System Eve send a message to Actor System Alice using Akka.Remote.

(In order to verify Eve cannot send a message to Alice just repeat the steps but enable certificate validation also for Eve and make sure it uses a certificate issued by a different root CA.)

Expected behavior

Eve cannot establish a connection to Alice and is not able to send a message to Alice because Alice has certificate validation enabled and Eve doesn't use a certificate issued by a common root CA. (Just as it is the case if both Alice and(!) Eve have certificate validation enabled.)

Actual behavior

Eve can establish a connection to Alice using an arbitrary certificate as soon as Eve suppresses certificate validation within its own configuration no matter whether Alice does or does not validate certificates.

Environment

Additional context

Maybe I should clarify why it is important for us that certificate validation enabled within Actor System Alice should always reject connections from Eve if Eve doesn't use a certificate from the root CA which issued the certificate for Alice -- no matter whether Eve has certificate validation suppressed or not.

We want to make sure other Actor Systems can only establish communication connections (and send messages) to Alice if they use a trusted certificate. Note that "trusted" in this context doesn't mean it has to just be trusted by the operating system but really trusted by Alice. And one way to accomplish this goal is to allow connections only if the other party uses a certificate issued by the same root CA as Alice does. So, the way it works in case both parties have certificate validation enabled is exactly what we would need. Communication is only possible if both parties use a certificate issued by a common root CA. If it worked always like this we could make sure that an attacker Eve couldn't communicate with Alice without access to a certificate issued by the root CA which issued Alice's certificate.

The way it works right now allows Eve to just suppress certificate validation in its own configuration and then send possibly malicious messages to Alice even though Alice is configured to NOT suppress certificate validation.
In addition, it's a bit strange that one party can decrease overall communication security by just suppressing certificate validation on its own side when the other side actually does verify certificates. Instead, the party with the highest security measures configured should dictate what other parties need to fulfill in order to allow them to successfully establish a communication channel.

eaba commented 2 years ago

Hello @jonnydee, this has been investigated by us and it is not something we are responsible for! This issue belongs to the DotNetty repo.

Aaronontheweb commented 2 years ago

That's correct - we don't have any control over how cert validation is handled, although I think when you suppress validation that does mean everything.

jonnydee commented 2 years ago

@eaba @Aaronontheweb Actually, I disagree that it is DotNetty's "problem" only. I had a look at the Akka.Remote's DotNettyTransport class which creates server and client channels and configures corresponding pipelines.

The server channel's pipeline is configured in the private method SetServerPipeline(IChannel) where a DotNetty TlsHandler is created with TlsHandler.Server(Settings.Ssl.Certificate) -- a static factory method which just creates a TlsHandlerinstance using the public constructor TlsHandler(TlsSettings settings). This constructor in turn just delegates to another public constructor TlsHandler(Func<Stream, SslStream> sslStreamFactory, TlsSettings settings) but passes a lambda as sslStreamFactory which calls the simple SslStream(Stream, Boolean) overload. However, there are other more advanced constructor overloads that could be used instead (e.g. SslStream(Stream, Boolean, RemoteCertificateValidationCallback). And because everything is public API class DotNettyTransport could also use them directly instead of just using the simple TlsHandler.Server(Settings.Ssl.Certificate) factory method.

For the client channel's pipeline the situation is similar.

So I don't think this bug should just be closed here, because other public DotNetty / .NET APIs could have been used to configure the client and server pipelines in a smarter way.

eaba commented 2 years ago

Can you create a PR for this?

jonnydee commented 2 years ago

I'm sorry, I don't have the time to do this in the near future. But would that be the criteria on which to decide if this is a "won't fix"?

If so, can't we just leave it open (assumed we can agree on this being a bug) until someone (maybe me sometime in the future) has time to give it a try? (I know that carrying around open issues may hurt in one way or the other but if issues are closed because nobody has time or expertise to do it than people that do won't have a chance to spot them.)

Aaronontheweb commented 2 years ago

Got it. I've reopened the issue and marked it as "up for grabs"

Our team does a ton of work on issues reported by end-users without any compensation, but we don't have the resources to handle everything - consider purchasing a support plan https://petabridge.com/services/support/ if you want us to prioritize work on this. Or feel free to submit a PR as @eaba said - we'd be happy to review it or provide design feedback before you start.

jonnydee commented 2 years ago

That's great, thanks :-)

I fully understand that and it's also absolutely, perfectly OK to prioritize things one actually gets paid for. Unfortunately, consideration to purchase a support plan currently is not an option at all. I'm an Akka.NET advocator, however, and when I find time I'll give this issue a try. Nice to hear I might count on some design feedback :-)

ismaelhamed commented 2 years ago

Maybe we could do the same for the server pipeline:

var tlsHandler = Settings.Ssl.SuppressValidation
    ? TlsHandler.Server(Settings.Ssl.Certificate)
    : new TlsHandler(new ServerTlsSettings(Settings.Ssl.Certificate, negotiateClientCertificate: true));

At this point I would also rename suppress-validation to require-mutual-authentication like in the JVM.

jonnydee commented 2 years ago

I also had this idea but I haven't tried yet. Do you happen to know if this solves the issue?