-
Notifications
You must be signed in to change notification settings - Fork 103
multi: extract session ID from context #1045
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
Conversation
b5f84a5
to
be018e8
Compare
667e513
to
00f7845
Compare
Add grpc interceptors that inject an LNC session's ID into the context as gRPC metadata. By injecting it as such, it will be transported over the wire in any outgoing gRPC calls. This lets us be sure that any session call sent to the RPCMiddleware interceptor in LND will continue to be grouped along with the appropriate session ID. This gives LND a way to send the metadata we include back to LiT meaning that we will later on be able to extract the session ID again.
00f7845
to
dea6cd9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work figuring this out 🎉. This is covered by itests, otherwise they would error out if no session id is transported and we use lnd v0.19.0-rc3, nice.
Are there any security implications as we are now relying on data that can be modified by lnd?
This is already the case for any of the data we get from the interceptor. This is why users need to take caution and only set the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice and clean PR 🚀!
In this commit, we update our various firewall interceptors so that they rely on the session ID passed via gRPC metadata to extract a session ID. For the PrivacyMapper and RuleEnforcer, these _MUST_ always contain a session ID and so we error out if one was not found. For the request logger, the session ID is optional and so we pass it to the new SessionID field in the AddActionReq - our bbolt actions DB will not make use of this field on persistence (but our incoming SQL version will).
dea6cd9
to
a89b350
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already the case for any of the data we get from the interceptor. This is why users need to take caution and only set the rpcmiddleware.enable flag if they are sure they can trust the lnd node that lit is connecting to
I think you meant the other way around, because the lnd node needs to trust litd, which runs the interceptor? With this PR, this gives the lnd node slightly more control to trick the litd node. I think the trust assumption is that litd and lnd are operated by the same entity, so it doesn't really matter, but maybe we should document this trust assumption somewhere.
Currently, in our RPC interceptors, we use the macaroon data in the request to extract the session ID. This is not ideal since we are using the root key ID of the macaroon and this can cause issues because sometimes we also store the first 4 bytes of an account ID in there. Or it could just be a totally unrelated macaroon that happens to have root key ID that matches with one of our session IDs.
We should instead use a much more reliable way of passing along session ID info: we can do this because any request that comes in via a session, we can intercept and add the session ID to the grpc metadata for the call via the context. This will then be included in the ctx info that LND gets when which interceptors to send the request too. So LND can then send this metadata info back to lit via the
lnrpc.RPCMiddlewareRequest
message (see PR here), LiT can then extract the session ID in a much more reliable way during request interception.NOTE: with this change, it must be the case that in remote mode LND is >=v0.19.0 for this change to work properly (since only in 19 will LND send back the metadata we use here) . So in our next release, we need to bump our minimum required LND version to 19 (we might do it anyways for taproot-asset reasons).
Depends on: