rules+firewall: rules and privacy mappings for channel creation #752

bitromortac · 2024-04-26T10:03:30Z

Based on #746

This PR adds functionality to litd in order to make automatic channel opens possible.

We add two new rules:

ChannelConstraint: to limit channel sizes
OnChainBudget: to limit onchain interactions with a per session budget
update the peer restriction rule to be active for channel opening endpoints

Automatic channel creation involves the following additional endpoints:

WalletBalance: check whether enough funds are available
PendingChannels: check if a channel is already pending with a node
ClosedChannels: for peer sanity checks
ConnectPeer: connect to a peer before opening
BatchOpenChannel: the request for opening

firewall/privacy_mapper.go

ellemouton

First pass - looking soooo good 🔥 very clean.

Gonna need a second pass to just really get into the budget logic.

Main suggestion on first pass is to perhaps add s "private only" restriction to open-chan constraints?

rules/onchain_budget.go

firewall/privacy_mapper.go

firewall/privacy_mapper_test.go

session/privacy_flags.go

firewall/privacy_mapper.go

rules/channel_constraints.go

ellemouton

Really great work! Pretty much g2g I think. One main question though:

How does session linking worth with the on-chain budget? Does the same budget carry over & do we prevent them from providing a new one? or do we update the budget when a session is linked?

firewall/privacy_mapper_test.go

firewall/privacy_mapper.go

rules/peer_restrictions_test.go

ellemouton · 2024-05-16T10:13:20Z

rules/channel_constraints.go

+	return &ChannelConstraint{
+		MinCapacitySat: channelBounds.MinCapacitySat,
+		MaxCapacitySat: channelBounds.MaxCapacitySat,
+		MaxPushSat:     channelBounds.MaxPushSat,


missing the new public/private allowed?

i think both are false here meaning no channels allowed?

should we make the default (false) => allowed & then rather have "NoPublic"/"NoPrivate"?

does a call to OpenChan work with this as is given that both are false here?

does a call to OpenChan work with this as is given that both are false here?

I think UnmarshalRuleValues is only used for sanity checking upon session registration, but the actual used final rule values are the ones from the server, so even if those are false, channel openings work.

not super sure what you mean?

we get the litrpc.RuleValue here and convert it to ChannelConstraint which we then use in AddAutopilotSession to determine the rules (caveats) to set in the macaroon for the session.

so am not sure what you mean by "the final rule values are the ones from the server"?

isnt that bad then? that if the user was to set both to false, that channel opens would still work?

we get the litrpc.RuleValue here and convert it to ChannelConstraint which we then use in AddAutopilotSession to determine the rules (caveats) to set in the macaroon for the session.

thanks for the clarification! I think I misunderstood the flow of the rules, was confused by the default values provided by the autopilot if no rules are requested. Registering with both values false will not be allowed here https://github.com/lightninglabs/lightning-terminal/blob/master/session_rpcserver.go#L1074, so I think it works as intended

bitromortac · 2024-05-16T15:37:00Z

Really great work! Pretty much g2g I think. One main question though:

How does session linking worth with the on-chain budget? Does the same budget carry over & do we prevent them from providing a new one? or do we update the budget when a session is linked?

Thank you for the reviews 🙏! It's possible to change the budget, it's used as a mechanism to top up funds for a session, so we don't prevent it. We could enforce it that it's only allowed to be increased though.

ViktorTigerstrom

Really nice PR 🔥🚀!! Just like Elle says, I think this is pretty much ready to go, so great work 🎉!

I mainly added some test coverage request comments, and also commented one potential issue for the onchain_budget handling. Let me know what you think about that potential issue. But otherwise this looks great 🔥!

ViktorTigerstrom · 2024-05-22T16:29:15Z

rules/onchain_budget.go

+			handleOpenChannelRequest,
+			func(ctx context.Context,
+				r *lnrpc.ChannelPoint) (proto.Message, error) {
+
+				return nil, o.handlePaymentConfirmed(ctx)
+			},
+			handleOpenChannelError,


I'm not sure that this is something we actually can/want to handle, but I thought I'd just point to a potential issue here with using the temp store in between requests and their responses, that could potentially allow on-chain spending above the rule bounds:

Since the temp store is cleared on every restart of litd, we MUST process the response for every successful request in order for it to actually be saved permanently and therefore also be used in the calculation for the current spent on-chain budget after litd is restarted.

Now, if either:

litd is shutdown after a successful request has been sent, but before we've had time to process the response

The processing of the response errors (causing a shutdown)

The spent on-chain amount will be lost and will not be used for the future on-chain budget calculations once litd is restarted.

Like I said though, I'm not sure if these edge cases is something we should address, but I just to make you aware of it.

Yes, that is (a minor) issue. I'm not sure how that would be solved, because one would need a message buffer on lnd that would persist them in order for litd to be able to ack them? So unless we don't find an easier solution, I'd suggest to keep it this way.

Yeah, I agree! Just thought it be worth pointing out.

this is a good point and I do think we should consider handling it. In the accounts system, we are "restart" proof so probably worth trying to be restart proof here too if possible.

for the accounts system, we can do this quite easily cause we know the payment hash to track before sending the request through, so then on startup, we call LND to see if the payment with that hash did in fact go through.

So is there any way here that we can force there to be some identifier? For example, I see that OpenChannelRequest has an optional Memo string field and same for `BatchOpenChannel. Can we not force the caller to set these (to something unique that we have not seen before) and then on startup, can we then not list channels & pending channels and then subscribe to channel open events to continue tracking these? For example I see that the Channel open event does contain the memo field.

If we do this then we also would not use the temp store. Instead we would have some kind of permanent "memo -> amount" map that we can check on startup.

thoughts?

I explored the idea of using the memo to keep track of pending channel opens (see commits after the itest). A missing piece is to add the memo field to the ClosedChannels api. With that we could then delete any pending actions for which we don't find an outcome (so that must have either been some internal failure to open the channel or a channel open was abandoned). In my opinion, the complexity is not really worth it. I think we should not use the temporary store, but persist all pending actions and remove them once we get a response/error. This way we are conservative and don't allow overspending. In the worst case this leads to a decreased budget, which may lead to UX issues, but I would consider a session to be something ephemeral and a possible problem can be fixed by just creating a new (non-linked) one.

In my opinion, the complexity is not really worth it. I think we should not use the temporary store, but persist all pending actions and remove them once we get a response/error

agreed. but we should have some identifier persisted with them that is restart safe (ie, not the request ID as used today). We want to make sure that whatever we do now we can potentially recover from in future.

but I would consider a session to be something ephemeral and a possible problem can be fixed by just creating a new (non-linked) one.

ah yes that is a good point!

rules/onchain_budget_test.go

rules/channel_constraints_test.go

rules/peer_restrictions_test.go

rules/chan_policy_bounds_test.go

firewall/privacy_mapper_test.go

firewall/privacy_mapper.go

ViktorTigerstrom

LGTM, great work 🔥🚀🔥!

Question, do we want to merge this before deciding to marge automatic channel opens, in case we'd find out that more functionality is needed?

rules/channel_constraints_test.go

ellemouton

we definitely need itest coverage here. I think there are a few things slipping through the cracks that would be picked up via an itest.

Namely:

a OpenChannel request wont currently make it through at all since the rule enforcer doesnt currently allow streaming calls. ALso - calls to OpenChannel currently look like they succeed from the PoV of autopilot when they actually do not.
The privacy mapper doesnt currently have support for the OpenChannel and OpenChannelSync calls.

I put together a rough itest demonstrating these things:
itest.patch

rules/onchain_budget.go

ellemouton · 2024-05-29T16:00:16Z

rules/channel_constraints.go

+	return &ChannelConstraint{
+		MinCapacitySat: channelBounds.MinCapacitySat,
+		MaxCapacitySat: channelBounds.MaxCapacitySat,
+		MaxPushSat:     channelBounds.MaxPushSat,


not super sure what you mean?

we get the litrpc.RuleValue here and convert it to ChannelConstraint which we then use in AddAutopilotSession to determine the rules (caveats) to set in the macaroon for the session.

so am not sure what you mean by "the final rule values are the ones from the server"?

ellemouton · 2024-05-29T16:01:57Z

rules/channel_constraints.go

+	return &ChannelConstraint{
+		MinCapacitySat: channelBounds.MinCapacitySat,
+		MaxCapacitySat: channelBounds.MaxCapacitySat,
+		MaxPushSat:     channelBounds.MaxPushSat,


isnt that bad then? that if the user was to set both to false, that channel opens would still work?

ellemouton · 2024-05-29T16:02:35Z

rules/channel_constraints.go

@@ -0,0 +1,354 @@
+package rules


we need an itest for this too

rules/channel_constraints.go

ellemouton · 2024-05-29T18:35:36Z

rules/channel_constraints_test.go

+
+// TestChannelConstraintCheckers ensures that the ChannelConstraint values
+// correctly accepts or denies a request.
+func TestChannelConstraintCheckers(t *testing.T) {


note that none of these tests cover the OpenChannel call (only the sync one) and since OpenChannel is a stream call, it currently just lets any call through....

We either need to start intercepting stream calls or we should remove the OpenChannel handling & explicitly dissallow it.

currently im also finding that these open chan calls (except for BatchOpenChannel) are not supported by the privacy mapper. So they can only work if the "no privacy" option is set on the session (ie, privacy flags dont matter here)

Thank you for testing this 🙏. Right, the OpenChannel and OpenChannelSync calls were not fully implemented and tested. I removed OpenChannel (and disallowed any streaming requests), but added the privacy functionality for OpenChannelSync.

bitromortac · 2024-05-31T14:11:03Z

Thanks a lot for the suggestions and the itest @ellemouton 🙏 💯.

I removed the OpenChannel streaming RPC functionality, which was not fully built out and added privacy mapping for OpenChannelSync.

During itests I found that there was an issue when combining other rules with the budget rule, which is stateful. It may happen that a request adds a temporary budget reservation while another rule is violated, resulting in the reserved budget not being removed. I tried to fix this by adding a RollbackRequest method to the Enforcer interface, which is called after any rule is violated for a request. In principle we would need to add something similar to commit a balance reservation after knowing that all response rules were followed. We don't have restrictions on responses right now which is why I didn't add that.

rules/onchain_budget.go

lightninglabs-deploy · 2024-06-07T15:02:08Z

@ellemouton: review reminder
@bitromortac, remember to re-request review from reviewers when ready

ellemouton

Thanks for adding the itest 🙏 definitely improves the confidence here!

Two main comments from me on the latest review

ellemouton · 2024-06-10T14:43:30Z

rules/interfaces.go

+
+	// RollbackRequest reverts the enforcer's state after errors.
+	RollbackRequest(ctx context.Context, uri string) error
 }


why do we need this if we have HandleErrorResponse?

ooooh i see: this is for when the request doesnt get through all the enforcers and errors on one later down the stack.

The only issue I see here is: what about the case when it isnt another rule interceptor that errors on the request but instead another interceptor (like: accounts system).

So im wondering if we dont instead need take a more general approach here so that we can trigger the other possibly stateful interceptors to also roll-back.

Ie, if we encounter an error on the request, can we somehow turn that into an error response instead so that layers above can know to roll-back too?

Ie, if we encounter an error on the request, can we somehow turn that into an error response instead so that layers above can know to roll-back too?

I think that interceptors handle sequentially. So if a validation error happens in the rule enforcer, the error is converted to a RPCErr. Does this error then propagate back through the other interceptors that were already passed? If so, then I think everything works as expected and each stateful interceptor should handle a failure case its own way

firewall/rule_enforcer.go

ellemouton · 2024-06-10T15:07:56Z

rules/onchain_budget.go

+			handleOpenChannelRequest,
+			func(ctx context.Context,
+				r *lnrpc.ChannelPoint) (proto.Message, error) {
+
+				return nil, o.handlePaymentConfirmed(ctx)
+			},
+			handleOpenChannelError,


this is a good point and I do think we should consider handling it. In the accounts system, we are "restart" proof so probably worth trying to be restart proof here too if possible.

for the accounts system, we can do this quite easily cause we know the payment hash to track before sending the request through, so then on startup, we call LND to see if the payment with that hash did in fact go through.

So is there any way here that we can force there to be some identifier? For example, I see that OpenChannelRequest has an optional Memo string field and same for `BatchOpenChannel. Can we not force the caller to set these (to something unique that we have not seen before) and then on startup, can we then not list channels & pending channels and then subscribe to channel open events to continue tracking these? For example I see that the Channel open event does contain the memo field.

If we do this then we also would not use the temp store. Instead we would have some kind of permanent "memo -> amount" map that we can check on startup.

thoughts?

ellemouton · 2024-06-13T19:20:27Z

re-requesting from @ViktorTigerstrom since quite a bit has changed

ViktorTigerstrom

Everything looks great, awesome job 🚀fire:!!!!

Just leaving one small suggestion in my comments below. Other than that, there's one last thing I noticed which wasn't really added by this PR:
If obfuscating an amounts field, the exact returned varies for every API request. This behaviour is different than how our normal obfuscation works, as we for example will return the same obfuscated pubkey with every unique API request in the firewall. I'm not sure if this is something we should address for now, but just thought I'd brought it up!

ViktorTigerstrom · 2024-07-01T13:09:42Z

rules/onchain_budget.go

+
+// removeReqId removes the request ID from the memo if present.
+func removeReqId(memo string) string {
+	parts := strings.Split(memo, ":")


It might be worth already implementing removal of the memo in by looping the over the parts and to look for the onBudget prefix to avoid that older versions of litd in the future leak extra privacy, and to have a reference how it should be done if we implement useage of the memo field for fetching the reqId in the future.

I have implemented that idea and tested the current prefix together with a different one

ViktorTigerstrom · 2024-07-01T13:11:43Z

rules/onchain_budget_test.go

+// TestRemoveMemo tests that request identifiers are correcly removed from the
+// memo string.
+func TestRemoveMemo(t *testing.T) {
+	tests := []struct {


If you implement my suggestion above, I'd add another test that shows that you can have another part before the onBudget-connID-reqID part.
If you decide not to do it, I'd probably add another test to show that it's intended behaviour that we can't have another part before it currently.

ellemouton

Great work 🔥

A couple of minor suggestions

firewall/rule_enforcer.go

ellemouton · 2024-07-02T09:35:47Z

firewall/rule_enforcer.go

+			_, err := rule.HandleErrorResponse(ctx, ri.URI, nil)
+			if err != nil {
+				return nil, err
+			}


hmm if we return here then we dont undo all the persisted changes.... perhaps log instead and then return later?

good point 🙏

firewall/rule_enforcer.go

terminal.go

ellemouton · 2024-07-02T09:56:03Z

rules/onchain_budget.go

+	// spendAmtKey is the key that will be used in the persisted KV store to
+	// store the total amount that has been spent.
+	onChainSpent = "spent-amt"
+
+	// pendingSpentKey is the key that will be used in the persisted KV
+	// store to keep track of the total pending spent amount.
+	onChainPending = "pending-spent"


variable names dont match those in the comments

ellemouton · 2024-07-02T10:03:10Z

rules/onchain_budget.go

+func (o *OnChainBudgetMgr) Stop() error {
+	return nil
+}


surely pretty easy to test? just extend TestOnChainBudgetCheckRequest: call a request, call Stop - see that it waits, then call response & see that Stop exits

ellemouton · 2024-07-02T10:04:32Z

rules/onchain_budget.go

+func (o *OnChainBudgetMgr) Stop() error {
+	return nil
+}


this would be a bit complicated to actually implement though afaict since what happens if LND stops before lit? we would need to hook in to that to know which requests we can just not wait for

ellemouton · 2024-07-02T10:07:33Z

rules/onchain_budget.go

+
+// OnChainBudgetEnforcer enforces requests and responses against a
+// OnChainBudget rule.
+type OnChainBudgetEnforcer struct {


yeah. Worth noting that we cant really rely on the memo field not changing though. So this is a nice to have, temp measure. Worst case scenario - user just links a session and updates the budget

rules/onchain_budget.go

ellemouton · 2024-07-02T10:16:21Z

rules/onchain_budget.go

@@ -0,0 +1,785 @@
+package rules


perhaps this is a good time to start a doc where we explain small details like the memo field so that it is easy to recap in future?

so perhaps let's add a rules/docs/onchain_budget.md where we explain some things here?

cool, I added a small doc, let me know if I missed anything there

We collect errors of all rule enforcers, handling errors in all of them should an error occur. This is to roll back state consistently.

This allows us to rewrite a request.

We pass a random lnd connection identifier to the rule enforcer that is unique per lnd connection lifetime. It is used to generate unique request identifiers that amend the non-unique request identifiers that are passed from lnd.

This adds an on-chain budget that handles requests and responses for the following endpoints: * OpenChannelSync * BatchOpenChannel The budget rule checks that the onchain fee rate is not violated and that pending and confirmed amounts are handled correctly. An edge case can occur when lit crashes or shuts down after the budget rule has forwarded a request but didn't receive a response yet. The pending budget is not removed in case the request didn't go through and is not accounted towards the spent budget in case the request did go through. To be able to handle these cases in the future we add a unique identifier to the request, that can be checked by calling LND's channel bookkeeping APIs. Not all of them expose the identifier yet, which is why pending actions cannot be deleted yet. This leads to underspending of the budget and can be fixed by user intervention by creating a new session. The memo prefix is removed when reading forwarding the bookkeeping requests for privacy reasons.

We add a new channel constraint rule that enforces limits on channel opening for the OpenChannelSync and BatchOpenChannel end points. The channel constraint rule takes care about: * min channel size * max channel size * max push amount * no closing address was used

We add a peer restriction for channel opening for OpenChannelSync and BatchOpenChannel.

Channel policy boundaries are enforced for channel openings.

Pull out transaction related constants to the top of the test. Adds a debug comment that is useful for this code. It is often needed to check the human readable representation of a message.

For closes we need to know the close type and settle balances to know which peers should be avoided in the future.

Only obfuscate pending open channels for now.

We obfuscate fields from the batch channel open requests and responses.

Also adds a privacy flag that controls obfuscation of network addresses.

levmi requested review from ellemouton, guggero and ViktorTigerstrom and removed request for guggero May 2, 2024 14:16

levmi assigned bitromortac May 2, 2024

bitromortac force-pushed the autoopen branch from 9dd8b86 to c266481 Compare May 9, 2024 05:22

bitromortac commented May 9, 2024

View reviewed changes

firewall/privacy_mapper.go Outdated Show resolved Hide resolved

ellemouton reviewed May 9, 2024

View reviewed changes

jamaljsr mentioned this pull request May 9, 2024

proto: update lit protos for autopilot channel creation lightninglabs/lnc-core#35

Closed

1 task

bitromortac force-pushed the autoopen branch from c266481 to 0d6112d Compare May 14, 2024 13:07

bitromortac requested a review from ellemouton May 14, 2024 13:12

bitromortac commented May 14, 2024

View reviewed changes

rules/channel_constraints.go Outdated Show resolved Hide resolved

ellemouton reviewed May 16, 2024

View reviewed changes

bitromortac force-pushed the autoopen branch from 0d6112d to aba0601 Compare May 17, 2024 10:49

bitromortac requested a review from ellemouton May 17, 2024 12:03

ViktorTigerstrom reviewed May 22, 2024

View reviewed changes

bitromortac force-pushed the autoopen branch from aba0601 to d177c1f Compare May 23, 2024 08:44

bitromortac requested a review from ViktorTigerstrom May 23, 2024 08:54

ViktorTigerstrom approved these changes May 23, 2024

View reviewed changes

rules/channel_constraints_test.go Outdated Show resolved Hide resolved

ellemouton requested changes May 29, 2024

View reviewed changes

bitromortac force-pushed the autoopen branch from d177c1f to c48bccd Compare May 31, 2024 13:22

bitromortac requested a review from ellemouton May 31, 2024 14:11

bitromortac commented May 31, 2024

View reviewed changes

rules/onchain_budget.go Outdated Show resolved Hide resolved

ellemouton reviewed Jun 10, 2024

View reviewed changes

bitromortac force-pushed the autoopen branch from c48bccd to 4cf4cfe Compare June 13, 2024 19:13

ellemouton requested a review from ViktorTigerstrom June 13, 2024 19:20

bitromortac force-pushed the autoopen branch 3 times, most recently from 2df24e6 to fc0d5f7 Compare July 1, 2024 09:29

session: add debug output to flags error

772b626

bitromortac force-pushed the autoopen branch from fc0d5f7 to fd52787 Compare July 1, 2024 09:37

ViktorTigerstrom approved these changes Jul 1, 2024

View reviewed changes

ellemouton approved these changes Jul 2, 2024

View reviewed changes

bitromortac and others added 16 commits July 2, 2024 15:04

rules: handle interfering rule violations

8f2ab27

We collect errors of all rule enforcers, handling errors in all of them should an error occur. This is to roll back state consistently.

firewall: error back for streaming rpcs

962d666

firewall: return altered intercepted message

916ddec

This allows us to rewrite a request.

rules: pass in lnd connection identifier

7489f0a

We pass a random lnd connection identifier to the rule enforcer that is unique per lnd connection lifetime. It is used to generate unique request identifiers that amend the non-unique request identifiers that are passed from lnd.

rules: restrict channel open peers

9911052

We add a peer restriction for channel opening for OpenChannelSync and BatchOpenChannel.

rules: restrict initial channel fee parameters

a36d6c0

Channel policy boundaries are enforced for channel openings.

firewall: refactor privacy mapper tests

203d0b7

Pull out transaction related constants to the top of the test. Adds a debug comment that is useful for this code. It is often needed to check the human readable representation of a message.

firewall: obfuscate WalletBalance

a7246e1

firewall: obfuscate ClosedChannels

ef84753

For closes we need to know the close type and settle balances to know which peers should be avoided in the future.

firewall: obfuscate PendingChannels

e797abd

Only obfuscate pending open channels for now.

firewall: obfuscate BatchOpenChannel

275a882

We obfuscate fields from the batch channel open requests and responses.

firewall: obfuscate OpenChannelSync

bca729a

firewall: obfuscate ConnectPeer

66e6d63

Also adds a privacy flag that controls obfuscation of network addresses.

itest: add test for channel opening

b2c929c

bitromortac force-pushed the autoopen branch 2 times, most recently from 3ff4876 to e6bd797 Compare July 4, 2024 09:57

docs: add release notes

5e1aec0

bitromortac force-pushed the autoopen branch from e6bd797 to 5e1aec0 Compare July 4, 2024 10:51

guggero mentioned this pull request Jul 8, 2024

version: bump to v0.13.99-alpha.rc2 #787

Merged

guggero merged commit ef1ddc9 into master Jul 9, 2024
13 checks passed

guggero deleted the autoopen branch July 9, 2024 08:07

rules+firewall: rules and privacy mappings for channel creation #752

rules+firewall: rules and privacy mappings for channel creation #752

Uh oh!

Conversation

bitromortac commented Apr 26, 2024

Uh oh!

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitromortac commented May 16, 2024

Uh oh!

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bitromortac Jun 11, 2024 •

edited

Loading