firewall+faraday: merge ForwardingAbility feature branch#1328
firewall+faraday: merge ForwardingAbility feature branch#1328bitromortac wants to merge 12 commits into
ForwardingAbility feature branch#1328Conversation
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 integrates the Faraday Highlights
New Features🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. 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 introduces gRPC unary and stream interceptors to apply privacy mapping for non-LND sub-daemon requests (such as Faraday) within LNC sessions, and enables peer pubkey obfuscation for Faraday's ForwardingAbility endpoint. It also adds a hidden noprivacy flag to autopilot sessions, refactors the Faraday subserver, and updates various dependencies. Feedback on these changes includes: ensuring the default timeVariation is applied when zero as documented; validating PrivacyTimestampVariation in the development config to prevent negative durations and potential integer overflows; optimizing the checkers map allocation in the privacy mapper to reduce overhead; and updating the go.mod replace directives to point to official upstream repositories instead of a personal fork.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return &PrivacyMapper{ | ||
| db: newDB, | ||
| randIntn: randIntn, | ||
| sessionDB: sessionDB, | ||
| db: newDB, | ||
| randIntn: randIntn, | ||
| sessionDB: sessionDB, | ||
| timeVariation: timeVariation, | ||
| permsMgr: permsMgr, | ||
| } |
There was a problem hiding this comment.
The documentation for NewPrivacyMapper states that if timeVariation is zero, DefaultTimeVariation is used. However, the implementation does not apply this default value, leaving timeVariation as 0 if passed as such. This can disable timestamp obfuscation entirely when a zero value is provided.
| return &PrivacyMapper{ | |
| db: newDB, | |
| randIntn: randIntn, | |
| sessionDB: sessionDB, | |
| db: newDB, | |
| randIntn: randIntn, | |
| sessionDB: sessionDB, | |
| timeVariation: timeVariation, | |
| permsMgr: permsMgr, | |
| } | |
| if timeVariation == 0 { | |
| timeVariation = DefaultTimeVariation | |
| } | |
| return &PrivacyMapper{ | |
| db: newDB, | |
| randIntn: randIntn, | |
| sessionDB: sessionDB, | |
| timeVariation: timeVariation, | |
| permsMgr: permsMgr, | |
| } |
| if c.PrivacyTimestampVariation != 0 { | ||
| cfg.PrivacyTimestampVariation = c.PrivacyTimestampVariation | ||
| } |
There was a problem hiding this comment.
The PrivacyTimestampVariation override from the development configuration is applied directly without validation. If a negative duration is supplied, casting it to uint64 in the privacy mapper (e.g., uint64(p.timeVariation)) will cause an integer overflow, leading to extremely large intervals and incorrect obfuscation behavior. Consider validating that the duration is non-negative.
if c.PrivacyTimestampVariation != 0 {
if c.PrivacyTimestampVariation < 0 {
return fmt.Errorf("privacy-timestamp-variation cannot be negative")
}
cfg.PrivacyTimestampVariation = c.PrivacyTimestampVariation
}| flags session.PrivacyFlags) map[string]mid.RoundTripChecker { | ||
|
|
||
| return map[string]mid.RoundTripChecker{ | ||
| //nolint:ll | ||
| "/frdrpc.FaradayServer/ForwardingAbility": mid.NewResponseRewriter( | ||
| &frdrpc.ForwardingAbilityRequest{}, | ||
| &frdrpc.ForwardingAbilityResponse{}, | ||
| handleForwardingAbilityResponse(db, flags), | ||
| mid.PassThroughErrorHandler, | ||
| ), |
There was a problem hiding this comment.
The checkers method allocates a new map and instantiates all round-trip checkers (along with their request/response templates and handler closures) on every single request. This introduces unnecessary allocation overhead and garbage collection pressure. Consider refactoring this lookup to use a switch-case or a lazy-initialization helper that only instantiates the specific checker requested by the URI.
| replace github.com/lightninglabs/faraday => github.com/bitromortac/faraday v0.0.0-20260611104839-edc3a405e6ec | ||
|
|
||
| replace github.com/lightninglabs/faraday/frdrpc => github.com/bitromortac/faraday/frdrpc v0.0.0-20260611104839-edc3a405e6ec |
There was a problem hiding this comment.
The replace directives point to a personal fork (github.com/bitromortac/faraday). While useful during development, merging code to the main branch with dependencies pointing to personal forks can lead to build fragility and security risks if the fork is deleted or modified. Please ensure these are updated to point to the official upstream repository before merging.
c9df71e to
26b3b10
Compare
|
Rebased on master, the changes can be seen with
The changes are basically in the config, the release notes, and dropping some faraday mod update. This has the latest faraday version of lightninglabs/faraday#244, but we may still wait for faraday adding db cleanup routines before we merge this, which will require another version change. |
bitromortac
added
the
no-changelog
26b3b10 to
e20dbdb
Compare
|
Rebased on master, added a temp replace for lightninglabs/faraday#245, which will be removed, once merged. |
In this commit we make faraday a first class subserver instead of only incorporating its grpc subserver. The bitcoind config is already part of the faraday config.
This can be useful for testing.
This is useful to test non-obfuscated sessions.
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.
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.
Pubkeys in the Peers field are obfuscated. Other data is averaged over large timespans usually, so that doesn't lead to privacy leaks.
e20dbdb to
042baef
Compare
|
Updated to latest faraday. |
|
@bitromortac, remember to re-request review from reviewers when ready |
4 participants
This merges a feature branch where we add support for faraday's new
ForwardingAbilityendpoint.The merge conflicts will be resolved by a rebase on master in a later push.