tls_manager: let REST proxy skip tls cert verification#8437
Conversation
|
Important Auto Review SkippedAuto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
ellemouton
force-pushed
the
v0-17-3-tls-fix
branch
from
ec72434 to
c94479f
Compare
ellemouton
force-pushed
the
v0-17-3-tls-fix
branch
from
c94479f to
e0421e7
Compare
There was a problem hiding this comment.
The comment above should be changed to indicate that we skip tls for REST calls.
There was a problem hiding this comment.
updated! thanks :)
ellemouton
force-pushed
the
v0-17-3-tls-fix
branch
from
e0421e7 to
404a50a
Compare
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
This flag results
in an ephemeral tls key&cert being used which is swapped out with the permanent tls key/cert once LND is unlocked. After a day, the ephemeral cert expires but the client continues trying to use that key.
IIUC, isn't the solution here just that the client should get the updated cert to communicate with the node? Certs eventually expire, which is a fact of life, but the clients should be able to get the updated cert to communicate with the node.
| // certificate (which might be tricky if we're using Let's | ||
| // Encrypt or if the ephemeral tls cert is being used), we just | ||
| // skip the certificate verification. Injecting a malicious | ||
| // hostname into the listener address will result in an error |
There was a problem hiding this comment.
What do you mean by injecting a malicious host name?
I think the attack scenario here is a MiTM (on the same machine), that is then used to obtain the macaroon directly from a request (previously not written to disk), and then can use that to issue direct commands.
There was a problem hiding this comment.
I'm also curious about the implication for a remote signer, I think the signer node needs to open this port for the watch only node?
There was a problem hiding this comment.
The REST proxy connects to the host/port configured by -rpclisten. So if you supply anything controlled by an attacker (on the same machine or remote), then lnd will attempt to listen on that host/port. And if it can't (because an attacker is already listening), then it would shut down before the REST proxy is even spun up. Or am I missing a scenario where you could inject a value that would not fail on startup?
|
Perhaps an alternative solution is: have the REST proxy ensure that it's using the long-lived cert when it goes to make that initial connection. IIUC, as is, this swap over can happen while |
| // If we're using the ephemeral certificate, we need to use the | ||
| // ephemeral cert path. | ||
| certPath := t.cfg.TLSCertPath | ||
| if t.ephemeralCertPath != "" { |
There was a problem hiding this comment.
Based on the reported scenario, does the issue happen while lnd has been running the entire time (> 24 hrs) after the first unlock, or after? If it's the latter, then we can also insert a check here re if the fresh/new cert is already in place, then we can use that.
There was a problem hiding this comment.
The proper fix here would indeed be to re-initialize the whole REST server once the long-term certificate is available. But because the call sites are quite in different places, it would mean a big refactor and cleanup. Which we would also want to cover with unit/integration tests. So not something we want to do under time pressure (which is IMO also how the original PR was merged in a state with several bugs).
6 participants
Similarly to what is done in LiT, we let the REST proxy skip TLS
verification when connecting to the gRPC server. This is necessary to fix this bug that appears in LiT that happens when the
--tlsencryptkeyflag is used. This flag resultsin an ephemeral tls key&cert being used which is swapped out with the permanent tls key/cert once LND is unlocked. After a day, the ephemeral cert expires but the client continues trying to use that key.
Since this is just LND connecting to itself, it should be fine to skip TLS verification here.