ENT-12997: Strict checksums and insecure flag #141
Conversation
…y. Seperated lockfiles to other tempfiles. Wrong checksum on pre-existing file now triggers warning, deletion of pre-existing file and re-download rather than raising of error.
…dded flags for supplying checksums for installing packages and masterfiles specified with url. All calls for download_package now allows for specifying checksums, and ignoring them
cf_remote/utils.py
Outdated
|
|
||
| with open(filepath, "rb") as f: | ||
| content = f.read() | ||
| return is_different_checksum(checksum, content) |
There was a problem hiding this comment.
No big deal. But this return statement does not have to be indented
| mkdir(tempfolder) | ||
| # Check if other process is handeling this - lockfile | ||
| lockfile = os.path.join(tempfolder, f"{filename}.lock") | ||
| with open(lockfile, "w") as lf: |
There was a problem hiding this comment.
Opening the file with in "w" mode may truncate the file while another process is writing to it.
| ) | ||
|
|
||
| # Copy over the tempfile and remove it | ||
| copy_file(tempfilename, path) |
There was a problem hiding this comment.
I would just use rename, because it is atomic. The copy_file function introduces a new race condition where two processes may be writing to ".{}.tmp".format(filename) at the same time (see CFE-4549). Furthermore, there is no need to copy the file, because we don't want to keep the original.
| log.warning("Pre-existing file did not match existing checksum. File is now deleted.") | ||
| else: | ||
| # Checksum was correct, skip download | ||
| return path |
There was a problem hiding this comment.
Should probably release the lock before returning
cf_remote/web.py
Outdated
| tempfilename, header = urllib.request.urlretrieve(url) | ||
| if file_has_different_checksum(tempfilename, checksum): | ||
| urllib.request.urlcleanup() | ||
| raise ChecksumError( |
There was a problem hiding this comment.
Should probably release the lock here as well
| # Find the checksum | ||
| name = os.path.basename(package_url) | ||
| checksum = None | ||
| if package_url in package_checksums: | ||
| # Checksum provided for the specified url | ||
| checksum = package_checksums[package_url] | ||
| elif name in package_checksums: | ||
| # Checksum provided for the specified name | ||
| checksum = package_checksums[name] | ||
| elif "any" in package_checksums: | ||
| # Single checksum or no checksum privided | ||
| checksum = package_checksums["any"] |
There was a problem hiding this comment.
This could be in a separate function, and you could add some unit tests testing different checksum files
| return SHA256_RE.match(checksum) | ||
|
|
||
|
|
||
| def parse_checksums_txt(checksums): |
There was a problem hiding this comment.
Please add some unit tests for this function
| tempfolder = os.path.join(tempfile.gettempdir(), "cf-engine") | ||
| tempfolder = os.path.join(tempfile.gettempdir(), "cf-remote") | ||
| mkdir(tempfolder) | ||
| # Check if other process is handeling this - lockfile | ||
| # Wait for other process if lockfile is taken. Otherwise lock it |
There was a problem hiding this comment.
Looks like these changes belong in one of the previous commits
| # Unlock the lockfile | ||
| fcntl.flock(lf.fileno(), fcntl.LOCK_UN) |
|
Closing until August. |
No description provided.