Skip to content

tls_manager: let REST proxy skip tls cert verification #8437

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.

Sign up for GitHub

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

Merged
Roasbeef merged 1 commit into from
Jan 30, 2024
Merged

tls_manager: let REST proxy skip tls cert verification #8437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion docs/release-notes/release-notes-0.17.4.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@
nodes where the chain sync got lost because fetching of already pruned blocks
from our peers was not garbage collected when the request failed.


* Let the REST proxy [skip TLS
verification](https://github.com/lightningnetwork/lnd/pull/8437) when
connecting to the gRPC server to prevent invalid cert use when the ephemeral
cert (used with the `--tlsencryptkey` flag) expires.

# New Features
## Functional Enhancements
Expand Down
31 changes: 13 additions & 18 deletions tls_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,32 +131,27 @@ func (t *TLSManager) getConfig() ([]grpc.ServerOption, []grpc.DialOption,
// and override the TLS config's GetCertificate function.
cleanUp := t.setUpLetsEncrypt(&certData, tlsCfg)

// If we're using the ephemeral certificate, we need to use the
// ephemeral cert path.
certPath := t.cfg.TLSCertPath
if t.ephemeralCertPath != "" {
Copy link
Copy Markdown
Member

@Roasbeef Roasbeef Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@guggero guggero Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

certPath = t.ephemeralCertPath
}

// Now that we know that we have a certificate, let's generate the
// required config options.
restCreds, err := credentials.NewClientTLSFromFile(
certPath, "",
)
if err != nil {
return nil, nil, nil, nil, err
}

serverCreds := credentials.NewTLS(tlsCfg)
serverOpts := []grpc.ServerOption{grpc.Creds(serverCreds)}

// For our REST dial options, we'll still use TLS, but also increase
// the max message size that we'll decode to allow clients to hit
// endpoints which return more data such as the DescribeGraph call.
// For our REST dial options, we skip TLS verification, and we also
// increase the max message size that we'll decode to allow clients to
// hit endpoints which return more data such as the DescribeGraph call.
// We set this to 200MiB atm. Should be the same value as maxMsgRecvSize
// in cmd/lncli/main.go.
restDialOpts := []grpc.DialOption{
Copy link
Copy Markdown
Collaborator

@hieblmi hieblmi Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above should be changed to indicate that we skip tls for REST calls.

Copy link
Copy Markdown
Collaborator Author

@ellemouton ellemouton Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated! thanks :)

grpc.WithTransportCredentials(restCreds),
// We are forwarding the requests directly to the address of our
// own local listener. To not need to mess with the TLS
// 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
Copy link
Copy Markdown
Member

@Roasbeef Roasbeef Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@yyforyongyu yyforyongyu Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator

@guggero guggero Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// on startup so this should be quite safe.
grpc.WithTransportCredentials(credentials.NewTLS(
&tls.Config{InsecureSkipVerify: true},
)),
grpc.WithDefaultCallOptions(
grpc.MaxCallRecvMsgSize(lnrpc.MaxGrpcMsgSize),
),
Expand Down