[GeorgeTsagk]

Change Description

This PR adds a first-party macaroon caveat, protector <profile-name>, that
restricts which request fields may be set on the RPC methods covered by a
named, compiled-in profile. Permissions decide which methods a macaroon may
call; protector caveats add a finer layer inside those methods.

Motivation: delegating channel management to a third party (a liquidity or
rebalancing service, an operator's own tooling, an LSP agent) without also
handing over the ability to redirect channel funds. Today a macaroon that can
call OpenChannel can also set push_sat or close_address, and one that can
call CloseChannel can set delivery_address, so "may manage channels"
implies "may move value to an arbitrary destination".

Design

• The caveat carries only a profile name; the rules live in lnd code. That
  makes the guarantee upgradeable: a release can tighten a profile (for example
  when a new value-carrying field is added to a request) and every macaroon
  already issued benefits on upgrade.
• A released profile name is frozen: its rules may only ever be tightened,
  never loosened. New semantics require a new name (channel-management-v2).
• A macaroon naming a profile the validating node does not know is rejected as
  a whole, and an older lnd rejects the caveat as unrecognized. Both directions
  fail closed.
• Added at bake time (lncli bakemacaroon --protector) or appended offline to
  an existing macaroon (lncli constrainmacaroon --protector).

First profile: channel-management-v1

Guarantee: the covered methods cannot redirect value to a third party.

Method	Denied fields
OpenChannel, OpenChannelSync	push_sat, close_address, funding_shim
BatchOpenChannel	push_sat, close_address (per batched channel)
CloseChannel	delivery_address
UpdateChannelPolicy	none (reviewed, no redirection vector; the entry records the review and reserves the slot)

Note the open/close pairing: close_address sets the upfront shutdown script a
later cooperative close pays to, so denying delivery_address is only
meaningful if close_address is denied at open time too. Out of scope for v1:
fee-based value burn and peer restrictions.

Enforcement, and the two parts worth reviewer scrutiny

Enforcement is a dedicated pair of interceptors placed after (inside of) both
the macaroon and the middleware interceptors:

1. After the middleware interceptors, because a registered RPC middleware
   may replace the request message. Enforcing at validation time would judge
   the original request while the handler executes the replacement. The
   protector stream wrapper wraps the middleware stream wrapper for the same
   reason.
2. Outside the validator dispatch, so a registered ExternalValidator that
   knows nothing about protector caveats cannot be used to accept a macaroon
   and thereby skip the field rules. Because the credential is then resolved
   independently, ambiguity there fails closed: more than one macaroon
   metadata value, an unparseable macaroon, a malformed caveat or an unknown
   profile are all rejected.

OpenChannel and CloseChannel are server-streaming, so the request only
exists after stream open. Streams whose macaroon carries a covering profile get
RecvMsg wrapped, with profiles resolved once at wrap time; all other streams
pass through unwrapped, so there is no cost on normal traffic.

Rules fail closed by construction: a populated field not explicitly classified
as allowed, denied or nested is rejected, as is a request of the wrong proto
type or one that cannot be inspected on a covered method. Denials use
codes.PermissionDenied. A unit test additionally walks the proto descriptors
of every covered message and requires every field to be classified, so a field
added in a later release fails CI until someone vets it rather than silently
falling outside a deny-list.

Scoping

A profile constrains only the methods it lists; uncovered methods are
unaffected. Narrowing the callable set stays the job of permissions, which also
lets several profiles combine on one macaroon. In practice the caveat is meant
to be paired with uri: ops:

lncli bakemacaroon --protector channel-management-v1 --timeout 7776000 \
    uri:/lnrpc.Lightning/GetInfo uri:/lnrpc.Lightning/ListChannels \
    uri:/lnrpc.Lightning/ListPeers uri:/lnrpc.Lightning/ConnectPeer \
    uri:/lnrpc.Lightning/OpenChannel uri:/lnrpc.Lightning/OpenChannelSync \
    uri:/lnrpc.Lightning/BatchOpenChannel uri:/lnrpc.Lightning/CloseChannel \
    uri:/lnrpc.Lightning/UpdateChannelPolicy

With broad entity permissions instead, uncovered methods such as SendCoins
remain fully usable. This is stated in the flag help, docs/macaroons.md and
the release notes.

Compatibility

Additive: no proto or database changes, and no behavior change for macaroons
without a protector caveat. One narrowing worth flagging: on non-whitelisted
methods, a present macaroon metadata value must now contain exactly one hex
encoded, parseable macaroon. This closes a fail-open path where a lenient
external validator could accept a request whose credential enforcement could
not resolve. Documented on RegisterExternalValidator.

Follow-up, not in this PR: a listprotectors-style RPC so a profile's rules can
be read from a running node, since printmacaroon shows the name but not its
meaning.

Steps to Test

go test ./macaroons/ ./rpcperms/
go test -race ./macaroons/ ./rpcperms/
cd macaroons && go test -fuzz FuzzGetProtectorProfiles -fuzztime 60s -run '^$' .
make itest icase=protector_macaroon backend=btcd

The itest covers the deny matrix on unary and streaming methods, batch open,
unknown profiles, a control case with an unconstrained macaroon, a full
open/policy-update/coop-close lifecycle through a protected macaroon, and a
registered request-rewriting middleware asserting the replaced request is the
one judged.

Manually, against a regtest node with a funded wallet and a peer: bake as
above, then

# denied, with codes.PermissionDenied
lncli --macaroonpath=chanmgr.macaroon openchannel --node_key <peer> \
    --local_amt 5000000 --push_amt 2000000
lncli --macaroonpath=chanmgr.macaroon closechannel --funding_txid <txid> \
    --output_index 0 --delivery_addr <addr>

# allowed
lncli --macaroonpath=chanmgr.macaroon openchannel --node_key <peer> \
    --local_amt 5000000
lncli --macaroonpath=chanmgr.macaroon updatechanpolicy --base_fee_msat 1500 \
    --fee_rate_ppm 600 --time_lock_delta 80

I ran this on a local regtest, including a control showing the identical
--push_amt 2000000 request succeeds with an unconstrained macaroon and really
does move 2,000,000 sats to the peer, which is the transfer the caveat blocks.

Design questions for reviewers

1. Uncovered methods. This PR says a profile has no opinion on methods it
   does not cover, leaving method scoping to uri: ops. The alternative is a
   default of "deny any write-capable method the profile has not vetted",
   derived from the permission map: stricter and self-defending against an
   over-broad bake, at the cost of coupling profiles to the entity/action model.
   Happy to switch if preferred.
2. Caveat grammar. Name in the caveat with rules in code, versus encoding
   the field list in the caveat. I chose the former so profiles tighten on
   upgrade, at the cost of holders not being able to invent their own field
   restrictions offline.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
• There is a change description in the release notes.

[github-actions]

🔴 PR Severity: CRITICAL

file classification | 13 files | 2642 lines changed

🟠 High (2 files)

• macaroons/protector.go - new macaroon caveat protection logic (macaroons/*)
• macaroons/service.go - macaroon service changes (macaroons/*)

🟡 Medium (4 files)

• cmd/commands/cmd_macaroon.go - CLI command (cmd/*)
• config_builder.go - core config wiring, uncategorized Go file
• rpcperms/interceptor.go - RPC permission interceptor, uncategorized package
• rpcperms/protector.go - new RPC permission protector, uncategorized package

🟢 Low (7 files)

• docs/macaroons.md - documentation
• docs/release-notes/release-notes-0.22.0.md - release notes
• itest/list_on_test.go - test-only
• itest/lnd_protector_macaroon_test.go - test-only
• macaroons/fuzz_test.go - test-only
• macaroons/protector_test.go - test-only
• rpcperms/protector_test.go - test-only

Analysis

The highest individually-classified tier is HIGH, driven by macaroons/protector.go and macaroons/service.go (macaroons/* is an auth/security-sensitive package). However, the PR is bumped one level to CRITICAL because non-test, non-generated lines changed total 907 (well above the 500-line threshold): cmd_macaroon.go (35), config_builder.go (1), docs/macaroons.md (48), release notes (25), macaroons/protector.go (186), macaroons/service.go (6), rpcperms/interceptor.go (34), rpcperms/protector.go (572).

This PR introduces a substantial new macaroon/RPC-permission "protector" mechanism spanning both the macaroons and rpcperms packages, touching macaroon caveat validation and the RPC interceptor path. Given the security-sensitive nature (macaroon auth, RPC permission gating) and the size of the change, expert review is warranted.

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

[GeorgeTsagk]

Extra notes for reviewer context, from a security-focused pass over this
branch. It found no hole in the enforcement mechanism itself; two edges are
worth attention, both verified against the code:

• The guarantee rests entirely on a correct bake and fails silently when the
  bake is wrong: broad entity permissions route around the profile
  (onchain:write reaches SendCoins), and a bake including
  macaroon:generate lets the holder mint itself a caveat-free macaroon, since
  BakeMacaroon neither checks that requested permissions are a subset of the
  caller's nor carries the caller's caveats over. Pre-existing lnd behavior,
  not introduced here, but a total bypass of the guarantee.

• Coverage erodes as lnd adds methods rather than fields: the exhaustiveness
  test only forces classification of new fields on already covered messages,
  so I plan a test requiring any method whose request carries one of the denied
  field names to be either covered by the profile or explicitly recorded as
  reviewed.

[bufo24]

This is great 🙏, thanks for working on this. This PR should close my feature request here: #10336

[MPins]

Read the description and all five commits. Answers to your two design questions below; code-level notes will follow in a separate pass so this thread stays about the design.

Design question 1: uncovered methods

Short answer: I lean towards keeping this PR as it is, with a bake-time warning added later in the listprotectors follow-up. The gap you are worried about is real and larger than SendCoins, and the alternative is workable with a small refinement, so this is a live trade-off rather than a dead end. But the refinement carries a permanent coupling cost against a problem the warning largely addresses, which is what tips it for me.

The gap is real

With entity permissions, OpenChannel requires onchain:write and offchain:write, while SendCoins requires only onchain:write. So the minimum entity bake that makes the profile useful at all already grants SendCoins. That is not an over-broad bake, it is the smallest one.

Parsing MainRPCServerPermissions for onchain:write + offchain:write: 15 methods reachable, of which 10 are write methods the profile has not reviewed.

AbandonChannel        DeletePayment           SendCustomMessage
ChannelAcceptor       FundingStateStep        SendMany
DeleteAllPayments     RestoreChannelBackups   SendOnionMessage
SendCoins

Sub servers register more at runtime, so that is a floor. AbandonChannel and RestoreChannelBackups sit in there next to the fund-moving ones.

As stated, the rule breaks your own example

Deny any write-capable method the profile has not vetted, applied to the bake your description recommends:

Method	Covered by profile	Requires write	Rule says
OpenChannel	yes	yes	allow
ListChannels	no	no (offchain:read)	allow
SendCoins	no	yes (onchain:write)	deny, as intended
ConnectPeer	no	yes (peers:write)	deny, which breaks the bake

ConnectPeer is not optional: server.OpenChannel looks the peer up in peersByPub and fails with "peer %x is not online" if it is not already connected, and lncli openchannel --connect reaches that same RPC from the client rather than bypassing it. Since the rule is a runtime default the bake still succeeds, so the macaroon looks correct and is only denied ConnectPeer in use.

The refinement that fixes it

Treat an explicit uri: op as the operator having reviewed that method:

deny write-capable methods that are neither covered by the profile nor
granted by an explicit uri: op

Entity bakes then lose everything outside the profile (SendCoins has no uri: op, so it is denied, which is the protection you want), while the example bake keeps working because ConnectPeer is named explicitly. It is
implementable without the bakery: the ops decode straight out of the macaroon id via lnrpc.MacaroonId, which is what printmacaroon already does.

It would also close a hole that is currently unlisted in Limitations: write methods of gRPC services co-registered on the same server (Loop, Pool and Faraday under litd) would be denied unless explicitly named.

The trade-off that remains

The refinement removes the false positive but not the coupling you flagged, and I think the coupling is the real question:

• "Is this method write-capable" still needs the permission map, which is assembled at runtime from sub server registrations, so the answer can differ between builds of the same release.
• "A profile's rules may only ever be tightened" becomes harder to reason about, because what a profile denies would then depend on a map that lives outside the profile and changes independently of it.

That is a real cost and I do not think the answer is obvious. If you judge it too high, the cheap version is to keep the current behaviour and warn at bake time: when --protector is passed, list the reachable write methods the named
profiles have not reviewed.

You described this failure mode in your own notes as one that "fails silently when the bake is wrong". A warning does not stop a wrong bake, but it stops it being silent, which is the part that makes the current behavior uncomfortable
rather than merely permissive.

warning: these permissions also reach 10 write methods that
channel-management-v1 has not reviewed: AbandonChannel, ..., SendMany.
Scope with uri: ops to avoid this.

No taxonomy, no legitimate bake broken, and those ten names land in front of the operator when the macaroon is minted rather than after funds move. It is advisory either way, since constrainmacaroon is offline by design and can
never be checked.

That dependency also settles when it should land. lncli would have to know what a profile covers in order to report what it does not, which is exactly what the listprotectors follow-up provides, so the warning belongs in that same follow-up rather than here. Nothing in this PR needs to change for it.

One asymmetry in favour of the stricter default, for completeness: it needs nothing new at all, since enforcement already has both the profile table and the permission map to hand, whereas the warning waits on the follow-up.
Emitting the warning from BakeMacaroon instead would remove the wait, at the price of a proto change to carry the profile name.

Design question 2: caveat grammar

Agree with the choice, and I think it is the load-bearing decision of the whole design. A field list encoded in the caveat would freeze the guarantee at bake time, so every macaroon already handed out would keep exactly the holes it was
minted with. The name in the caveat is what makes tightening on upgrade possible at all.

One correction to the rationale rather than to the decision. You give two reasons for it: profiles tighten on upgrade, and printmacaroon output stays auditable. The second does not hold yet.

printmacaroon is fully offline, so it can only ever show the caveat string. The profile is documented in docs/macaroons.md, so a holder is not completely in the dark, but that is documentation for one release. What makes it
insufficient is the first reason itself: because the rules of a name can be tightened by any later release, what matters is what the node doing the validating enforces today, and there is currently no way to ask it that. The two halves of the rationale are in tension until something resolves a profile name against the running node.

That is the listprotectors-style RPC you already have in mind.

[GeorgeTsagk]

Thanks for the thorough pass, both answers make sense to me. Agree on keeping the current behavior and leaving the bake-time warning to the listprotectors follow-up, and the uri: op refinement is a good candidate for that discussion.

Pushed a docs-only update following your notes. The macaroon docs now state that write access must be granted through explicit uri: permissions and never entity write permissions, with the reason (the minimum entity bake for OpenChannel already reaches SendCoins and nine other unreviewed write methods), and include a real channel manager bake from a production deployment as the worked example. Limitations gained a bullet on services co-registered on the lnd gRPC server (Loop, Pool, Faraday under litd) being uncovered. Release note and the --protector help text carry the same guidance. Also dropped the printmacaroon auditability claim from the rationale, agreed that only holds once the running node can be asked.

[MPins]

Code-level pass, following on from the design questions answered in a separate thread.

Thanks for the docs update after that thread.

[MPins]

I've opened a separate issue for the 'invoices:read' point above: #11196

[MPins]

Thanks for the quick turnaround. I went through b9e7835f2 and everything I raised is addressed.

One optional nit:

• The invoices:read sentence holds for regular invoices. Hold and AMP invoices store no preimage, and this macaroon creates hold invoices through AddHoldInvoice, so "regular invoices" would be exact.

Unrelated to the code here I opened #11203 to refresh macaroons/README.md and left the protector bullet out on purpose so the two do not collide. Happy to add it there once this lands, or add it here if you prefer and I will rebase.

[GeorgeTsagk]

Rebased on master and took the regular invoices wording. For the README bullet, go ahead and add it in #11203 once this lands, keeps this one from growing.

[litbot-9000]

@ViktorT-11: review reminder
@calvinrzachman: review reminder

[litbot-9000]

@ViktorT-11: review reminder
@calvinrzachman: review reminder