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.

Sign up for GitHub

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

Jump to bottom

get_url: Verify checksum using tmpsrc, not dest #64092

Merged
merged 1 commit into from
Jun 20, 2024
Merged

get_url: Verify checksum using tmpsrc, not dest #64092

merged 1 commit into from
Jun 20, 2024
+55 −7

Conversation

dbrgn
Copy link
Contributor

@dbrgn dbrgn commented Oct 30, 2019

SUMMARY

Previously, if the checksum of the downloaded file did not match the
specified checksum, the destination file was removed. This possibly
leaves the system that is being provisioned in an invalid state.

Instead, the checksum should be calculated on the temporary file only.
If there's a mismatch, delete the temporary file, not the destination
file.

This requires checking the checksum before moving the file.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

get_url

ADDITIONAL INFORMATION

Steps to reproduce:

  1. Download a file using the get_url module using the correct checksum. Ensure that the file was downloaded successfully.
  2. Now change the checksum so that it's invalid and re-run the get_url module
  3. Even though the module failed, the previously downloaded file was removed

@dbrgn dbrgn force-pushed the get-url-checksum-tmpsrc branch from 3c75e4c to 34a2b64 Compare October 30, 2019 09:10
@ansibot
Copy link
Contributor

ansibot commented Oct 30, 2019

cc @jpmens @ptux
click here for bot help

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. net_tools Net-tools category support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Oct 30, 2019
@dbrgn
Copy link
Contributor Author

dbrgn commented Oct 30, 2019

I'll provide an integration test in a few minutes.

@dbrgn dbrgn force-pushed the get-url-checksum-tmpsrc branch from 34a2b64 to b5370b2 Compare October 30, 2019 10:33
@dbrgn
Copy link
Contributor Author

dbrgn commented Oct 30, 2019

I'll provide an integration test in a few minutes.

Done, commit was updated!

@samdoran samdoran requested a review from sivel October 31, 2019 19:15
@samdoran samdoran added needs_verified This issue needs to be verified/reproduced by maintainer P3 Priority 3 - Approved, No Time Limitation and removed needs_triage Needs a first human triage before being processed. labels Oct 31, 2019
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 8, 2019
sivel
sivel previously requested changes Mar 18, 2020
View reviewed changes
Copy link
Member

@sivel sivel left a comment

Choose a reason for hiding this comment

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

This PR also needs a changelog fragment in changelogs/fragments

@dbrgn dbrgn force-pushed the get-url-checksum-tmpsrc branch from b5370b2 to 0a719af Compare March 18, 2020 16:38
@dbrgn
Copy link
Contributor Author

dbrgn commented Mar 18, 2020

@sivel changelog fragment was added.

@ansibot ansibot added support:community This issue/PR relates to code supported by the Ansible community. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Mar 18, 2020
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Mar 28, 2020
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed stale_review Updates were made after the last review and the last review is more than 7 days old. labels May 16, 2020
@ansibot ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 5, 2020
@ansibot ansibot removed the support:community This issue/PR relates to code supported by the Ansible community. label Mar 4, 2021
@ansibot ansibot added the stale_review Updates were made after the last review and the last review is more than 7 days old. label Jul 12, 2023
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Oct 24, 2023
@dbrgn @Akasurde
get_url: Verify checksum using tmpsrc, not dest
d2ec0f0 
Previously, if the checksum of the downloaded file did not match the
specified checksum, the *destination* file was removed. This possibly
leaves the system that is being provisioned in an invalid state.

Instead, the checksum should be calculated on the temporary file only.
If there's a mismatch, delete the *temporary* file, not the destination
file.

This requires checking the checksum before moving the file.
@Akasurde Akasurde force-pushed the get-url-checksum-tmpsrc branch from 0a719af to d2ec0f0 Compare June 15, 2024 01:33
@Akasurde Akasurde requested a review from sivel June 15, 2024 01:33
@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html pre_azp This PR was last tested before migration to Azure Pipelines. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 15, 2024
@Akasurde Akasurde requested a review from s-hertel June 19, 2024 14:56
@s-hertel s-hertel added verified This issue has been verified/reproduced by maintainer and removed needs_verified This issue needs to be verified/reproduced by maintainer labels Jun 20, 2024
@s-hertel s-hertel dismissed sivel’s stale review June 20, 2024 14:38

changelog added and formatting updated to use fstring

@s-hertel

This comment was marked as resolved.

Sign in to view
@azure-pipelines Azure Pipelines

This comment was marked as resolved.

Sign in to view
@ansibot ansibot removed the stale_review Updates were made after the last review and the last review is more than 7 days old. label Jun 20, 2024
@s-hertel s-hertel merged commit c2c6005 into ansible:devel Jun 20, 2024
94 checks passed
@s-hertel
Copy link
Contributor

@dbrgn Thanks for fixing the issue and writing tests to prevent a regression!

@Akasurde Thanks for rebasing and updating the branch!

@ansible ansible locked and limited conversation to collaborators Jul 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@s-hertel s-hertel s-hertel approved these changes

@sivel sivel Awaiting requested review from sivel

Assignees
No one assigned
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. module This issue/PR relates to a module. net_tools Net-tools category P3 Priority 3 - Approved, No Time Limitation support:core This issue/PR relates to code supported by the Ansible Engineering Team. verified This issue has been verified/reproduced by maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dbrgn @ansibot @s-hertel @sivel @samdoran