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

display a warning if the user use --proxy and hidden service and not … #3249

Closed
wants to merge 1 commit into from

Conversation

Saibato
Copy link
Contributor

@Saibato Saibato commented Nov 8, 2019

…disables dns

As @bitcoin-software reported the default behavior is in UX therms
not what the users expects if --proxy is configured. Users tent to associate
with the term proxy, that all traffic is routed trough the proxy.

We should warn the user also in the logs and on startup, if he not selects also --use-always-proxy=true
and --disable-dns, when having defined and used also a hidden service.

The default behavior is that ipv4/6 FQDN will be resolved locally,
since we use tor socks5 proxy for onion address with the socks4a improvement,
the user could and should let Tor do the dns.

Changelog-Changed: Warn the users that he had configed a possible tor dns/ip leak

@Saibato Saibato changed the title display a warning is the user use --proxy and hidden service and not … display a warning if the user use --proxy and hidden service and not … Nov 8, 2019
@Saibato Saibato force-pushed the warn-proxy branch 3 times, most recently from 1be1379 to b4d1b98 Compare November 8, 2019 16:50
@Saibato Saibato marked this pull request as ready for review November 8, 2019 19:56
@darosior
Copy link
Collaborator

darosior commented Nov 8, 2019

--use-always-proxy=true and --disable-dns

What do you think of disabling DNS if --use-always-proxy=true ?


if (ld->proxyaddr && (!ld->use_proxy_always || ld->config.use_dns)) {
if (tal_count(ld->proposed_wireaddr) != 0
&& some_tor_addresses(ld->proposed_wireaddr)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation :-)

Copy link
Contributor Author

@Saibato Saibato Nov 8, 2019

Choose a reason for hiding this comment

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

What do you think of disabling DNS if --use-always-proxy=true ?

actually doing the follow PR right now

`if (!daemon->use_proxy_always)
if (broken_resolver(daemon)) {
or so ...

Then all is according to the docu of the flag`


if (ld->proxyaddr && (!ld->use_proxy_always || ld->config.use_dns)) {
if (tal_count(ld->proposed_wireaddr) != 0 && some_tor_addresses(ld->proposed_wireaddr)) {
log_unusual(ld->log, "WARNING: Your Tor proxy setup with use of hidden service address"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
log_unusual(ld->log, "WARNING: Your Tor proxy setup with use of hidden service address"
log_unusual(ld->log, "WARNING: Your Tor proxy setup with use of hidden service address"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:-)

Copy link
Collaborator

@darosior darosior left a comment

Choose a reason for hiding this comment

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

ACK a954dee

Copy link
Contributor

@gorazdko gorazdko left a comment

Choose a reason for hiding this comment

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

Warning and changelog text could be grammatically correct. Possible improvements:

  • WARNING: Your Tor proxy setup with hidden service address, --always-use-proxy=true and --disable-dns is not recommended. You have configured a possible dns leak of your hidden service. Please enable both options, we hope you know what you are doing!

  • Changelog-Changed: Warn users about a possible Tor dns/ip leak

But im not a native english speaker either. Meybe they could jump in...

if (ld->proxyaddr && (!ld->use_proxy_always || ld->config.use_dns)) {
if (tal_count(ld->proposed_wireaddr) != 0 && some_tor_addresses(ld->proposed_wireaddr)) {
log_unusual(ld->log, "WARNING: Your Tor proxy setup with use of hidden service address"
" and --allways-use-proxy=%s and --disable-dns=%s"
Copy link
Contributor

Choose a reason for hiding this comment

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

spelling: --always-use-proxy, --disable-dns (without argument)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WARNING: Your Tor setup use a hidden service address --always-use-proxy=false and --disable-dns not set is not recommended, please set --always-use-proxy=true and use --disable-dns, we hope you know what you are doing!
c-lightning try to help you and detected that you have configured a possible dns leak of your hidden service

Copy link
Contributor

@gorazdko gorazdko Nov 10, 2019

Choose a reason for hiding this comment

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

Thanks, much more understandable.

WARNING: Your Tor setup uses a hidden service address with --always-use-proxy=false and without --disable-dns which is not recommended. Please set --always-use-proxy=true and use --disable-dns, we hope you know what you are doing!
c-lightning is trying to help you because it detected that you have configured a possible dns leak of your hidden service.

Copy link
Contributor Author

@Saibato Saibato Nov 10, 2019

Choose a reason for hiding this comment

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

Thank you too, you caught one, but now its enough for me, since I fear that ends in tautologies.
Some say I am as polite and hansom as a satoshi, but I clearly differ when there is no real progress, then I tent to get rude and bossy.

Here is the bossy unfair mean and I feel a shame for exscript of the original draft of this PR and some hints for the next party, where some caught the front row tick tick tok

Cybermen have tagged your system for upgrade by low r value and the doctor is out of town ...

Copy link
Contributor

Choose a reason for hiding this comment

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

There was no ill intent in my posts. We can leave it as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

keep CALM, You are clearly welcome and even now there is one mistype, if you insist, I will change the p to P to make you happy

Copy link
Contributor

Choose a reason for hiding this comment

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

i dont insist on anything - its your call. And this PR does not affect my feelings whatsoever.

…disables dns

@bitcoin-software reported the default behavior is in UX therms
not what the users expects if --proxy is configured. Users tent to associate
with the term proxy, that all traffic is routed trough the proxy.

We should warn the user, if he not selects also --use-always-proxy=true
and disable-dns, when having defined and used also a hidden-service.

The default behavior is that ipv4/6 FQDN will be resolved locally,
since we use tor socks5 proxy for onion address with the socks4a improvement
we could and should let Tor do the dns.

Changelog-Changed: Warn users if they about to config a possible dns leak

Signed-off-by: Saibato <[email protected]>
@rustyrussell
Copy link
Contributor

Hmm, if this change is desired, we should go further:

  1. Deprecate --always-use-proxy.
  2. Add --proxy-tor-only.
  3. Change default to be always-use-proxy.

That's pretty clear, I think? Personally, I want my node to be able to reach nodes on Tor, but I don't need most traffic over it.

@Saibato
Copy link
Contributor Author

Saibato commented Nov 11, 2019

That's pretty clear, I think? Personally, I want my node to be able to reach nodes on Tor, but I don't need most traffic over it.

Me too, AFAICS the new logo of Tor is "Take back the internet with Tor" , c-ln's Tor implementation is unique in LN space ( gretz and thx to MIT and hobbit ), since we have this true quiet mode ( if we want ). If Tor is the default and all have a this semi unique Tor address bound to node id fine, most traffic from node to node will be intern relayed and never leave Tor.
Exitspoints are on the other hand a single point of failure.

I agree to reach Ip address directly should never be removed, we could add a random jitter between the hopps and the first exit to ip that should be enough, to not called out as doxy app.

In so far, this PR must not be applied, if we are crystal clear in documentation and defaults, what we try to do here.

We are in early stages and most users up till now don't need this warning, since they are mostly deep techie and know.
C-ln will probably be a major transport layer and at some point default will be the norm, so ...

For historical purpose, I personally would like the discussion here and not in semi hidden telegram or signal channels, since such channeled unopen discussion modes are often the begin of control structures applied.

I like your suggestion of --proxy-tor-only and to change the defaults of proxy, how about --add --tor= soly for tor socks ?. And have --proxy for general socks4/5 proxy ?

I push a WIP --proxy-tor-only and to change the defaults, today that could trigger this discussion further.
And one for --random-jitter for all packets might be better work than use of dandelon , but that almost yields in research space. Sigh..
Please feel free to speed me out and take the lead in this, since that matter is also your home turf.

@Saibato
Copy link
Contributor Author

Saibato commented Nov 16, 2019

With #3251 👍 merged and #3254 on it"s way I close this now, to defer priority to other issue. This is now low prio and I don't want to be part of pr inflation and review lag, since attention defers can also be a kind of vector programing ... thx for review, I close this now

@Saibato Saibato closed this Nov 16, 2019
@cdecker
Copy link
Member

cdecker commented Nov 17, 2019

With #3251 merged and #3254 on it"s way I close this now, to defer priority to other issue. This is now low prio and I don't want to be part of pr inflation and review lag, since attention defers can also be a kind of vector programing ... thx for review, I close this now

Not at all, thanks for the PR, and feel free to reopen later or file an issue to track progress 👍

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.

5 participants