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.

Sign up for GitHub

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

Jump to bottom

server+tor: add support for Tor HASHEDPASSWORD authentication method #4048

Merged
merged 1 commit into from
Mar 10, 2020
Merged

server+tor: add support for Tor HASHEDPASSWORD authentication method #4048

merged 1 commit into from
Mar 10, 2020

Conversation

wpaulino
Copy link
Contributor

@wpaulino wpaulino commented Mar 5, 2020

Tested and confirmed locally that all three authentication methods (SAFECOOKIE, HASHEDPASSWORD, NULL) work as intended. SAFECOOKIE remains as the default unless a control password is provided.

Replaces #2548.

@wpaulino wpaulino requested review from carlaKC and removed request for Roasbeef and cfromknecht March 5, 2020 00:31
@wpaulino wpaulino added this to the 0.10.0 milestone Mar 5, 2020
@wpaulino wpaulino added enhancement Improvements to existing features / behaviour tor labels Mar 5, 2020
meeDamian
meeDamian reviewed Mar 5, 2020
View reviewed changes
Copy link
Contributor

@meeDamian meeDamian left a comment

Choose a reason for hiding this comment

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

Only a single nit, other than that really looking forward to this one! 😁

tor/controller.go Outdated Show resolved Hide resolved
wereHamster
wereHamster reviewed Mar 5, 2020
View reviewed changes
tor/controller.go Outdated Show resolved Hide resolved
carlaKC
carlaKC approved these changes Mar 9, 2020
View reviewed changes
Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Ran locally and works as expected, LGTM!

Would be cool to update docs/configuring_tor.md to indicate that there's an option other than safecookie available. Should be as simple as noting the tor config change and tor.password flag but not a blocker.

@wpaulino
Copy link
Contributor Author

wpaulino commented Mar 9, 2020

Would be cool to update docs/configuring_tor.md to indicate that there's an option other than safecookie available.

@carlaKC added!

@wpaulino wpaulino requested a review from carlaKC March 9, 2020 21:56
@Roasbeef Roasbeef requested a review from Crypt-iQ March 10, 2020 00:19
carlaKC
carlaKC approved these changes Mar 10, 2020
View reviewed changes
Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

added!

Nice, still LGTM 😄

Crypt-iQ
Crypt-iQ approved these changes Mar 10, 2020
View reviewed changes
Copy link
Collaborator

@Crypt-iQ Crypt-iQ left a comment

Choose a reason for hiding this comment

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

Tested and works as expected! Should mention how to get the HashedControlPassword, other than that LGTM.

docs/configuring_tor.md Show resolved Hide resolved
tor/controller.go Outdated Show resolved Hide resolved
@wereHamster @wpaulino
server+tor: add support for Tor HASHEDPASSWORD authentication method
018e8b5 
This provides users an alternative over the SAFECOOKIE authentication
method, which may not be as useful if users are connecting to a remote
Tor sevrer due to lnd not being able to retrieve the cookie file.
@wpaulino wpaulino merged commit e17ad8b into lightningnetwork:master Mar 10, 2020
@wpaulino wpaulino deleted the tor-hashed-password branch March 10, 2020 18:12
@guggero guggero mentioned this pull request Apr 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wereHamster wereHamster wereHamster left review comments

@meeDamian meeDamian meeDamian approved these changes

@Crypt-iQ Crypt-iQ Crypt-iQ approved these changes

@carlaKC carlaKC carlaKC approved these changes

Assignees
No one assigned
Labels
enhancement Improvements to existing features / behaviour tor
Projects
None yet
Milestone
0.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@wpaulino @wereHamster @meeDamian @Crypt-iQ @carlaKC