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

Add macaroon based RPC middleware interceptor #5101

Merged
merged 11 commits into from
Sep 21, 2021

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Mar 14, 2021

With this PR we introduce the concept of RPC middleware: A mechanism
similar to the existing channel or HTLC interceptors but this time for
gRPC messages themselves.
An RPC middleware can register itself to the main RPC server to get
notified each time a new gRPC request comes in, a gRPC response is sent
back or a streaming RPC is connected. The middleware can
validate/inspect incoming requests and modify/overwrite outgoing
responses.

The new flow for gRPC requests with this PR looks like this:

//        |
//        | gRPC request from client
//        |
//    +---v--------------------------------+
//    |   InterceptorChain                 |
//    +-+----------------------------------+
//      | Log Interceptor                  |
//      +----------------------------------+
//      | RPC State Interceptor            |
//      +----------------------------------+
//      | Macaroon Interceptor             |
//      +----------------------------------+--------> +---------------------+
//      | RPC Macaroon Middleware Handler  |<-------- | External Middleware |
//      +----------------------------------+          |   - approve request |
//      | Prometheus Interceptor           |          +---------------------+
//      +-+--------------------------------+
//        | validated gRPC request from client
//    +---v--------------------------------+
//    |   main gRPC server                 |
//    +---+--------------------------------+
//        |
//        | original gRPC request to client
//        |
//    +---v--------------------------------+--------> +---------------------+
//    |   RPC Macaroon Middleware Handler  |<-------- | External Middleware |
//    +---+--------------------------------+          |   - modify response |
//        |                                           +---------------------+
//        | edited gRPC request to client
//        v

Motivation

This PR was initially motivated by #291. Adding all that business logic to lnd itself just didn't seem like the reasonable way to implement accounts.

Instead a similar mechanism to the channel and HTLC acceptors would make more sense as it would also allow other developers to implement custom business logic outside of lnd, remotely similar to how plugins work in c-lightning for example.
The main advantage of the approach in this PR is that the custom business logic would not live in the same process (or even a sub-process) of lnd. The main disadvantage lies in the added request/response latency that intercepting each message can add.

An example of how this can be used to implement an account system on top of lnd can be seen here, where the functionality is added to faraday.

Security

Since this also opens the door for malicious software to interfere with
lnd in a negative way, we bind everything to macaroons with custom
caveat conditions: A middleware declares upon registration which custom
caveat name it can handle. Only client requests that send a macaroon
with that custom caveat will then be given to the middleware for
inspection. The alternative is for a middleware to register for
read-only access. In that case it will get access to all requests and
responses but isn't allowed to modify anything.
Therefore requests with the default, unencumbered macaroons
can never be modified by any middleware.

Fixes #4383.

@guggero guggero added gRPC authentication macaroons P3 might get fixed, nice to have labels Mar 14, 2021
@alexbosworth
Copy link
Contributor

How do you pass extra data to the middleware in the request? Modify the proto to include the additional data?

@guggero
Copy link
Collaborator Author

guggero commented Mar 17, 2021

This isn't supported with the current version, really. Modifying the proto would kind of defeat the purpose of having a generic interceptor/middleware that allows you to use a stock lnd.
It could be done in a number of different ways though. What kind of additional information did you have in mind?
In the use case I built this for, the only extra information required is a caveat in the macaroon.

@alexbosworth
Copy link
Contributor

This isn't supported with the current version, really. Modifying the proto would kind of defeat the purpose of having a generic interceptor/middleware that allows you to use a stock lnd.
It could be done in a number of different ways though. What kind of additional information did you have in mind?
In the use case I built this for, the only extra information required is a caveat in the macaroon.

I don't have any specific information in mind, but since you can mutate the macaroon I guess you can add more request data there?

I'm curious why the interceptor doesn't see the mutated request data if it has access to the raw protobuf request blob?

@gkrizek
Copy link
Contributor

gkrizek commented Mar 19, 2021

Would registering your middleware to the main RPC require a macaroon?

Once your middleware is registered, it only see requests from macaroons with the custom caveat?

@guggero
Copy link
Collaborator Author

guggero commented Mar 19, 2021

Yes, to register the middleware you need a macaroon with the macaroon:write permission which only the admin default macaroon has. You also tell lnd which custom macaroon caveat your middleware handles and lnd will then only let you intercept any requests that have a macaroon with that caveat.

@gkrizek
Copy link
Contributor

gkrizek commented Mar 19, 2021

Got it. I think this is a great idea.

Do you think there would ever be a situation to allow all macaroons to go through the middleware or to use a macaroon other than macaroon:write to register? I'll explain an idea I had;

What if I wanted to have a readonly middleware that could keep an access logs of the commands ran on the node? In that scenario it would be great to use a macaroon with macaroon:read permissions to register the RPC middleware. And when I use the macaroon:read permission to register that means the middleware could never take any action on a request. Just view it. This would also be a good case to see all macaroon actions, not just custom caveats. Just a thought I had...

@guggero
Copy link
Collaborator Author

guggero commented Mar 22, 2021

Yeah, I think with the mechanism in place where the middleware has to "register" itself after connecting, we could definitely add some feature toggles. Where you could say "I want read access but to all requests".

And maybe we could add a new macaroon permission as well, for example middleware:read and middleware:write. With the read permission you would only be allowed to set the above mentioned flag to true anyway.
I think that could be very useful!

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Woah, this is gonna be soooo powerful

The implementation is also rather streamlined IMO, first review pass, left a few initial comments. Will also check out that Faraday example so I can get a better idea w.r.t how this can be used in practice

macaroons/constraints.go Show resolved Hide resolved
cmd/lncli/cmd_macaroon.go Show resolved Hide resolved
cmd/lncli/cmd_macaroon.go Outdated Show resolved Hide resolved
cmd/lncli/cmd_macaroon.go Show resolved Hide resolved
rpcperms/middleware_handler.go Show resolved Hide resolved
rpcperms/middleware_handler.go Show resolved Hide resolved
rpcperms/middleware_handler.go Show resolved Hide resolved
lnrpc/rest-annotations.yaml Outdated Show resolved Hide resolved
rpcperms/interceptor.go Show resolved Hide resolved
@guggero guggero force-pushed the macaroon-interceptor branch 2 times, most recently from b44eb41 to a0a66ae Compare August 11, 2021 20:26
@guggero
Copy link
Collaborator Author

guggero commented Aug 11, 2021

Rebased and addressed commits.

The following things still need to be added before the WIP state can be removed:

  • Add unit tests
  • Add integration tests
  • Add flag to enable interceptor globally

@guggero guggero force-pushed the macaroon-interceptor branch 2 times, most recently from 084889b to 74b72bc Compare August 12, 2021 20:01
@guggero
Copy link
Collaborator Author

guggero commented Aug 12, 2021

Update: only one TODO remaining, removing WIP state now:

  • Add itest case for overwriting a response.

@guggero guggero changed the title WIP: Add macaroon based RPC middleware interceptor Add macaroon based RPC middleware interceptor Aug 12, 2021
@guggero guggero requested review from Roasbeef and removed request for wpaulino August 12, 2021 20:02
@guggero guggero force-pushed the macaroon-interceptor branch from 74b72bc to eca9468 Compare August 13, 2021 07:57
@Kixunil
Copy link
Contributor

Kixunil commented Sep 13, 2021

My last question was not related to read/write permissions.

Basically I can see at least two use cases:

  • Have multiple users with budgets, each gets a unique macaroon and budgets are tracked by the middleware.
  • Let an application only see invoices it created - so if an application is compromised the privacy leak is limited.

In order to achieve this the middleware has to be able to distinguish macaroons somehow. What's the best way?

@guggero
Copy link
Collaborator Author

guggero commented Sep 13, 2021

Just need to review itest + do some regtest testing now, testing with faraday example best way to do so I assume?

@carlaKC I rebased the lndclient and faraday example branches but didn't have time to fully test them again... I hope they still work. But yes, the faraday example is probably the best one to test this out.

In order to achieve this the middleware has to be able to distinguish macaroons somehow. What's the best way?

I'm not sure I understand the problem. The middleware gets the full, raw macaroon in the interception request. So it can look at all the caveats and decide what needs to be done based on that.

@guggero guggero force-pushed the macaroon-interceptor branch from eef1b8a to 869c647 Compare September 13, 2021 14:14
@Kixunil
Copy link
Contributor

Kixunil commented Sep 13, 2021

I'm asking whether putting the client identifier into custom condition is a good way to do it or if there's a better way.

@guggero
Copy link
Collaborator Author

guggero commented Sep 13, 2021

I'm asking whether putting the client identifier into custom condition is a good way to do it or if there's a better way.

I think so, yes. With that construction you can safely give the macaroon to a user and they cannot remove the restriction.
If you want multiple middlewares to be able to interact with requests of the same user, you can do that as well. For example the accounting middleware would add the custom macaroon caveat account <acct_ID> and another , let's say custom logging middleware would add a "marker" caveat, for example mylogger.
A user would then get a macaroon that has both caveats applied. So any request from that user would be intercepted by both middlewares. And the logger middleware could still extract the account ID from that other caveat, even if it didn't specifically register for it.

@Roasbeef
Copy link
Member

Just needs a rebase and this can land, also we likely want to land this soon to avoid rebases since it touches so much in the RPC layer.

@Roasbeef Roasbeef enabled auto-merge September 16, 2021 22:32
@guggero guggero force-pushed the macaroon-interceptor branch from 869c647 to 56edb90 Compare September 17, 2021 08:32
Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

tACK! What an awesome change to lnd 🥇 can't wait to see what people build out on top of this.

Didn't test the full scope of this (had to timebox my fun), but managed to cover the following with a regtest interceptor stub:

  • Readonly interceptor
  • Custom interceptor w/ modification
  • Custom interceptor with + without caveats
  • Mandatory interceptor enforcement

Only thing that I noticed while testing is that it's difficult to use our lndclient with mandatory interceptors, since we do a bunch of GetInfo calls etc to get started, but should be easy enough to just add middle ware registration before those calls? More of a note to self than an issue with the PR.

Just an unhappy linter then we're gg 🚀

@guggero guggero force-pushed the macaroon-interceptor branch from 56edb90 to 4cd1394 Compare September 17, 2021 15:54
@guggero guggero disabled auto-merge September 17, 2021 18:20
@guggero
Copy link
Collaborator Author

guggero commented Sep 17, 2021

Oops, looks like I broke something in the itests with the latest rebase. Will fix on Monday.

We'll re-use the code for extracting a macaroon from a request context
later on so we extract it into its own exported function.
The way the macaroon bakery library lnd uses works is that one has to
register a Checker method for each caveat name that should be supported.
Since we want to allow fully customizable custom caveats we add another
layer of naming to the caveat by splitting the condition of the "outer"
caveat into two pieces, the custom caveat name and the actual custom
caveat condition.
The custom Checker function only checks that the format is correct and
that there is a handler available for a custom condition. It does not
check the condition itself, however. If the passed in acceptor signals
acceptance of a custom caveat then the bakery accepts the macaroon as a
whole (given its signature, standard caveats and permissions are all
correct) and assumes that another component down the line will make sure
the actual custom condition of a caveat is valid.
Fixes lightningnetwork#4383 by adding a new SafeCopyMacaroon function that correctly
clones all caveats and prevents modifications on the copy from affecting
the original.
With the new condition and checker in place, we can give the end user
the ability to add such a custom caveat to a baked macaroon.
There won't be an RPC counterpart for this operation since all first party
caveats currently are only added on the client side.
The custom RPC middleware logic that we are going to add in the next
commits will need to log under their own sub logger so we add one with a
new subsystem name.
With this commit we introduce the concept of RPC middleware: A mechanism
similar to the existing channel or HTLC interceptors but this time for
gRPC messages themselves.
An RPC middleware can register itself to the main RPC server to get
notified each time a new gRPC request comes in, a gRPC response is sent
back or a streaming RPC is connected. The middleware can
validate/inspect incoming requests and modify/overwrite outgoing
responses.

Since this also opens the door for malicious software to interfere with
lnd in a negative way, we bind everything to macaroons with custom
caveat conditions: A middleware declares upon registration which custom
caveat name it can handle. Only client requests that send a macaroon
with that custom caveat will then be given to the middleware for
inspection. The only exception is if the middleware instead registers
to use the read-only mode. In that mode it will be able to intercept
all requests/responses, even those not made with a special encumbered
macaroon. But the middleware won't be able to alter responses in the
read-only mode. Therefore requests with the default, unencumbered macaroons
can never be modified by any middleware.
@guggero guggero force-pushed the macaroon-interceptor branch from 4cd1394 to d38853c Compare September 20, 2021 11:38
With the middleware handler in place, we now need to add a new gRPC
interceptor to the interceptor chain that will send messages to the
registered middlewares for each event that could be of interest to them.
@guggero guggero force-pushed the macaroon-interceptor branch from d38853c to 3f7909b Compare September 20, 2021 15:05
@Roasbeef Roasbeef merged commit 9264185 into lightningnetwork:master Sep 21, 2021
@guggero guggero deleted the macaroon-interceptor branch September 21, 2021 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authentication gRPC macaroons P3 might get fixed, nice to have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macaroons: macaroon.Clone() doesn't copy caveats
7 participants