firewall: add interceptor for non-LND calls - #1271
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the privacy architecture of Lightning Terminal (LiT) by extending privacy mapping capabilities to non-LND sub-daemon calls. By introducing a dedicated gRPC unary interceptor, the system now ensures that sensitive data in requests and responses for services like Faraday is correctly obfuscated before reaching the sub-daemon. The implementation follows a fail-close design pattern to maintain security integrity and includes comprehensive updates to the initialization flow and test suite. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a gRPC unary interceptor to apply privacy mapping for non-LND sub-daemon requests within LNC sessions, including documentation of the call flows and integration into the terminal startup logic. Feedback identifies a critical startup race condition that could cause a panic, performance inefficiencies due to redundant session database lookups, and a potential security risk where requests missing session metadata are allowed to pass through without privacy mapping.
| sessionID, err := extractSessionFromContext(ctx) | ||
| if err != nil { | ||
| log.Tracef("LNC privacy interceptor: no session for "+ | ||
| "%s, passing through", info.FullMethod) | ||
| return handler(ctx, req) | ||
| } |
There was a problem hiding this comment.
If the session ID cannot be extracted from the context, the interceptor currently logs a trace and passes the request through without any privacy mapping. If this interceptor is intended for a dedicated LNC listener, any request missing a session ID should likely be rejected (fail-close) to prevent potential privacy leaks or unauthorized access to sub-daemons with real identifiers. Allowing a pass-through here might bypass the privacy mapping layer if a client manages to send a request without the expected metadata.
There was a problem hiding this comment.
Generally this looks good, thanks 🔥! Worth looking into some of the review-bot comments though :).
Also: In the future, we'll likely want to make the accounts system be able to act on requests to other sub-systems than just lnd (specifically taproot-assets seems likely). I think a potential solution of how we would achieve that would be to create some a generic account interceptor layer that works on normal gRPC requests/responses, and not just lnrpc.RPCMiddlewareRequests. I think it's likely out of scope for this PR, but when we do so, it impact what's being added by this PR.
Just thought I'd add a comment regarding that, in case that in some way changes how you'd like to design this PR :).
Additionally, this PR should add a release note IMO.
eee3d03 to
6c8ca38
Compare
|
The unit-race test failure is independent of this PR, fixed in #1281. |
|
@bitromortac, remember to re-request review from reviewers when ready |
The checkAndReplaceIncomingRequest and replaceOutgoingResponse helpers each looked up the session from sessionDB by ID, resulting in two redundant GetSession round-trips per intercepted request. Move the lookup to the top of Intercept and pass the resolved *session.Session into both helpers so the session is fetched exactly once. This also makes the helpers reusable from callers that already have the session in hand, such as the unary interceptor introduced in the next commit.
6c8ca38 to
6f74420
Compare
The privacy mapper currently runs only inside LND's middleware chain via PrivacyMapper.Intercept, so requests that LNC routes directly to non-LND sub-daemons (e.g. Faraday) reach the sub-daemon with real channel IDs, peer pubkeys and amounts. Add a gRPC unary server interceptor on the LNC session server that applies the same pseudo-to-real request mapping and real-to-pseudo response mapping for these calls. LND URIs are skipped because LND's middleware already covers them. The interceptor is fail-close: it blocks the request if any dependency (permissions manager, session DB, privacy DB) is not yet initialized, if the call does not carry a session ID, or if no privacy checker is registered for the URI. A small PermissionsManager interface is introduced so the mapper can identify LND URIs without a hard dependency on the perms package.
The unary interceptor added in the previous commit leaves streaming RPCs unmapped, so a privacy-enabled LNC session can still open a stream against a non-LND sub-daemon (e.g. Faraday) and receive real channel IDs, peer pubkeys and amounts. Add the streaming counterpart with the same fail-close logic: LND URIs and sessions without privacy pass through; non-LND streams from privacy-enabled sessions are rejected with PermissionDenied until per-message stream rewriting is implemented.
Documents the dual-path privacy mapping architecture: LND requests are mapped via LND's middleware chain (PrivacyMapper.Intercept) after being proxied to LND, while non-LND sub-daemon requests are mapped at the LNC gRPC interceptor (PrivacyMapper.UnaryInterceptor) before reaching the sub-daemon. Each flow is illustrated with a mermaid sequence diagram.
6f74420 to
a75fd7a
Compare
| // Fail-close: non-LND streaming RPCs are not yet supported | ||
| // by the privacy mapper. Block them to prevent cleartext | ||
| // leaks to LNC sessions with privacy enabled. | ||
| log.WarnS(ctx, "Blocking non-LND stream: privacy mapping "+ | ||
| "not supported for streams", nil, | ||
| "method", info.FullMethod, | ||
| "session_id", sess.ID) | ||
|
|
||
| return status.Errorf(codes.PermissionDenied, | ||
| "streaming RPCs for non-LND services are not "+ | ||
| "supported with privacy mapping") | ||
| } |
There was a problem hiding this comment.
Just wanted to check that you've made sure that this can't cause any issues with tapd (and potentially loop) if we merge this as is?
There was a problem hiding this comment.
I tested the following:
- loop out via TW session
- local unary frcli call
- local streaming loop call (monitor)
- payment using accounts via a session
So I'd consider this well tested and will merge.
| if p.permsMgr == nil { | ||
| return false, fmt.Errorf("permissions manager not initialized") | ||
| } | ||
| return p.permsMgr.IsSubServerURI(subservers.LND, uri), nil |
There was a problem hiding this comment.
nit: new line before return.
There was a problem hiding this comment.
will skip due to CI run
c9a5dbb
into
lightninglabs:faraday-forwarding-ability
firewall: add interceptor for non-LND calls
5 participants
Adds a gRPC unary interceptor to LNC sessions that applies privacy mapping to non-LND sub-daemon calls (e.g. Faraday). LND URIs are skipped since they're already handled by LND's middleware chain.
I added a diagram to see how the two flows work for lnd/subdaemon calls:
https://github.com/bitromortac/lightning-terminal/blob/eee3d033fff0f808ec5aa3119721fb5d3fd95c9c/docs/privacy-mapping-flows.md
If the design is ok, I can also use a streaming interceptor.