Skip to content
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.

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

cmd/litcli: add super macaroon helper commands #568

Merged
merged 6 commits into from
Jun 20, 2023

Conversation

ellemouton
Copy link
Member

@ellemouton ellemouton commented Jun 2, 2023

This PR adds a few super macaroon related commands:

  • litcli helper supermacrootkey command is added which lets a user generate a random root key ID for a super macaroon
  • litcli helper issupermacaroon command allows a users to easily confirm if a macaroon is considered a super macaroon or not.
  • litcli helper bakesupermacaroon can be used to bake a super macaroon.

The supermacrootkey and issupermacaroon commands dont require a connection to LiTd.

@ellemouton
Copy link
Member Author

cc @gkrizek

@ellemouton
Copy link
Member Author

happy to add any other helpers in this area that would be useful :) ideas welcome!

@ellemouton
Copy link
Member Author

i could maybe add a bakesupermac endpoint?

@CRex15
Copy link

CRex15 commented Jun 2, 2023

i could maybe add a bakesupermac endpoint?

That would be quite helpful. It was quite complicated and error prone trying to bake the super macaroon with the normal BakeMacaroon API as described here: lightninglabs/lightning-api-ng#19. I think it would also help clarify that there is a difference between the two, which I was unaware of before running into that issue.

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 addition! Just a few usability/UX comments, otherwise looks great.

cmd/litcli/helpers.go Outdated Show resolved Hide resolved
cmd/litcli/helpers.go Show resolved Hide resolved
cmd/litcli/helpers.go Outdated Show resolved Hide resolved
@lightninglabs-deploy
Copy link

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

@ellemouton ellemouton force-pushed the superMacHelpers branch 2 times, most recently from 17458b8 to 27e6806 Compare June 14, 2023 19:03
Copy link
Member Author

@ellemouton ellemouton left a comment

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 ! I've updated and have added a BakeSuperMacaroon endpoint along with a bakesupermacaroon litcli command.

cmd/litcli/helpers.go Show resolved Hide resolved
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, LGTM 🎉

cmd/litcli/proxy.go Outdated Show resolved Hide resolved
@guggero
Copy link
Member

guggero commented Jun 15, 2023

Ah, just one request: Can we please add the new RPC to one of the itests? Just to make sure authentication and everything works as expected.

@ellemouton
Copy link
Member Author

Can we please add the new RPC to one of the itests? Just to make sure authentication and everything works as expected.

is this not covered by the existing test for litrpc.Proxy.GetInfo? since they are both for the same rpc service.

or do you mean that we should use the new endpoint to bake a macaroon in the itests & then test that that macaroon works for other endpoints?

@guggero
Copy link
Member

guggero commented Jun 15, 2023

is this not covered by the existing test for litrpc.Proxy.GetInfo? since they are both for the same rpc service.

Ah yes, you're right. So all the security restrictions should apply to the new call as well. SGTM.

Copy link
Contributor

@ViktorTigerstrom ViktorTigerstrom left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM once rebased 🚀! Should be really useful commands!!

Only thing I noticed are a minor typo, and a sanity check to ensure my comment below isn't a potential issue.

cmd/litcli/proxy.go Show resolved Hide resolved
cmd/litcli/helpers.go Show resolved Hide resolved
var file_proxy_proto_goTypes = []interface{}{
(*StopDaemonRequest)(nil), // 0: litrpc.StopDaemonRequest
Copy link
Contributor

@ViktorTigerstrom ViktorTigerstrom Jun 19, 2023

Choose a reason for hiding this comment

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

This is probably a stupid comment, but just wanted to make sure this isn't a potential issue:

There is no problem here that this overwrites the old msgTypes values here with new types, correct (ie. value 0 was the type litrpc.StopDaemonRequest but now becomes litrpc.BakeSuperMacaroonRequest)? Just thinking if we'd have external services or something that uses the protos and that way become incompatible, or if this could somehow become a backwards compatibility issue if one downgrades LiT or so.

This is likely not something that could become an issue, but thought I'd just leave this comment to make sure that's the case.

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 question!

So I think that either:

  • these indices are only used within this file. So if you scroll up to where file_proxy_proto_msgTypes is used, you can see that it is only used by the message types once the types are known and are not used on the wire.
  • or, they are used on the wire but the mapping is conveyed via the file_proxy_proto_rawDesc descriptor above.

either way - gRPC protos are backwards compatible & you can test this by running Lit with this PR but compiling litcli with master & then see that the litcli getinfo or litcli stop calls work just the same no matter if litcli is compiled with this branch or master branch :)

Regenerate the protos after the recent bump of the protobuf dependency.
In this commit, a `supermacrootkey` helper command is added which lets a
user generate a random root key ID for a super macaroon along with an
`issupermacaroon` command that allows a users to easily confirm if a
macaroon is considered a super macaroon or not. Neither of these
commands require a connection to LiTd.
@ellemouton ellemouton merged commit fca7cfe into lightninglabs:master Jun 20, 2023
@ellemouton ellemouton deleted the superMacHelpers branch June 20, 2023 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants