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

Add ignore_hash option in settings.ini #684

Merged
merged 11 commits into from
Nov 5, 2023

Conversation

lebarsfa
Copy link
Contributor

Based on the work of @mardy and as a quick workaround for some problems described in #521 and #224, here is a proposition to add ignore_hash option in settings.ini. When set to True, sha256 hashes would not be checked.

mardy and others added 4 commits December 23, 2022 18:24
This allows aqt to work even when the server download.qt.io is
unreachable.

Signed-off-by: Alberto Mardegan <mardy@users.sourceforge.net>
@miurahr miurahr self-requested a review May 30, 2023 03:18
@miurahr miurahr added the enhancement New feature or request label May 30, 2023
@ddalcino
Copy link
Contributor

ddalcino commented May 30, 2023

You could save yourself and a lot of users some trouble by checking the sha1 hash (just pass "sha1" to the get_hash function), rather than not checking the sha256 hash at all. The sha1 hash is pretty much always available at the mirrors. Failure to check the hash at all makes it pretty easy to download corrupted files.

We started having trouble with #521 when we moved to sha256 for security reasons. The sha256 hash is often unavailable at mirrors, and not available for a few hours after new releases are uploaded.

Whatever you call the new option, I think we need to make it extremely clear that this option is insecure and not for production use. Personally, I would include the text INSECURE_NOT_FOR_PRODUCTION in the name of the option itself. We don't want to see malware getting into anyone's production binaries because someone didn't understand the consequences of turning on this option.

Also, this feature is going to need documentation (see docs/configuration.rst).

aqt/helper.py Outdated Show resolved Hide resolved
aqt/helper.py Outdated Show resolved Hide resolved
aqt/helper.py Outdated Show resolved Hide resolved
aqt/helper.py Outdated Show resolved Hide resolved

def download_bin(_base_url):
url = posixpath.join(_base_url, qt_package.archive_path)
logger.debug("Download URL: {}".format(url))
return downloadBinaryFile(url, archive, "sha256", hash, timeout)
return downloadBinaryFile(url, archive, Settings.hash_algorithm, hash, timeout)
Copy link

Choose a reason for hiding this comment

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

12% of developers fix this issue

reportGeneralTypeIssues: Argument of type "bytes | None" cannot be assigned to parameter "exp" of type "bytes" in function "downloadBinaryFile"
  Type "bytes | None" cannot be assigned to type "bytes"
    Type "None" cannot be assigned to type "bytes"


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

Copy link
Owner

Choose a reason for hiding this comment

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

As Sonatype pointed out for you @lebarsfa , downloadBinaryFile method only accept argument passed by hash should be bytes variable type.

see def downloadBinaryFile(url: str, out: Path, hash_algo: str, exp: bytes, timeout: Tuple[float, float]) -> None:

You make variable hash able to be None

hash = get_hash(qt_package.archive_path, Settings.hash_algorithm, timeout) if not Settings.ignore_hash else None

Please fix a type inconsistency.

@lebarsfa
Copy link
Contributor Author

lebarsfa commented May 30, 2023

Here are some changes:

  • As suggested, renamed ignore_hash option to INSECURE_NOT_FOR_PRODUCTION_ignore_hash.
  • New hash_algorithm option, to be able to choose sha1 instead of sha256. However, if https://download.qt.io/ is not available, it seems the desired mirror will need to be added to trusted_mirrors list for the hashes to be downloaded from there. Unfortunately, it appears that the mirrors often do not provide any hash for Updates.xml, so I proposed to just show a warning for that specific file when this happens, and continue the downloads of the other files, which should have available hashes.
  • Documentation for both options.

I am not sure of what to do with sonatype-lift comments, I did not have them on my fork...

Copy link
Owner

@miurahr miurahr left a comment

Choose a reason for hiding this comment

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

Please fix the review comments that Sonatype bot give you

miurahr and others added 3 commits November 3, 2023 09:58
express default value of hash_algorithm

Signed-off-by: Hiroshi Miura <miurahr@linux.com>
- Python 3.9 and later introduce a keyword argument ``usedforsecurity``
- Set to False because we use hash to check file integrity not for password hash.

Signed-off-by: Hiroshi Miura <miurahr@linux.com>
@miurahr miurahr merged commit 612dc7b into miurahr:master Nov 5, 2023
88 of 90 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants