Skip to content
This repository has been archived by the owner on Feb 23, 2021. It is now read-only.

Enable macaroons #552

Merged
merged 5 commits into from
Aug 24, 2018
Merged

Enable macaroons #552

merged 5 commits into from
Aug 24, 2018

Conversation

valentinewallace
Copy link
Contributor

Due to a security vulnerability, macaroons should always be enabled within the app.

ghost
ghost approved these changes Aug 23, 2018
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this out with electron-dev and packaged app - works as advertised. LGTM 👍

public/grpc-client.js Outdated
function getMacaroonCreds(lndSettingsDir) {
return grpc.credentials.createFromMetadataGenerator(function(args, callback) {
const metadata = new grpc.Metadata();
const macaroonPath = path.join(lndSettingsDir, 'admin.macaroon');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting that soon the macaroons will be stored in the network directory. Do we want to hold off on this PR until this happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When that happens, the fix should just be a path update

test/integration/action/action-integration.spec.js Outdated
@@ -226,6 +221,7 @@ describe('Action Integration Tests', function() {

it('should fund wallet for node1', async () => {
btcdProcess.kill();
await nap(NAP_TIME);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flaky test :D?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, fixed thx

@valentinewallace
Copy link
Contributor Author

valentinewallace commented Aug 24, 2018 •
edited
Loading

Build's failing because we're not using the latest lnd on travis, will update once I figure out what the best commit would be.

tanx
tanx suggested changes Aug 24, 2018
View reviewed changes
public/grpc-client.js
credentials = grpc.credentials.combineChannelCredentials(
credentials,
macaroonCreds
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the locker doesn't need macaroons? Only the lnrpc.Lightning service?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the wallet hasn't been initialized, the macaroons don't exist yet. The wallet creation process involves creating the macaroons :)

public/grpc-client.js
const macaroonHex = fs.readFileSync(macaroonPath).toString('hex');
metadata.add('macaroon', macaroonHex);
callback(null, metadata);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work with return if it's asynchronous? Don't we need to wait for the callback to set the metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to combine the ssl credentials with the macaroon metadata, we need to turn the metadata into a CallCredentials object.

createFromMetadataGenerator requires an metadata generator (the asynchronous function) and returns a CallCredentials object that can be composed with the ssl ChannelCredentials object: googleapis/google-cloud-node#1346 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Thanks for the explanation.

public/grpc-client.js Outdated
metadata.add('macaroon', macaroonHex);
return metadata;
function getMacaroonCreds(lndSettingsDir, network) {
return grpc.credentials.createFromMetadataGenerator(function(args, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use array function (args, callback) => here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

public/grpc-client.js
credentials = grpc.credentials.combineChannelCredentials(
credentials,
macaroonCreds
);
lnd = new lnrpc.Lightning(`localhost:${lndPort}`, credentials);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we don't need to pass the metadata in each api call because we inject the credentials here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, thought it simplified the code a bit to not have to remember to pass in metadata each time.

Copy link
Contributor Author

@valentinewallace valentinewallace Aug 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we stayed with passing in metadata each time, we'd also have to get the macaroons to action/grpc and pass them into the Duplex here: https://github.com/lightninglabs/lightning-app/blob/master/src/action/grpc.js#L96 or else stream writes fail due to missing macaroons.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok great 👍

src/config.js
@@ -9,7 +9,7 @@ module.exports.RATE_DELAY = 15 * 60 * 1000;

module.exports.LND_PORT = 10009;
module.exports.LND_PEER_PORT = 10019;
module.exports.MACAROONS_ENABLED = false;
module.exports.NETWORK = 'testnet';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we're in dev mode? Then the app will start with testnet instead of simnet?

assets/script/setup_local_cluster.md

lncli --rpcserver=localhost:10009 --no-macaroons --lnddir=data/lnd sendpayment --pay_req=ENCODED_INVOICE
lncli --rpcserver=localhost:10009 --lnddir=data/lnd sendpayment --pay_req=ENCODED_INVOICE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice update to the docs 👍

tanx
tanx suggested changes Aug 24, 2018
View reviewed changes
public/electron.js Outdated
@@ -123,7 +123,7 @@ function createWindow() {
ipcMain,
lndSettingsDir,
lndPort: LND_PORT,
macaroonsEnabled: MACAROONS_ENABLED,
network: NETWORK,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

network: isDev ? 'simnet' : NETWORK
Else the app will use 'testnet' in dev mode as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah missed that, fixed

@valentinewallace
Fix path to macaroon file due to lnd changes
4a3409a 
Now, each network has its own macaroons.
@tanx tanx merged commit a1c9c4c into master Aug 24, 2018
@tanx tanx deleted the enable-macaroons branch August 24, 2018 20:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tanx tanx tanx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@valentinewallace @tanx