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

[WIP, feedback wanted] macaroons: add new request hash caveat to improve replay protection #1181

Closed

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented May 4, 2018

This PR addresses #285.

To make sure that a macaroon that is somehow intercepted during transmission cannot be used by an attacker, the lncli already sets a caveat for a 60 second timeout. Additionally, the macaroon can be locked down to a specific IP address.
With this PR the gRPC client (for now, just lncli, the feature can be disabled with --no-macaroon-request-hash) creates a SHA256 hash of the method name concatenated with the JSON serialized request gRPC object.
That hash is added to the macaroon as a first-party caveat named request-hash.
On the server side, this caveat is validated by hashing the method and request object again.

I'm not sure if I'm on the right track with my implementation. That's why I created the PR with the status 'work in progress'. I would really appreciate feedback on what I did, especially from @aakselrod since he created the original issue/feature request.

As I see it, the following questions arise from the way I implemented it:

  • The solution works well for unary/synchronous requests, since the request data is available and can be hashed. For streaming/asynchronous requests, this cannot be done, because the macaroon is only sent at the beginning, when opening the connection. But through that connection, many requests can then be made and no further macaroon is required. For example, the SendPayment is a streaming command where several payments can be sent through the same stream but with only one macaroon that is sent at the beginning. So the only thing we can hash is the method name itself (like lnrpc.SendPayment). But if an attacker can sniff that macaroon, he can send payments himself (until the 60 second timeout runs out). So my question is: Does anyone have an idea on how to further improve the replay protection for streaming requests?
  • The JSON serialization of a request object should produce the same output for the same object on all platforms/languages/libraries, because this is done by the gRPC library that should behave the same in Java, JavaScript, Python, Ruby, and so on. So it would be possible for this feature to work for other clients than just lncli and should in principle also be doable over the REST gateway. My question is: Should we provide code samples on how to do that for all languages? Should we maybe write some kind of simple integration tests for the languages we support?

This is the current state of the implementation:

  • Get the request-hash feature to work for unary/synchronous requests in lncli.
  • Get the request-hash feature to work for streaming/asynchronous requests in lncli.
  • Add a feature toggle parameter called --no-macaroon-request-hash.
  • Add a test for the UnaryServerInterceptor.
  • Add documentation on how this is implemented in lncli.
  • Add documentation on how the request hash feature can be used by app developers using the gRPC interface.
  • Finish tests for all client and server interceptors.
  • Manually test all unary and stream commands in lncli.
  • Split commits into smaller changes

As always, thank you for your input!

@guggero guggero force-pushed the macaroon-request-hash branch 4 times, most recently from 5166706 to 2715521 Compare May 12, 2018 10:50
@guggero guggero force-pushed the macaroon-request-hash branch 2 times, most recently from 4ec6ae5 to bf8af71 Compare May 23, 2018 11:28
@guggero guggero force-pushed the macaroon-request-hash branch 3 times, most recently from 637734f to 72311bc Compare June 5, 2018 08:24
@guggero guggero force-pushed the macaroon-request-hash branch 3 times, most recently from 15ee13e to b4032d2 Compare June 14, 2018 06:21
@guggero guggero force-pushed the macaroon-request-hash branch 2 times, most recently from cdbfed0 to a8c5970 Compare July 1, 2018 10:30
@Roasbeef Roasbeef added P3 might get fixed, nice to have needs review PR needs review by regular contributors labels Jul 10, 2018
@guggero guggero force-pushed the macaroon-request-hash branch 2 times, most recently from 8f29a4a to e632adf Compare July 29, 2018 07:58
@guggero guggero changed the title [WIP] macaroons: add new request hash caveat to improve replay protection [WIP, feedback wanted] macaroons: add new request hash caveat to improve replay protection Jul 30, 2018
@guggero guggero force-pushed the macaroon-request-hash branch 2 times, most recently from cdbbd5b to 6bca2ad Compare August 19, 2018 10:28
@guggero guggero force-pushed the macaroon-request-hash branch 2 times, most recently from fb0007a to 6ae3ef9 Compare September 15, 2018 06:21
@guggero guggero force-pushed the macaroon-request-hash branch 2 times, most recently from 5ea5e34 to f51af6e Compare December 29, 2018 13:27
@guggero guggero force-pushed the macaroon-request-hash branch 2 times, most recently from 78549b0 to a9cb175 Compare January 4, 2019 07:55
@guggero guggero force-pushed the macaroon-request-hash branch 2 times, most recently from d7264d1 to 1bab374 Compare February 22, 2019 11:03
@guggero
Copy link
Collaborator Author

guggero commented Jan 14, 2020

Not really relevant and not a big security benefit.

@guggero guggero closed this Jan 14, 2020
@guggero guggero deleted the macaroon-request-hash branch December 27, 2020 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macaroons needs review PR needs review by regular contributors P3 might get fixed, nice to have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants