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

Setup grpc server #104

Merged
merged 11 commits into from
Jun 5, 2024
Merged

Setup grpc server #104

merged 11 commits into from
Jun 5, 2024

Conversation

orbitalturtle
Copy link
Collaborator

@orbitalturtle orbitalturtle commented May 6, 2024

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:

  • The first few commits are preparatory to pave the way for the grpc server.
  • To interact with LND we need the user to pass in their LND macaroon. We use grpc metadata for this to make it easy to tag this on to each API call.
  • Updates the CLI to connect to the server.

docs/grpc_client.md Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@mrfelton
Copy link
Contributor

mrfelton commented May 7, 2024

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?

proto/offers.proto Outdated Show resolved Hide resolved
@orbitalturtle
Copy link
Collaborator Author

@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
Copy link
Contributor

mrfelton commented May 7, 2024

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.

proto/offers.proto Outdated Show resolved Hide resolved
@mrfelton
Copy link
Contributor

mrfelton commented May 12, 2024

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.

docs/grpc_client.md Outdated Show resolved Hide resolved
src/server.rs Outdated Show resolved Hide resolved
src/server.rs Outdated Show resolved Hide resolved
This commit Arc-ifies the reference to OffersHandler, which is necessary
because once we set up the grpc server, we'll need a second thread-safe
reference to it.
This commit removes the RefCell in MessengerUtilities, which hampers us from
being able to reference OfferHandler in two different tokio tasks, which we'll
need to do when we set up our grpc server.
We move logger setup into main, so we're able to access the logger when we
start up the server.
With the grpc server, we don't need to check whether the onion messenger has
started before we pay an offer - we know it's already running.
That way, we don't get a "AlreadyProcessing" error if the client attempts to
pay the offer a second time.
@orbitalturtle orbitalturtle force-pushed the grpc-server branch 3 times, most recently from e424956 to a3ed7dd Compare May 29, 2024 05:03
This updates the CLI to connect to the LNDK gRPC server in order to make API
calls. We add the necessary macaroon metadata to each request. In addition,
we remove the cert and address flag options because we no longer require
passing in an LND certificate and address to connect to LND.
@orbitalturtle orbitalturtle merged commit b2586d7 into master Jun 5, 2024
4 checks passed
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.

2 participants