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] Enable rpc TLS and macaroon authentication #4129

Closed

Conversation

ghubstan
Copy link
Contributor

@ghubstan ghubstan commented Apr 7, 2020

This change adds the simplest macaroon authentication scheme
with no caveats (analog to ACLs). A macaroon is created in the
app data dir by BisqSetup, if needed, and all :cli calls include
that hex encoded macaroon to the server for authentication.

To enable TLS, a temporary certificate and pkcs8 key were manually
generated by a bash script in new temp folder (cert). The cert &
key are installed in the same temporary folder. The server depends
on both the cert and key, the client on the cert.

More specific code changes to support tls/auth:

  • Added macaroons dependency to :core and :cli.

  • Build grpc server instance with useTransportSecurity(cert,key).

  • Inject Config into CoreApi so it can pass the appDataDir to
    the grpc AuthenticationInterceptor.

  • Bakes new macaroon in a new MacaroonOven during server startup
    (if not present).

Other changes:

  • Daemon resources folder was moved to the expected location under
    src/main.

  • Added -XX:MaxRAM=4g jvm option to bisq-daemon and bisq-cli startup
    scripts. This cuts :daemon's resident memory consumption by 4 GB.
    (This option should probably be added to all startup scripts.)

A few comments not included in commit...

In general, I am attempting to imitate the way Lightning Network's lnd project uses macaroons. For an intro, see

Some of the next problems to solve are

  • The appDataDir is not available to :cli, as Config is not in the classpath. There is
    a temporary hack to find the default appDataDir (where the macaroon lives) on
    OSX and Linux, but not Windows.

  • The end-user needs to be informed that his TLS certificate and macaroon need
    to be copied to his :cli host, if different than :daemon host.

  • A hard coded macaroon secretKey is passed from BisqSetup to the MacaroonOven
    in maybeCreateMacaroon().

  • Not sure about proper way to create certificate and key for TLS,
    currently using the bash script in cert folder to generate cert & key
    for development.

  • The certificate+key and macaroon need to be created for correct hostname(s),
    for now only works for 'localhost'.

  • GrpcServer has hard coded paths to temporary cert & pkcs8 key:

    server = ServerBuilder.forPort(port).useTransportSecurity(
        new File("cert/aes256/server.crt"),
            new File("cert/aes256/pkcs8_key.pem"))
    
    
  • Need to find a TLS cert encryption algo "thought" not to be broken
    by the NSA & Co., and choices are limited by what Netty supports.

This change adds the simplest macaroon authentication scheme
with no caveats (analog to ACLs).  A macaroon is created in the
app data dir by BisqSetup, if needed, and all :cli calls include
that hex encoded macaroon to the server for authentication.

To enable TLS, a temporary certificate and pkcs8 key were manually
generated by a bash script in new temp folder (cert).  The cert &
key are installed in the same temporary folder.  The server depends
on both the cert and key, the client on the cert.

More specific code changes to support tls/auth:

 * Added macaroons dependency to :core and :cli.

 * Build grpc server instance with useTransportSecurity(cert,key).

 * @Inject Config into CoreApi so it can pass the appDataDir to
  the grpc AuthenticationInterceptor.

 * Bakes new macaroon in a new MacaroonOven during server startup
  (if not present).

Other changes:

 * Daemon resources folder was moved to the expected location under
   src/main.

 * Added -XX:MaxRAM=4g jvm option to bisq-daemon and bisq-cli startup
   scripts.  This cuts :daemon's resident memory consumption by 4 GB.
   (This option should probably be added to all startup scripts.)
@ghubstan ghubstan requested a review from cbeams as a code owner April 7, 2020 14:38
@ripcurlx ripcurlx changed the title Enable rpc TLS and macaroon authentication [WIP] Enable rpc TLS and macaroon authentication Apr 10, 2020
@ripcurlx
Copy link
Contributor

@ghubstan Just edited the title to make it more obvious from the outside that this PR is [WIP]. If you want to be on the safe side, that your PR doesn't get merged just make it a Draft PR.

@ghubstan ghubstan marked this pull request as draft April 10, 2020 14:58
@cbeams
Copy link
Contributor

cbeams commented Apr 20, 2020

@ghubstan, I've taken a look at this and run it for myself. At this point it seems like the macaroon approach is simply too complex for any benefit it might bring. I feared this might be the case. Incidentally, I overheard a conversation the other day that the LND team has never actually used any of the more advanced aspects of macaroons prior to the recent announcement of Lightning Service Authentication Tokets (LSAT), which is something that Bisq would have no analog for.

I realize this was probably quite a bit of effort to put this PR together, but it confirms my fear that this kind of auth approach may simply be too heavy with little real benefit. It's not only heavy from an implementation point of view, but from a usage point of view. The bitcoin-cli basic auth model is familar to everyone, and doing things with certs in this kind of context is not obvious at all.

At this point I'd like to shift gears and try implementing basic auth in gRPC after all. Like with bitcoind, we should make it clear that Bisq's RPC API is not suitable for exposure to the open internet. Instead, we would assume and encourage that any non-localhost access of Bisq's RPC API should happen via Tor. This is natural enough, given that Bisq is Tor-based already.

Would you be willing to take a cut at implementing basic auth (in a separate PR)? I'm happy to get together and talk about this as well, if you have any concerns.

@ghubstan
Copy link
Contributor Author

Yes, I'll back up and create a new PR that implements basic auth using bitcoin-cli as the reference model.

@ghubstan ghubstan closed this Apr 23, 2020
@ghubstan ghubstan deleted the grpc-tls-n-macaroon-auth-v0.01 branch April 23, 2020 17:51
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.

3 participants