Skip to content

lncli: add command to create new macaroon#1160

Merged
Roasbeef merged 3 commits into
lightningnetwork:masterfrom
guggero:new-macaroon
Oct 25, 2019
Merged

lncli: add command to create new macaroon#1160
Roasbeef merged 3 commits into
lightningnetwork:masterfrom
guggero:new-macaroon

Conversation

@guggero
Copy link
Copy Markdown
Collaborator

@guggero guggero commented Apr 30, 2018

As discussed in #1147 there is a need to create macaroons with custom permissions set since the three existing macaroon files admin.macaroon, invoice.macaroon and readonly.macaroon aren't fine-grained enough.

A new gRPC method named NewMacaroon is added:

  • Introduces a new permission entity named macaroon (could be used for macaroon based RPCs mentioned in Add support for accounting-based macaroons #291 too).
  • write access to the entity macaroon is necessary to call the method NewMacaroon.
  • The admin.macaroon gets write access to the entity macaroon.
  • A new command is added to lncli named newmacaroon that calls this gRPC method.
  • As parameters to the NewMacaroon method a list of entity/action pairs for the allowed operations can be passed.

Example:

lncli newmacaroon --permission=invoices/write --permission=invoices/read --save_to=~/.lnd/custom-invoice.macaroon --timeout=10

Creates a macaroon that is valid for reading and writing invoices during the next 10 seconds.

Closes #283, #1147, #3516.

NOTE for release notes: Users will need to delete or move their admin.macaroon, readonly.macaroon and invoices.macaroon before starting 0.9, otherwise they won't get regenerated macaroons that have the required permission (macaroon:generate) to mint custom macaroons.

@vegardengen
Copy link
Copy Markdown
Contributor

This one actually conflicts with #1147

Would it make sense to make this into one common PR? Or merge one, then rebase/fix conflicts after that?

@meshcollider meshcollider added macaroons cli Related to the command line interface gRPC labels May 1, 2018
@guggero
Copy link
Copy Markdown
Collaborator Author

guggero commented May 1, 2018

If either of the two PRs is merged, I'll rebase the other one. The functionality should not conflict, both commands have their usefulness IMO, even if there is overlap in some of the command arguments.

@Roasbeef Roasbeef requested a review from aakselrod May 1, 2018 19:54
@guggero guggero force-pushed the new-macaroon branch 6 times, most recently from 898a08e to de4c7ca Compare May 8, 2018 09:51
@guggero guggero force-pushed the new-macaroon branch 3 times, most recently from 4e4cca6 to 0cd3c8a Compare May 12, 2018 10:47
@guggero guggero force-pushed the new-macaroon branch 2 times, most recently from 140edb9 to 2d79147 Compare May 23, 2018 11:00
@guggero guggero force-pushed the new-macaroon branch 2 times, most recently from 5c0a41f to 798db9c Compare June 1, 2018 06:13
@guggero guggero force-pushed the new-macaroon branch 3 times, most recently from f829b7e to b04419a Compare June 9, 2018 06:03
@guggero guggero force-pushed the new-macaroon branch 2 times, most recently from 850f64b to 6fbc5b7 Compare June 14, 2018 06:22
@guggero guggero force-pushed the new-macaroon branch 2 times, most recently from ca7c476 to e17108b Compare July 1, 2018 10:31
@Roasbeef Roasbeef added P3 might get fixed, nice to have needs review PR needs review by regular contributors needs testing PR hasn't yet been actively tested on testnet/mainnet labels Jul 10, 2018
@guggero guggero requested a review from Roasbeef as a code owner September 30, 2019 08:10
@joostjager joostjager added this to the 0.9 milestone Oct 9, 2019
@guggero guggero force-pushed the new-macaroon branch 2 times, most recently from fa64927 to 9290643 Compare October 10, 2019 13:44
@joostjager
Copy link
Copy Markdown
Contributor

During testing I noticed that all of the main macaroons (not just admin.macaroon) need to be removed in order to trigger regeneration. Idk if there is a reason for that.

Comment thread lnrpc/rpc.proto Outdated
Comment thread cmd/lncli/commands.go Outdated
Comment thread cmd/lncli/commands.go Outdated
Comment thread cmd/lncli/commands.go Outdated
Comment thread cmd/lncli/commands.go Outdated
Comment thread cmd/lncli/commands.go Outdated
Comment thread cmd/lncli/commands.go Outdated
Comment thread cmd/lncli/commands.go Outdated
Comment thread cmd/lncli/commands.go Outdated
Comment thread rpcserver.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you'd be following up this PR with more granular macaroon baking on the individual call level, how would that map to bakery.Ops?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you mean being able to create a macaroon for a RPC call that shares permissions with another, but the macaroon is only allowed for the first one?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

My idea for the follow-up PR would be that we would use a constant for the entity (for example "rpc") and the full RPC URL (as seen in rpcserver.go, for example "/lnrpc.Lightning/SendCoins") as the action. This would result in quite large macaroons but would offer most flexibility.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have we already heard of a use case where our partitioning is insufficient and this fine granularity is required?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not yet. But I can think of multiple use cases for this. For example being able to estimate a fee through lnd but not being able to see the wallet balance.

@guggero guggero requested a review from wpaulino October 17, 2019 10:22
Copy link
Copy Markdown
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

During testing I noticed that all of the main macaroons (not just admin.macaroon) need to be removed in order to trigger regeneration. Idk if there is a reason for that.

Don't know why either, it should regenerate any that don't already exist. See

lnd/lnd.go

Line 360 in 254de64

if !fileExists(cfg.AdminMacPath) && !fileExists(cfg.ReadMacPath) &&
.

Great work @guggero! I know many users have requested this feature in the past, so it's nice to finally be able to push this forward.

Comment thread lnrpc/rpc.proto Outdated
Comment thread server.go Outdated
Comment thread rpcserver.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you mean being able to create a macaroon for a RPC call that shares permissions with another, but the macaroon is only allowed for the first one?

Comment thread rpcserver.go Outdated
Comment thread rpcserver.go Outdated
Comment thread cmd/lncli/commands.go Outdated
Comment thread cmd/lncli/commands.go Outdated
Comment thread cmd/lncli/commands.go Outdated
@guggero
Copy link
Copy Markdown
Collaborator Author

guggero commented Oct 23, 2019

Thank you @joostjager and @wpaulino for the reviews!
I addressed everything unless explicitly commented above.
As discussed offline, I moved the list of permissions that exist to the server side and removed it from the command documentation text. The full list of supported actions and entities is returned in the error message if a user specifies something incorrect.

Will introduce a RPC call to get all supported permissions in the follow-up PR that also introduces fine-grained permissions on individual RPC method level.

Comment thread cmd/lncli/cmd_new_macaroon.go Outdated
Comment thread cmd/lncli/cmd_new_macaroon.go Outdated
Comment thread cmd/lncli/cmd_new_macaroon.go Outdated
Comment thread lnrpc/README.md Outdated
Comment thread rpcserver.go Outdated
Comment thread rpcserver.go Outdated
Comment thread rpcserver.go Outdated
Comment thread rpcserver.go Outdated
Comment thread rpcserver.go Outdated
Comment thread rpcserver.go Outdated
Comment thread rpcserver.go Outdated
Comment thread rpcserver.go Outdated
Comment thread cmd/lncli/cmd_new_macaroon.go Outdated
Comment thread rpcserver.go Outdated
Comment thread cmd/lncli/cmd_new_macaroon.go Outdated
Comment thread cmd/lncli/cmd_new_macaroon.go Outdated
Copy link
Copy Markdown
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

LGTM 💥

I think another nice follow-up would be to allow lncli to use a custom macaroon through a flag as hex to prevent having to save it to a file.

Comment thread cmd/lncli/cmd_new_macaroon.go Outdated
Comment thread rpcserver.go Outdated
@guggero
Copy link
Copy Markdown
Collaborator Author

guggero commented Oct 24, 2019

@Roasbeef asked me to rename the service to BakeMacaroon.

Comment thread rpcserver.go Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli Related to the command line interface gRPC macaroons needs review PR needs review by regular contributors needs testing PR hasn't yet been actively tested on testnet/mainnet P3 might get fixed, nice to have

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add command-line tool to delegate macaroons

7 participants