macaroon_service: add MacaroonService #86
Conversation
ellemouton
force-pushed
the
macaroonService
branch
2 times, most recently
from
e5b4d7d to
788566e
Compare
guggero
left a comment
guggero
left a comment
There was a problem hiding this comment.
Very nice, this'll allow us to remove this same code from three other projects 🎉
Looks pretty good, just a few minor comments.
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
Looks good!
Would just bump one of the dependent services to use this before merge to test it out. There's inevitably something small that needs fixing, so nice to see before merge.
ellemouton
force-pushed
the
macaroonService
branch
from
788566e to
18d7a7c
Compare
guggero
left a comment
guggero
left a comment
There was a problem hiding this comment.
One last comment about migrating away from an empty password. That might be worthy of a unit test too.
macaroon_service.go
Outdated
| cfg: cfg, | ||
| } | ||
|
|
||
| if len(cfg.DBPassword) == 0 { |
There was a problem hiding this comment.
Should be != 0. Though looking at the Pool PR, do we need a way to migrate the DB to a non-empty password?
We could add a special case here. If we have an empty password but all the other variables set (lnd client, ephemeral key and key locator), we first try to unlock with the shared key (in case migration was successful already). If that fails, try to unlock with an empty password. If that succeeds, change the password to the shared key.
Though that would make this a breaking change for Loop/Pool/Faraday, as users couldn't go back to an older version after migrating the DB. I think that is fine, since they could just delete the macaroon DB if they really needed to go back to a previous version.
There was a problem hiding this comment.
Should be != 0
right. i just realised that the reason i initially had it as if cfg.DBPassword != nil was cause currently pool & loop set zero len (but non-nil) db passwords here.
If we have an empty password but all the other variables set (lnd client, ephemeral key and key locator), we first try to unlock with the shared key (in case migration was successful already). If that fails, try to unlock with an empty password. If that succeeds, change the password to the shared key.
cool, sounds good!
ellemouton
force-pushed
the
macaroonService
branch
from
18d7a7c to
6a16363
Compare
|
Thanks for the reviews @carlaKC & @guggero 🚀 I have updated it as @guggero has suggested to allow for migration and also added a unit test for this. However, now it does not allow the client to have an empty passphrase at all. Is that wanted? or do we still want to accommodate that case? if so, i can easily update it to include that. I will re-request review once i have tested the updates properly |
ellemouton
force-pushed
the
macaroonService
branch
from
6a16363 to
c91e9bf
Compare
Yes, I think a passphrase should be mandatory so the DB is encrypted. Either it's supplied as a string or derived through the shared secret. |
ellemouton
force-pushed
the
macaroonService
branch
2 times, most recently
from
19b644c to
0ba1292
Compare
macaroon_service.go
Outdated
| nBytes, _ = hex.DecodeString( | ||
| "0215b5a3e0ef58b101431e1e513dd017d1018b420bd2e89dcd71f45c031f00469e", | ||
| ) | ||
| MacaroonPubKey, _ = btcec.ParsePubKey(nBytes, btcec.S256()) |
There was a problem hiding this comment.
better name for this perhaps? 😅
There was a problem hiding this comment.
How about SharedKeyNUMS (=nothing up my sleeve, nobody knows the private key for this point)?
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
LGTM, tested with loop PR and all in order 👌
guggero
left a comment
guggero
left a comment
There was a problem hiding this comment.
Very nice work on this, turned out great!
Two last nits, then we can get this merged 🎉
macaroon_service.go
Outdated
| nBytes, _ = hex.DecodeString( | ||
| "0215b5a3e0ef58b101431e1e513dd017d1018b420bd2e89dcd71f45c031f00469e", | ||
| ) | ||
| MacaroonPubKey, _ = btcec.ParsePubKey(nBytes, btcec.S256()) |
There was a problem hiding this comment.
How about SharedKeyNUMS (=nothing up my sleeve, nobody knows the private key for this point)?
This adds a MacaroonService which can be used by other servers (Loop, Pool, Faraday and Lit) to setup a macaroon db, create default macaroons etc.
ellemouton
force-pushed
the
macaroonService
branch
from
0ba1292 to
d021bdf
Compare
|
Shweeeet, thanks @guggero 🚀 updated 👍 |
|
This PR doesn't use anything that was only introduced in 0.14.2, so 0.14.0 is correct. |
3 participants
Add MacaroonService. Loop, Pool, Faraday and now Lit all need to spin up their own macaroon services and currently the logic is just duplicated in each repo. So this commit aims to consolidate all that logic into one place.
With this commit, a caller can choose to encrypt their macaroon.db file with a password of their choosing or they can specify that instead a shared key should be derived with LND which will then be used to encrypt the db.