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

TOR bridge + TOR Proxy + migration config_file_version #617

Merged

Conversation

deevope
Copy link
Contributor

@deevope deevope commented May 19, 2021

Solve #585

TOR Bridge

Requirements:

  • obfs4proxy binary installed and in the path

In case you have a bridge_line and lauch a command with --bridge argument, the bridge information from the cmd arguments will be proprietary and used instead of the one confirgured in the grin-wallet.toml

Wallet Configuration file

Must be in the following format "obfs4 [IP:PORT] [FINGERPRINT] cert=[CERT] iat-mode=[IAT-MODE]"

  • To enable, input the bridge info with the field bridge_line in grin-wallet.toml:
bridge_line = "obfs4 159.69.221.234:8080 3506E82EA71882C5F7C45911099C569E6C4C5376 cert=YttFtoyK5vcSbif3et94RnsIsgH+tHfGS8uILvaB668363aPK/0SngqJhmXt1xKTCE4Jcw iat-mode=0"
  • To disable, just comment it

CMD arguments

short : -g
long : --bridge

  • send command, input the bridge info after the arg --bridge
grin-wallet send -d grin1qyklgau87awekq9jzd2dn4pmm5k6c6enz66j607cyg2y5wdlpwws3z9xaf 42 --bridge "obfs4 159.69.221.234:8080 3506E82EA71882C5F7C45911099C569E6C4C5376 cert=YttFtoyK5vcSbif3et94RnsIsgH+tHfGS8uILvaB668363aPK/0SngqJhmXt1xKTCE4Jcw iat-mode=0"
  • listen command, input the bridge info after the arg --bridge :
grin-wallet listen --bridge "obfs4 159.69.221.234:8080 3506E82EA71882C5F7C45911099C569E6C4C5376 cert=YttFtoyK5vcSbif3et94RnsIsgH+tHfGS8uILvaB668363aPK/0SngqJhmXt1xKTCE4Jcw iat-mode=0"

Migration to config_file_version=2

Took another approach than what was done on the node side. Takes the old key-field value and migrate them in a new file and add the missing the new key-field with their default value. In this case: config_file_version = 2 and bridge_line = "". Support minimal config file.

@deevope deevope force-pushed the tor_bridge-migration_config_file branch 2 times, most recently from a2f2a92 to c8d1a5d Compare May 19, 2021 17:12
@deevope deevope force-pushed the tor_bridge-migration_config_file branch 4 times, most recently from fe7109a to e17761c Compare May 22, 2021 12:12
Copy link
Member

@j01tz j01tz left a comment

Choose a reason for hiding this comment

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

Thanks for your effort on this! Tor bridge support is important to improve privacy and circumvent censorship when building transactions.

Took a quick pass at the code, have a few general comments/questions:

  • Am still concerned a bit about generating a new config and migrating values as some custom changes would be lost, maybe without the user realizing it will happen. Maybe someone else can weigh in here. Is there a simpler way to handle this? Maybe by expecting the bridge config line and if missing just append it to the end of the config file or something? Or am I missing something for why that can't work cleanly? Would migrating config files in this way be more or less likely to break things than appending lines to configs with potentially unknown locations/contents?

  • The logic seems to only support obfs4, it might be nice to make things more generic to support multiple bridge proxy protocols? Or make the language more clear that it only supports obfs4 bridges? Bringing this up because for example some users will use shadowsocks instead of obfs4 because they cannot obtain unblocked obfs4 bridges easily (note: while obfs4 protocol itself works fairly well behind GFW, it can be hard to obtain bridge IPs that are not already blocked by GFW..). Maybe this is meant for a future update, but at some point we may want something more of a generic tor_bridge_proxy which can support multiple protocols like obfs4, shadowsocks etc.

Once we decide how we want to address the above (if at all!) will make a more thorough pass.

This week I will test thoroughly on linux, it would be great if others could pick up testing on osx and windows.

Thanks again for putting this together @deevope!

@phyro
Copy link
Member

phyro commented Jun 7, 2021

@deevope any comments on the above?
@j01tz did you manage to find some time for testing this on linux?

@j01tz
Copy link
Member

j01tz commented Jun 8, 2021

@j01tz did you manage to find some time for testing this on linux?

No, I think deevope was planning quite a few changes and so holding off on testing until they are ready. I think deevope is maybe away for travel atm

@deevope
Copy link
Contributor Author

deevope commented Dec 26, 2021

@j01tz @phyro Deeply sorry for the delay.
However, now it supports: obfs4, snowflake, meek_lite and tor proxy in general so shadowsocks as well. Parsing a lot the input from the user to avoid little mistake also.
For the config migration, I tried to let as much as possible the comments of the old config. the only comment that will be not included in the new migration file will those attached to the key logging. Which is really minor and the best we can do to keep backward and forward compatibility imo. Would be easier in the future to add new fields in the toml and get them directly printed in the grin-wallet config.

@deevope deevope requested a review from j01tz December 26, 2021 12:04
@deevope deevope changed the title TOR bridge + migration config_file_version TOR bridge + TOR Proxy + migration config_file_version Dec 26, 2021
@deevope deevope force-pushed the tor_bridge-migration_config_file branch from 6d3ce16 to b23c9ed Compare December 26, 2021 12:28
if let Some(t) = tb.transport {
let transport = t.to_lowercase();
match transport.as_str() {
"socks4" | "socks5" | "http" | "https" | "http(s)" => {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to include 'socks5h' here? (See #629)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The match that you commented, is just in order to parse the user input in the config file for him to choose which type of proxy he want to set. You could refer to the tor browser settings for reference. And write accordingly in the torrc config the transport, cf https://github.com/mimblewimble/grin-wallet/pull/617/files#diff-260d6fd89f30bf49927a33dffd4a77eb74479cb3b919f1f818478d1db8fa1e78R115-R144

@deevope deevope force-pushed the tor_bridge-migration_config_file branch from 62f9763 to eb68a20 Compare January 2, 2022 14:23
@yeastplume
Copy link
Member

Still on radar, just need a day or two to check this out properly and test the configuration migration and new options, etc!

@yeastplume
Copy link
Member

All appears fine, at least in local testing on my machine, and thanks for doing this; this is really important.

I can't guarantee this will work for all users at the moment, but we don't have a planned release for the wallet just yet, so there will be plenty of time to test this out on master and fix any issues.

@yeastplume yeastplume merged commit c424a0e into mimblewimble:master Feb 3, 2022
bayk added a commit to mwcproject/mwc-wallet that referenced this pull request Aug 13, 2024
 config_file_version (mimblewimble#617)
* tor bridge config and args
* migration `config_file_version=2`
* small fixes typo, comment etc..
* support: snowflake, meek_lite, obsf4 and tor proxy
* remove useless serde
* improve migrate function
* few fixes
* add bridge flags to pay and receive + few fixes
* some improvements
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