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

Auto-download over Tor #12094

Merged

Conversation

kiminuo
Copy link
Collaborator

@kiminuo kiminuo commented Dec 18, 2023

Depends on #12121
Fixes #8800
Follow-up to #8741

This PR adds auto-download feature over Tor. Currently, it works over the clearnet. To make the feature work, it was necessary to implement HTTP redirection support for Tor.

Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

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

The PR is quite hard to review because it bundles many changes but is really good.
I tested and could download through Tor without any issue.

What do you consider is missing for this feature? It seems to me that there is not much else to do other than remove debug code

I see no difficulty nor controversies in this PR, it's simply good. I guess you can remove debug code, and I will provide a more complete review

WalletWasabi/Services/UpdateManager.cs Outdated Show resolved Hide resolved
@kiminuo
Copy link
Collaborator Author

kiminuo commented Dec 21, 2023

The PR is quite hard to review because it bundles many changes but is really good.

I extracted some minor changes to #12121 to make this PR smaller.

…04-WW-AutoDownload

# Conflicts:
#	WalletWasabi.Daemon/Global.cs
#	WalletWasabi/Services/UpdateManager.cs
@kiminuo kiminuo changed the title [wip] Auto-download over Tor Auto-download over Tor Dec 21, 2023
@kiminuo
Copy link
Collaborator Author

kiminuo commented Dec 21, 2023

What do you consider is missing for this feature? It seems to me that there is not much else to do other than remove debug code

I removed the debug code. I'm not really aware of anything missing.

# Conflicts:
#	WalletWasabi.Daemon/Global.cs
@kiminuo kiminuo marked this pull request as ready for review December 21, 2023 21:12
@adamPetho
Copy link
Collaborator

An easy way to test this is to use this PR to download the new release, right?

@kiminuo
Copy link
Collaborator Author

kiminuo commented Dec 22, 2023

Yes, that's right.

@adamPetho
Copy link
Collaborator

Hmm, for me updating WW fails with:

2023-12-22 18:05:24.496 [10] ERROR      UpdateManager.UpdateChecker_UpdateStatusChangedAsync (91)       Getting new update failed with error. Exception: System.InvalidOperationException: Invalid wasabi signature.
   at WalletWasabi.Helpers.WasabiSignerHelpers.VerifySha256Sum(Byte[] sha256Hash, Byte[] signatureBytes) in WalletWasabi\Helpers\WasabiSignerHelpers.cs:line 46
   at WalletWasabi.Helpers.WasabiSignerHelpers.VerifySha256SumsFileAsync(String sha256SumsAscFilePath) in WalletWasabi\Helpers\WasabiSignerHelpers.cs:line 35
   at WalletWasabi.Services.UpdateManager.DownloadAndValidateWasabiSignatureAsync(String sha256SumsFilePath, String sha256SumsUrl, String wasabiSigUrl) in WalletWasabi\Services\UpdateManager.cs:line 242
   at WalletWasabi.Services.UpdateManager.GetInstallerAsync(Version targetVersion) in WalletWasabi\Services\UpdateManager.cs:line 115
   at WalletWasabi.Services.UpdateManager.UpdateChecker_UpdateStatusChangedAsync(Object sender, UpdateStatus updateStatus) in WalletWasabi\Services\UpdateManager.cs:line 77

On master, verifying the signature passes and the download starts.

@kiminuo
Copy link
Collaborator Author

kiminuo commented Dec 22, 2023

Hmm, for me updating WW fails with:

2023-12-22 18:05:24.496 [10] ERROR      UpdateManager.UpdateChecker_UpdateStatusChangedAsync (91)       Getting new update failed with error. Exception: System.InvalidOperationException: Invalid wasabi signature.
   at WalletWasabi.Helpers.WasabiSignerHelpers.VerifySha256Sum(Byte[] sha256Hash, Byte[] signatureBytes) in WalletWasabi\Helpers\WasabiSignerHelpers.cs:line 46
   at WalletWasabi.Helpers.WasabiSignerHelpers.VerifySha256SumsFileAsync(String sha256SumsAscFilePath) in WalletWasabi\Helpers\WasabiSignerHelpers.cs:line 35
   at WalletWasabi.Services.UpdateManager.DownloadAndValidateWasabiSignatureAsync(String sha256SumsFilePath, String sha256SumsUrl, String wasabiSigUrl) in WalletWasabi\Services\UpdateManager.cs:line 242
   at WalletWasabi.Services.UpdateManager.GetInstallerAsync(Version targetVersion) in WalletWasabi\Services\UpdateManager.cs:line 115
   at WalletWasabi.Services.UpdateManager.UpdateChecker_UpdateStatusChangedAsync(Object sender, UpdateStatus updateStatus) in WalletWasabi\Services\UpdateManager.cs:line 77

On master, verifying the signature passes and the download starts.

Right, there was extra BOM character in WalletWasabi\Client\Installer\SHA256SUMS.asc which changed the hash. Fixed by fafbcc3.

adamPetho
adamPetho previously approved these changes Dec 22, 2023
Copy link
Collaborator

@adamPetho adamPetho left a comment

Choose a reason for hiding this comment

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

tACK, I was able to download v2.0.5 over Tor with this PR.

For the code changes, I can only give a trustACK.

@kiminuo
Copy link
Collaborator Author

kiminuo commented Dec 22, 2023

@Kukks Friendly ping. I believe you were interested in "HTTP redirect support for Tor".

Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

tACK

Copy link
Collaborator

@MaxHillebrand MaxHillebrand left a comment

Choose a reason for hiding this comment

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

tACK 322284b

Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

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

The code is hard to review for me, but I went through it for quite some time and do not find clear mistakes.
I also tested and it works without any issue.

Should redirection also be tested? Or is the implementation common knowledge?

@kiminuo
Copy link
Collaborator Author

kiminuo commented Dec 23, 2023

Should redirection also be tested?

It is tested by downloading the file. But it can be tested also in other situations.

Or is the implementation common knowledge?

It's not "common knowledge". It's more like "HTTP protocol is not actually easy to implement" because RFCs are not totally complete regarding behavior (in edge/corner cases).

It's hard to test redirecting fully. So I did my best to implement it and I just hope it's OK. And then on the other hand, if we discover there is a bug, we will simply add a new unit test and fix it. We need redirecting only for this particular auto-download feature. @Kukks has other use cases, so maybe we'll colaboratively find out other needs.

@kiminuo kiminuo merged commit 0dfc168 into WalletWasabi:master Dec 23, 2023
6 of 7 checks passed
@kiminuo kiminuo deleted the feature/2023-12-04-WW-AutoDownload branch December 23, 2023 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add stream download option to IHttpClient
5 participants