Skip to content
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.

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

Jump to bottom

rpcserver: forward gRPC proxy requests to localhost when listening on all interfaces #2247

Merged
merged 1 commit into from Jan 23, 2019
Merged

rpcserver: forward gRPC proxy requests to localhost when listening on all interfaces #2247

merged 1 commit into from Jan 23, 2019

Conversation

wpaulino
Copy link
Contributor

This prevents certificate issues when accessing the gRPC REST proxy externally.

Fixes #1567.

@Roasbeef Roasbeef added gRPC networking P3 might get fixed, nice to have needs review PR needs review by regular contributors needs testing PR hasn't yet been actively tested on testnet/mainnet labels Dec 3, 2018
@leshik
Copy link

leshik commented Dec 16, 2018

@wpaulino Just wanted to drop a note that I've patched my lnd instance with this PR and it works ok for me in Docker with wildcard RPC addresses. Hope that more people (@NicolasDorier @dennisreimann @zwarbo) try it so it could find its way to master.

@NicolasDorier
Copy link
Contributor

I don't understand, this PR seems to transform 0.0.0.0 to 127.0.0.1, this is not what I want.

@leshik
Copy link

leshik commented Dec 17, 2018 •
edited

@NicolasDorier It does this internally so the REST-to-gRPC proxy goes to the same host where LND is running (and since it's the same single binary, i.e. proxy and gRPC can't be on different hosts, it always works with 127.0.0.1). It doesn't bind REST endpoint to loopback. So it will fix the issue you were facing.

@NicolasDorier
Copy link
Contributor

Oh I get it! thanks a lot, will test.

@jamaljsr
Copy link
Contributor

I also ran into the "authentication handshake failed" error on my remote node, trying to setup the joule extension. I manually patched lnd with these changes, rebuilt and now the REST api works as expected. Thanks @wpaulino 👍

@Roasbeef Roasbeef added this to In progress in High Priority via automation Dec 18, 2018
@Roasbeef Roasbeef moved this from In progress to Needs review in High Priority Dec 18, 2018
@joostjager
Copy link
Collaborator

This issue also showed up in related PR #2428.

cfromknecht
cfromknecht previously approved these changes Jan 9, 2019
View reviewed changes
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM! 👻

halseth
halseth previously approved these changes Jan 16, 2019
View reviewed changes
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM :D

rpcserver.go
@@ -533,9 +533,17 @@ func (r *rpcServer) Start() error {
// TODO(roasbeef): eventually also allow the sub-servers to themselves
// have a REST proxy.
mux := proxy.NewServeMux()
grpcEndpoint := cfg.RPCListeners[0].String()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a comment why this string replacing is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

High Priority automation moved this from Needs review to Final Testing -- Ready For Merge Jan 16, 2019
@wpaulino
rpcserver: forward gRPC proxy requests to localhost when listening on…
552b71c 
… all interfaces

This prevents certificate issues when accessing the gRPC REST proxy externally.
@wpaulino wpaulino dismissed stale reviews from halseth and cfromknecht via 552b71c January 22, 2019 00:07
High Priority automation moved this from Final Testing -- Ready For Merge to Needs review Jan 22, 2019
High Priority automation moved this from Needs review to Final Testing -- Ready For Merge Jan 23, 2019
cfromknecht
cfromknecht approved these changes Jan 23, 2019
View reviewed changes
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM! 🦁

@wpaulino wpaulino merged commit 9860df6 into lightningnetwork:master Jan 23, 2019
High Priority automation moved this from Final Testing -- Ready For Merge to Done Jan 23, 2019
@wpaulino wpaulino deleted the grpc-proxy-endpoint branch January 23, 2019 00:21
@joostjager joostjager mentioned this pull request Jan 24, 2019
Merged
@wpaulino wpaulino mentioned this pull request Mar 19, 2019
Closed
@wereHamster wereHamster mentioned this pull request Mar 20, 2019
Closed
@wpaulino wpaulino mentioned this pull request Apr 4, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfromknecht cfromknecht cfromknecht approved these changes

@halseth halseth halseth approved these changes

Assignees
No one assigned
Labels
gRPC needs review PR needs review by regular contributors needs testing PR hasn't yet been actively tested on testnet/mainnet networking P3 might get fixed, nice to have
Projects
No open projects
High Priority
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@wpaulino @leshik @NicolasDorier @jamaljsr @joostjager @cfromknecht @halseth @Roasbeef