Skip to content

multi: custom URI sessions #438

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.

Sign up for GitHub

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
Merged
merged 5 commits into from
Oct 27, 2022
Merged

multi: custom URI sessions #438

merged 5 commits into from
Oct 27, 2022

Conversation

ellemouton
Copy link
Member

@ellemouton ellemouton commented Oct 25, 2022 •
edited
Loading

With this PR, you can now create a custom session where on creation, you can specify the exact list of URIs that can be called with the session.

Can create a new custom session as follows:

litcli  s a --label=blah --type=custom --uri_list=/lnrpc.Lightning/ListChannels,/poolrpc.Trader/GetInfo

Can test this with lightninglabs/lightning-node-connect#59

@ellemouton ellemouton mentioned this pull request Oct 25, 2022
Merged
guggero
guggero reviewed Oct 25, 2022
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very nice! Looking forward to this feature 🎉

session_rpcserver.go Outdated
"readonly and custom macaroon types supported in LiT")
}

var perms []bakery.Op
Copy link
Member

Choose a reason for hiding this comment

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

Can we refactor this into a switch so it'll be easier to add the account session type, please? See 9b24a18.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah great idea 👍

session_rpcserver.go Outdated
@@ -203,10 +231,17 @@ func (s *sessionRpcServer) resumeSession(sess *session.Session) error {
Id: []byte(macExpiry.Condition),
})

if sess.Type == session.TypeMacaroonCustom {
Copy link
Member

Choose a reason for hiding this comment

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

Same here re switch statement.

litrpc/lit-sessions.proto Outdated
@@ -76,6 +76,8 @@ message Session {
bytes remote_public_key = 10;

uint64 created_at = 11 [jstype = JS_STRING];

repeated MacaroonPermission macaroon_custom_permissions = 12;
Copy link
Member

Choose a reason for hiding this comment

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

Should we show the full recipe here? So including the caveats?

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea!

Copy link
Member Author

@ellemouton ellemouton Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

hmmm do we also want to add a custom-caveats option to the AddSessionRequest message then?

Copy link
Member Author

Choose a reason for hiding this comment

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

if we were to add the custom-caveats option to the request, then would it be fine just to let the user specify strings that we will use for the caveat ID? ie, is it fine to leave the location & verification ID empty ?

session_rpcserver.go Outdated
}, nil
}

// marshalRPCCustomPerms converts a list of macaroon permissions into their RPC
// counterpart.
func marshalRPCCustomPerms(ops []bakery.Op) []*litrpc.MacaroonPermission {
Copy link
Member

Choose a reason for hiding this comment

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

See previous comment about adding the full recipe to the session RPC, then this could marshal the whole recipe (and we could skip the nil check on L419 and do that here.

cmd/litcli/sessions.go Outdated
"this list will only be used if the 'type' " +
"flag is set to 'custom'. If multiple URIs " +
"are specified, they should be separated by " +
"commas (eg. 'uri1,uri2,uri3')",
Copy link
Member

Choose a reason for hiding this comment

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

Should we use a StringSlice flag instead and just mention "can be specified multiple times" here?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure 👍

@ellemouton ellemouton requested review from itsrachelfish and removed request for jamaljsr October 25, 2022 16:13
ellemouton
ellemouton commented Oct 26, 2022
View reviewed changes
Copy link
Member Author

@ellemouton ellemouton left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the review @guggero!!

Updated as per suggestion. Added 2 more commits. The first makes the run of the existing LNC auth itests more efficient and the second adds new tests for the custom macaroon perms.

Also left one question about custom caveats

litrpc/lit-sessions.proto Outdated
@@ -76,6 +76,8 @@ message Session {
bytes remote_public_key = 10;

uint64 created_at = 11 [jstype = JS_STRING];

repeated MacaroonPermission macaroon_custom_permissions = 12;
Copy link
Member Author

Choose a reason for hiding this comment

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

good idea!

session_rpcserver.go Outdated
"readonly and custom macaroon types supported in LiT")
}

var perms []bakery.Op
Copy link
Member Author

Choose a reason for hiding this comment

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

yeah great idea 👍

cmd/litcli/sessions.go Outdated
"this list will only be used if the 'type' " +
"flag is set to 'custom'. If multiple URIs " +
"are specified, they should be separated by " +
"commas (eg. 'uri1,uri2,uri3')",
Copy link
Member Author

Choose a reason for hiding this comment

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

sure 👍

@ellemouton ellemouton requested a review from guggero October 26, 2022 08:19
guggero
guggero previously approved these changes Oct 26, 2022
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very cool, LGTM 🎉

@ellemouton
litcli: update to allow for custom URI list
61792bf
@ellemouton
itest: optimise the LNC tests
ec5b512 
To speed up the existing LNC auth tests, this commit creates a single
LNC session & connection and does all the tests over the single
connection rather than spinning up one per test. This imporves the time
taken to run the tests drastically.
@ellemouton
itest: add tests for custom macaroon sessions
b38acfa
jamaljsr
jamaljsr approved these changes Oct 26, 2022
View reviewed changes
Copy link
Member

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

tACK LGTM 🔥

Works as advertised. I just have a question. I am testing this along with lightninglabs/lightning-node-connect#59. I was able to add a custom session successfully.

$ rt run zanelit sessions add --label mycustom --type=custom --uri=/lnrpc.Lightning/GetInfo --uri=/routerrpc.Router/GetMissionControlConfig
{
	"session": {
		"label": "mycustom",
		"session_state": "STATE_CREATED",
		"session_type": "TYPE_MACAROON_CUSTOM",
		"expiry_timestamp_seconds": "1674590241",
		"mailbox_server_addr": "mailbox.terminal.lightning.today:443",
		"dev_server": false,
		"pairing_secret": "43a472e678984e803ac31e4cba80",
		"pairing_secret_mnemonic": "dry castle ridge van lumber divorce twelve glow erosion staff",
		"local_public_key": "03e728a7ab09810c97b98d47cc792686ab294d139091bfdcca9dcca5bac1628afd",
		"remote_public_key": null,
		"created_at": "1666814241",
		"macaroon_recipe": {
			"permissions": [
				{
					"entity": "uri",
					"action": "/lnrpc.Lightning/GetInfo"
				},
				{
					"entity": "uri",
					"action": "/routerrpc.Router/GetMissionControlConfig"
				}
			],
			"caveats": [
			]
		}
	}
}

When I connected with the LNC example-server, the lnrpc.State.GetState RPC was still available. is this intentional? If so, why is that?

image

@ellemouton
Copy link
Member Author

@jamaljsr , yes that is expected because lnrpc.State.GetState does not require any permissions so will always be callable

@ellemouton ellemouton merged commit 9126875 into lightninglabs:master Oct 27, 2022
@ellemouton ellemouton deleted the fine-grained-perms branch October 27, 2022 07:41
@itsrachelfish itsrachelfish mentioned this pull request Oct 27, 2022
Closed
@itsrachelfish itsrachelfish mentioned this pull request Nov 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guggero guggero guggero approved these changes

@jamaljsr jamaljsr jamaljsr approved these changes

@itsrachelfish itsrachelfish Awaiting requested review from itsrachelfish

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ellemouton @guggero @jamaljsr