-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Use https:// for XMR explorer API endpoints, except if localhost or Tor #4492
Conversation
wiz
commented
Sep 7, 2020
•
edited
Loading
edited
- If Tor *.onion hostname, use HTTP with Tor proxy
- If 127.0.0.1 or localhost, use HTTP without Tor proxy
- If LAN address or *.local FQDN, use HTTP without Tor proxy
- If any other FQDN hostname, use HTTPS with Tor proxy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, suggested change to simplify: https://
prefix included in URL and then only need to detect localhost so as to disable socks5 proxy. Regex changed to allow optional port specifier. Defaults needs changing to include the https. @Emzy service to use https also?
httpClient.setBaseUrl(model.getServiceAddress());
if (model.getServiceAddress().matches(".*127.0.0.1.*|.*.local.*|.*localhost.*")) {
log.info("Ignoring Socks5 proxy for local net address: {}", model.getServiceAddress());
httpClient.setIgnoreSocks5Proxy(true);
}
@jmacxx after thinking about it a bit more, that still isn't nearly enough to cover all the possible cases we need to support. let me work on some new regex. |
95186c9
to
82c6aaa
Compare
82c6aaa
to
03451d3
Compare
utACK Is there not a simpler way to detect a private IP address space (LAN address)? |
Problem: each time the user edits a url then restarts bisq, modify the url again the prefix is prepended, ends up like |
maybe easiest to just add the https to the textfield at input. |
core/src/main/java/bisq/core/trade/txproof/xmr/XmrTxProofRequest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/bisq/core/trade/txproof/xmr/XmrTxProofRequest.java
Outdated
Show resolved
Hide resolved
7e916e6
to
1c45a9c
Compare
* If Tor *.onion hostname, use HTTP with Tor proxy * If 127.0.0.1 or localhost, use HTTP without Tor proxy * If LAN address or *.local FQDN, use HTTP without Tor proxy * If any other FQDN hostname, use HTTPS with Tor proxy
1c45a9c
to
0869f9a
Compare
@jmacxx good catch, I forgot to strip the protocol prefix out, just pushed a commit that should fix that issue. I should have tested more, but I was trying to rush this out. @chimp1984 maybe, and there is also the decision to route the connection through Tor or not, which would require its own slider switch - using regex we can strictly enforce https or Tor onion except for localhost or LAN ip address. IMO the goal is to let user be able to add xmrchain.net or whatever public explorers they want to trust, but without letting them accidentally put http:// and expose them to MITM attacks. I'm also going to tweak the strings a bit with @m52go, since "Service addresses" is a big vague and user might be confused what exactly to enter. To be honest this feature feels like it needs a lot more testing and polish before we ship it in a public release to users... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will review tomorrow, was out the whole day.... |
@jmacxx thanks for testing with your http:// node on LAN with custom port number, this is exactly what we needed. I suppose a good test is to paste in something like this and watch how it auto-parses it:
|
@chimp1984 after this PR, what do you think about making the list of Monero Explorers into a multi-line combobox like the ones for altcoins and fiat below? we could have a list of hard-coded ones enabled by default, and allow the user to enter their own into the list, would look nicer than a comma delimitated text input field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks fine to me. Can't say the same for the regexes, but they might well be correct, I can't tell. If I can get another ack for regexes I'm fine with this PR. @Emzy that seems to be your forte :)
@wiz Which one are candidates? We could add one or 2 by default for more security. Only potential issue is that more services we use the higher the chance that one has a network issue and then auto-conf fails. Beside that, are those offering onion addresses and if not are they supporting Tor connections (no captcha)? I think a UI list is a bit too much for that feature as it would also overload more the settings UI as it is already... |
I agree, and Devin is going to setup a 3rd node instance for us so we should be fine for now. But that is not related to this PR i suppose, and we can think about it again later. |
utACK
The regexes and the networks to be local addresses look right. But I'm not a JAVA dev. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK