Skip to content

Conversation

@jalavosus
Copy link

This PR allows an instantiation of LndServicesConfig to use raw macaroon data (passed as bytes or base64-encoded strings) as well as raw TLS certificate data (passed as bytes), allowing for LndServicesConfig to be used programmatically using loaded kubernetes secrets, hardcoded macaroons/certs, etc.

Pull Request Checklist

  • PR is opened against correct version branch.
  • Version compatibility matrix in the README and minimal required version
    in lnd_services.go are updated.

Passing RawMacaroons as a param to LndServicesConfig allows for
using macaroon data loaded from files/kubernetes secrets/etc. instead of
relying on all macaroons being present in a specific directory. This is
especially useful when running in containers or kubernetes pods.
Passing RawTLS allows a user to provide the raw TLS Cert bytes for their
LND instance, instead of relying on a hardcoded file/filepath.
@guggero guggero self-requested a review December 30, 2020 20:27
@guggero
Copy link
Contributor

guggero commented Jan 4, 2021

Thanks for the PR!
This is definitely something that can be very useful and I think we should add the feature.
Though I'm sure you have a specific use case in mind, therefore my question before I do a full review:
Would it not be enough to specify one raw macaroon and call it RawCustomMacaroon? That would simplify things quite a bit. You'd need to either pass the raw admin macaroon or bake a custom one, but that's where we want to head anyway (no more multiple macaroon files as they're tedious to handle).

Copy link

@torkelrogstad torkelrogstad left a comment

Choose a reason for hiding this comment

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

Tested that I can connect to LND by passing in []byte values.

@jalavosus
Copy link
Author

Would it not be enough to specify one raw macaroon and call it RawCustomMacaroon? That would simplify things quite a bit.

I actually like this idea a lot, and would make certain use cases much easier to implement!

This enables far fewer switch statements, as well as negating the need
for a new custom struct type to be passed.

The CustomMacaroon parameter expects that the passed Macaroon bytes are
either the contents of admin.macaroon, or a self-baked macaroon with all
necessary client permissions.
@jalavosus
Copy link
Author

@guggero and @torkelrogstad, I just pushed a slight refactor to this PR -- instead of using a self-rolled RawMacaroon type, LndServicesConfig takes a CustomMacaroon parameter, which is the raw []byte slice of either admin.macaroon or a self-baked macaroon with all necessary client permissions.

@guggero
Copy link
Contributor

guggero commented Jan 18, 2021

Concept ACK, thanks for the changes!

The PR needs a rebase. It would also be great if you could clean up the commit structure and split the changes into small and logical commits. There are also a few style nits that should be addressed.
Please see https://github.com/lightningnetwork/lnd/blob/master/docs/code_contribution_guidelines.md as we try to adhere to those rules in all our projects.

@guggero guggero removed their request for review February 16, 2021 08:15
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Looks pretty good, needs a squash and rebase :)

default:
return nil, fmt.Errorf("unsupported network: %v",
cfg.Network)
if cfg.CustomMacaroon == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: reverse this check, check non-nil path instead

if cfg.CustomMacaroon == nil {
var loadMacErr error

readonlyMac, loadMacErr = loadMacaroon(
Copy link
Contributor

Choose a reason for hiding this comment

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

should move this into the above if

@guggero
Copy link
Contributor

guggero commented Oct 25, 2021

Replaced by #51.

@guggero guggero closed this Oct 25, 2021
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.

4 participants