ansible-galaxy - fixup role install failure handling #82052

s-hertel · 2023-10-20T21:11:42Z

SUMMARY

~~Fixes #81965~~

ddf0311 added sanitization for the tarfile linknames, making '..' invalid for symlinks. This can result in a traceback (if there were no valid parts), or setting symlinks to new paths using the valid parts. This PR restricts that limitation to 'name' again, and instead validates symlinks exist (when we've adjusted them) and that they resolve to a path in the role archive. (Moved to #82165)

This gives an error for invalid parts in the tarfile name/linkname instead of giving a warning and trying to construct a new one.
- I think this is the right behavior for linkname to prevent broken content
- Should I keep it backwards compatible for name in case someone is using the auto-rename-to-something-safe behavior?

Roles are now extracted to a tempdir to prevent partial installs.

Tarfile members outside of the role root are no longer installed (leading to broken roles).

This method is getting long, but refactoring seemed a bit invasive to backport.

[] Needs a test for tarfile containing members outside the role root
[] Needs a test for rolling back a failed install
[] Needs agreement/tests on behavior for invalid components in tarfile member names and linknames - waiting on Targeted fix for installing roles with symlinks containing '..' #82165 since linknames containing '..' aren't safe currently.

ISSUE TYPE

Bugfix Pull Request

Make install more strict about what is extracted from the tarfile: - fail on invalid content we previously skipped - give a warning for excluded tarfile content that previously would have been included Allow installing symlinks again, as long at they resolve to a path within the role changelog

add tests for safe and unsafe relative links in roles

HarryWeppner · 2023-11-07T16:13:44Z

@s-hertel and @nitzmahone, this PR promises to fix other galaxy roles as well - do you have an ETA when this will land? Thanks.

s-hertel · 2023-11-07T19:37:32Z

lib/ansible/galaxy/role.py

+                                        # Doesn't happen with our build process, but avoid extracting members that aren't adjacent to the role metadata
+                                        display.warning(f"Only extracting members in {n_archive_parent_dir}: ignoring {member.name}")
+                                        ignore_external.add(member)
+                                        continue


If a tarfile contains something like:

./role-data/ ├── role-name │ ├── meta/main.yml │ └── tasks └── hitchhiker └── keep

we find the meta/main.yml in the role-name directory, but install all tarfile members (not just those adjacent to the meta directory). On devel this ends up installing an invalid role (and so it needs to be manually deleted to reinstall).

This could be an error instead, but I thought the warning was better at least than installing a broken role.

Should I separate this from the other changes or is it something we want to backport with the symlinks fix?

s-hertel · 2023-11-07T19:39:49Z

lib/ansible/galaxy/role.py

+                                    if invalid_parts or not n_final_parts:
+                                        raise AnsibleError(f"role content {member.name} is not able to be extracted")


This is a small change to try to avoid installing broken roles, but is not backwards compatible. Should this affect name and linkname?

…handled

s-hertel · 2023-11-07T21:39:57Z

@HarryWeppner I don't have an ETA (it needs a review), but I decided to split the symlinks fix into a separate PR #82165 to make it safer to backport, since the other issues this is fixing are not new.

webknjaz · 2023-11-14T19:59:15Z

lib/ansible/galaxy/collection/__init__.py

@@ -952,7 +952,10 @@ def _tempdir():
    try:
        yield b_temp_path
    finally:
-        shutil.rmtree(b_temp_path)
+        try:


(nit) using with contextlib.suppress(FileNotFoundError): might read better.

webknjaz · 2023-11-14T20:04:21Z

lib/ansible/galaxy/role.py

@@ -412,24 +421,42 @@ def install(self):
                                        # to debug a broken installation.
                                        if not n_part:
                                            continue
-                                        if n_part == '..':
+                                        if n_part == '..':  # TODO: only disallow .. for 'name' once symlinks are validated
+                                            invalid_parts |= True


(maintainability concern)

Isn't this the same as just

Suggested change

invalid_parts |= True

invalid_parts = True

?

I keep having to re-verify what | does in the REPL whenever I see |=. It might be useful for collection types (sets, for example), but | used with True on any side of the operand always results in True. So why not just use the assignment operator to save the readers some potential confusion?

webknjaz · 2023-11-28T15:41:55Z

A rebase should fix the rpmfluff issue in CI.

HarryWeppner · 2024-01-15T21:14:20Z

@s-hertel, I saw that #82165 was merged into devel but none of the backports were merged. Is there a released version of ansible that has the fix for handling ..?

s-hertel · 2024-01-15T22:59:22Z

@HarryWeppner That's correct, it's only been merged to the devel branch.

s-hertel · 2024-01-19T13:48:59Z

@HarryWeppner Backports have been merged, should be fixed in 2.16.3, 2.15.9, and 2.14.4 (RC1 scheduled for Jan 22, GA Jan 29).

ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. has_issue labels Oct 20, 2023

fix parentheses and cumulative value

ed8913e

s-hertel mentioned this pull request Oct 20, 2023

ansible-galaxy [WARNING]: Illegal filename '..': '..' is not allowed (ansible-core 2.14.11) #82051

Closed

mkrizek removed the needs_triage Needs a first human triage before being processed. label Oct 24, 2023

mkrizek requested a review from nitzmahone October 24, 2023 15:19

s-hertel marked this pull request as ready for review October 24, 2023 19:02

s-hertel force-pushed the fix-role-install-misc-and-symlinks branch from 53ecfd5 to 9d54a6d Compare October 24, 2023 19:04

s-hertel marked this pull request as draft October 24, 2023 19:14

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 24, 2023

Akasurde mentioned this pull request Oct 25, 2023

https://github.com/ansible/ansible/pull/81780 breaks installation of Mayeu.RabbitMQ 1.4.0 #82073

Closed

1 task

s-hertel force-pushed the fix-role-install-misc-and-symlinks branch from 9d54a6d to f5bc6ad Compare October 25, 2023 15:51

tidy

81d0a32

add tests for safe and unsafe relative links in roles

s-hertel force-pushed the fix-role-install-misc-and-symlinks branch from f5bc6ad to 81d0a32 Compare October 25, 2023 15:55

s-hertel marked this pull request as ready for review October 25, 2023 15:55

ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 25, 2023

s-hertel mentioned this pull request Oct 26, 2023

Fix galaxy role illegal filename handling #81966

Closed

shorten error and improve stderr parsing

daa3007

HarryWeppner mentioned this pull request Oct 27, 2023

ERROR! couldn't resolve module/action 'conda' andrewrothstein/ansible-miniconda#31

Closed

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 3, 2023

s-hertel commented Nov 7, 2023

View reviewed changes

s-hertel mentioned this pull request Nov 7, 2023

Targeted fix for installing roles with symlinks containing '..' #82165

Merged

Remove symlinks fix and only improve how broken role installation is …

06fdc78

…handled

s-hertel removed the request for review from nitzmahone November 7, 2023 21:54

s-hertel changed the title ~~ansible-galaxy - fixup role install failure and symlinks~~ ansible-galaxy - fixup role install failure handling Nov 7, 2023

s-hertel marked this pull request as draft November 7, 2023 21:57

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. 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 Nov 7, 2023

webknjaz reviewed Nov 14, 2023

View reviewed changes

webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Nov 28, 2023

ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Dec 5, 2023

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 Dec 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ansible-galaxy - fixup role install failure handling #82052

ansible-galaxy - fixup role install failure handling #82052

s-hertel commented Oct 20, 2023 •

edited

HarryWeppner commented Nov 7, 2023

s-hertel Nov 7, 2023

s-hertel Nov 7, 2023

s-hertel commented Nov 7, 2023

webknjaz Nov 14, 2023

webknjaz Nov 14, 2023 •

edited

webknjaz commented Nov 28, 2023

HarryWeppner commented Jan 15, 2024

s-hertel commented Jan 15, 2024

s-hertel commented Jan 19, 2024

		if invalid_parts or not n_final_parts:
		raise AnsibleError(f"role content {member.name} is not able to be extracted")

ansible-galaxy - fixup role install failure handling #82052

Are you sure you want to change the base?

ansible-galaxy - fixup role install failure handling #82052

Conversation

s-hertel commented Oct 20, 2023 • edited

SUMMARY

ISSUE TYPE

HarryWeppner commented Nov 7, 2023

s-hertel Nov 7, 2023

Choose a reason for hiding this comment

s-hertel Nov 7, 2023

Choose a reason for hiding this comment

s-hertel commented Nov 7, 2023

webknjaz Nov 14, 2023

Choose a reason for hiding this comment

webknjaz Nov 14, 2023 • edited

Choose a reason for hiding this comment

webknjaz commented Nov 28, 2023

HarryWeppner commented Jan 15, 2024

s-hertel commented Jan 15, 2024

s-hertel commented Jan 19, 2024

s-hertel commented Oct 20, 2023 •

edited

webknjaz Nov 14, 2023 •

edited