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

Autotor torport fix #4603

Merged
merged 2 commits into from
Jun 17, 2021
Merged

Autotor torport fix #4603

merged 2 commits into from
Jun 17, 2021

Conversation

rustyrussell
Copy link
Contributor

We were accidentally using the port that the tor service was connecting to, not the /torport the user said to use.

Fixes: #4597

@rustyrussell
common/test: test wireaddr parsing.
8e57fa4 
In case this was the issue (it wasn't)!.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
connectd: fix advertizing for Tor ports.
5943319 
We were accidentally using the port that the tor service was
connecting to, not the /torport the user said to use.

Fixes: ElementsProject#4597
Reported-by: @openoms
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: Config: `addr` autotor and statictor /torport arguments now advertized correctly.
@rustyrussell rustyrussell added this to the v0.10.1 milestone Jun 16, 2021
@Saibato
Copy link
Contributor

Saibato commented Jun 16, 2021

hmm... would that not also apply to autotor? I wonder what i reasoned about to use local->port and not port for static and blobs and autotor. Maybe some undesired privacy side effects?
I guess to demand defining the advertised and used port with /torport or to more precise the doc might also fix this.
Tor can be configured external bind to any port or the default and use only internal the local.
It should have been possible in the issue case to reach the node over 9735, obliviously we should not announce the intern port, hmm...

vincenzopalazzo
vincenzopalazzo approved these changes Jun 16, 2021
View reviewed changes
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

LGTM.

ack 5943319

@rustyrussell
Copy link
Contributor Author

hmm... would that not also apply to autotor? I wonder what i reasoned about to use local->port and not port for static and blobs and autotor. Maybe some undesired privacy side effects?

Yes, this fix covers both.

I guess to demand defining the advertised and used port with /torport or to more precise the doc might also fix this.
Tor can be configured external bind to any port or the default and use only internal the local.
It should have been possible in the issue case to reach the node over 9735, obliviously we should not announce the intern port, hmm...

Yes, /torport was what we told Tor to listen on, but we didn't actually advertize that! So this is clearly wrong.

@rustyrussell rustyrussell merged commit 976a790 into ElementsProject:master Jun 17, 2021
@openoms openoms mentioned this pull request Oct 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincenzopalazzo vincenzopalazzo vincenzopalazzo approved these changes

Assignees
No one assigned
Labels
bug gossip tor
Projects
None yet
Milestone
v0.10.1
Development

Successfully merging this pull request may close these issues.

Tor settings: default port is advertised despite specific bind-addr setting
3 participants
@rustyrussell @Saibato @vincenzopalazzo