lndk-org / lndk

MIT License
84 stars 22 forks source link

Setup grpc server #104

Closed orbitalturtle closed 5 months ago

orbitalturtle commented 6 months ago

This PR sets up a grpc server for interacting with a running LNDK instance. The main command it supports right now is to pay an offer.

In summary:

mrfelton commented 6 months ago

Why do we need to need to pass the lnd connection params through to the grpc calls using grpc metadata, given that we already establish those through lndk's startup paramaters?

orbitalturtle commented 6 months ago

@mrfelton This is actually something I went back and forth on, and would love some feedback. I landed on passing in the credentials because we need some sort of authentication mechanism for using lndk-cli anyway, and it seems reasonable to me to make sure that whoever's using lndk-cli is actually authorized to be using lnd. And since lnd already has those auth measures, it feels better to me to pass those credentials in here then to invent some new auth measures if it's not needed.

You mentioned tls in Slack, which I agree is a must. But even with that, it feels wrong to me to not verify that the lndk-cli user has the needed lnd credentials.

@carlaKC would love your thoughts on this as well when you have a moment -- though take your time.

mrfelton commented 6 months ago

It would make more sense to actually secure the LNDK gRPC server with the lnd creds directly.

The user story should be:

As a user of LNDK, I should be required to provide LND creds in order to use the API. 

As it's implemented now, it's not adding any additional security since the only thing being passed in the grpc metadata is the paths to the creds, not the creds themselves. So one only has to guess what path the creds are stored at on the LNDK instance in order to be able to pay offers, and they are most likely stored in the default location.

This is how it works currently:

var channel = new(lndkUriWithPort, ChannelCredentials.Insecure);
var client = new OffersClient(channel),
var headers = new Metadata {
    {"tls_cert_path", "/lnd/.lnd/tls.cert"},
    {"macaroon_path", "/lnd/.lnd/data/chain/bitcoin/regtest/admin.macaroon"},
    {"address", "https://olympus-lndus1:10009"}
};
var res = await client.Offers.PayOfferAsync(new PayOfferRequest {
    Offer = offer,
    Amount = amount,
}, headers: headers);

Anyone can guess those paths.

The caller should be required to pass the actual creds, so the client call should look more like:

var certPem = "-----BEGIN CERTIFICATE-----\n...";
var macaroonHex = "0201036C6E6402F801...";

ChannelCredentials credentials = new SslCredentials(certPem);
var macaroonInterceptor = new AsyncAuthInterceptor((_, metadata) => {
    metadata.Add(new("macaroon", macaroonHex));
    return Task.CompletedTask;
});
credentials = ChannelCredentials.Create(credentials, CallCredentials.FromInterceptor(macaroonInterceptor));
var channel = new(lndkUriWithPort, credentials);
var client = new OffersClient(channel),
var res = await client.Offers.PayOfferAsync(new PayOfferRequest {
    Offer = offer,
    Amount = amount,
});

Either that, or LNDK should generate it's own tls cert and macaroon and those should be required to call it's gRPC api.

mrfelton commented 6 months ago

A couple of things I noticed:

  1. lndk uses --cert whilst lndk-cli uses --tls-cert. Would be better if these were consistent to avoid confusion
  2. lndk has the default value for --grpc-host as ::1 which is a reasonable default for the server, but lndk-cli also uses that same value. A better default value for the client would be localhost as ::1 doesn't always forward to localhost.