Needless requirement of tlsextradomain

[Kixunil]

Description

tlsextradomain shouldn't be needed if the certificate is explicitly pinned. The wallet already checks the certificate matches, so domain doesn't provide any value beside annoyance.

Expected Behavior

The wallet checks that the certificate provided in URI matches the one sent by the server and accepts it without other conditions.

Actual Behavior

Zap rejects certificates without tlsextradomain despite it knowing that the certificate matches.

Possible Fix

Remove unnecessary domain check, check certificate equality only.
Even better, instead of putting the whole certificate into the URI (and QR code), it could just use SHA256, which would be significantly shorter.

Steps to Reproduce

1. configure and launch lnd instance without tlsextradomian and tlsextraip
2. generate QR code and use it for connecting
3. see error
4. set tlsextradomain
5. delete tls.cert and tls.key
6. restart lnd
7. re-create QR code
8. connect and see it working

Context

I was trying to connect to a lnd node. I will also want to provide lndconnect QR code in the future using fairly complex process that may not know the domain name at the time of setting up lnd. This would lead to terrible complexity.

Your Environment

• Zap version: 0.3.0-beta
• Android version: 10
• Device: Google Pixel 3
• LND Version: 0.10.0-beta

[michaelWuensch]

Thanks for this issue.
First of all, does this mean you have solved the connection problem you had in #202?

This is an interesting find. I never explicitly check for tlsextradomain or tlsextraip.
Are you sure these values have to be there or was it just due to recreating the cert?
If this is actually true, then this happens somewhere while creating the certificate.

The way we do it right now is:

• We transmit the whole certificate in the QR code / Lnd connect string.
• We create a certificate using the data transmitted before using: CertificateFactory.getInstance("X.509").generateCertificate(certificateData);
• We then add this to the trusted certificates.

I am not an expert on certificates, but shouldn't this work without tlsextradomain and tlsextraip?
Wouldn't this mean we also need the tlsextradomain in the cert when we just verify the hash as this is something certificate internal?

Nevertheless I like your idea with just hashing the cert as those immensly dense QR codes are not good.

As I am neither an expert on certificates, nor have a lot of spare time right now,
feel free to make a PR that fetches the certificate and returns it as byte[] if you want to speed up the process :-)

[Kixunil]

Yes, I solved the problem in #202 (however, that issue should still be open).
Just regenerating the certificate didn't occur to me, but I doubt that'd help. Maybe I will try it a bit later. Still, as you say, hash is more convenient.

I am not an expert on certificates, but shouldn't this work without tlsextradomain and tlsextraip?

Maybe it's a bug/feature in some Android library? If feature, then I'm not sure what purpose it serves though. I don't see any security increase.

Wouldn't this mean we also need the tlsextradomain in the cert when we just verify the hash as this is something certificate internal?

I hope not. I'd expect that the process is to completely disable certificate verification and then implement custom one. (I did something very similar on macOS a long time ago so it could work in principle, if the library is same.)

I'm not an expert either, not even in development and this is not my top priority rn but I will try to help once I get to implementing the feature I need. Possibly within two months.

[michaelWuensch]

@mrfelton
Maybe you can shed some light into usage of tlsextradomain and tlsextraip.
Can it work without? If yes, would that be a bad idea regarding security?
What are your thoughts on just including a hash of the cert in the LND connect string?

[mrfelton]

tlsextradomain and tlsextraip need to be set in the lnd.conf of the node you are connecting to. With these set lnd will generate a tls certificate that is bounded to those domains/ips.

Clients can then connect to the node with this certificate alone.

If you try connecting to a node using a domain name or ip that is not set in tlsextra* your tls connection will fail.

Technically, you can bypass the need for these details to be in the cert or even the need to supply the cert to your tls client by telling your tls client to bypass tls validation and not reject self signed certificates, but it's a bad idea from a security perspective.

[Kixunil]

@mrfelton why would it be a bad idea if you also check that the certificate is the one supplied by the QR code (or hash)?

[michaelWuensch]

@Kixunil
I couldn't get the idea out of my head and actually made a test implementation of the certificate hashing and new trusting mechanism yesterday. It was a lot easier than i thought.
michaelWuensch/BitBanana@96ebf6c

Please note that this does not change anything considering the tlsextradomain or tlsextraip.
We are still using the certificate to create a secure connection and therefore are bound to those values. The only thing that has changed is the way we determine if we should trust that certificate.
It allows us to now only transmit a certificate hash in the lndconnect string which makes the QR-Code a lot less dense.

To make this happen though it has to be implemented for Desktop and iOS as well and the actual lndconnect has to be updated.

Thanks for your input!

[Kixunil]

That's really cool! Did you keep domain check intentionally or you didn't find out how to do it yet?

[Kixunil]

I'm currently thinking deep about integrating Zap into my project and requiring extratlsdomain will lead to a very unclean design. :(

Would you merge a PR removing the check if I figure out how to do it safely?

[michaelWuensch]

@Kixunil
Depending on what integrating zap into your project exactly means, forking and modifing the project yourself might be a solution. Have a look at the MIT License in this case.

We might merge a PR like that, if we reviewed it and have no concerns.
But I can't answer that question now without further research.

[Kixunil]

I mean, I want to add support for generating QR codes. The biggest issue is that tlsextradomain is settable by administrator and I don't want to overwrite such setting. I'd have to remove the ability or make it conditional, which could easily become confusing.

I'll try to look at it then. I will not dare to make a PR that would make it possible to do MITM or contain unreadable spaghetti, that you can be certain. :)

[Kixunil]

FYI, I needed to do a change similar to adding tlsextradomain for BTCPayServer, so I decided to support tlsextradomain too. I figured the setting can be used multiple times, so thankfully there's no conflict with admin configuration.

This means I'm de-prioritizing my PR to remove the requirement. I might still do it at some point, but probably not anytime soon. Thanks for all your support so far anyway!