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

module_utils: require X_OK when checking cwd sanity #69201

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

seirl
Copy link
Contributor

@seirl seirl commented Apr 27, 2020

SUMMARY

Every time an Ansible module is run, it checks that it is running from a
"sane" working directory, a.k.a where it has the permission to be. This
is because when running commands, it might temporarily change its
working directory, then later restore pop the cwd to the previous one.

Therefore, if the module is not run from a working directory where it
has the permission to be, the module will execute fine, but then fail
when trying to restore to the previous directory.

Up until now the way this cwd "sanity" was checked was by checking for
the F_OK and R_OK permissions, which respectively check that the
directory exists and is readable. However, this is actually not
sufficient to check whether you can cwd to it! You also need to check
for X_OK, which for directories mean that the directory is searchable.

See for example:

>>> cwd = os.getcwd()
>>> cwd
'/root'
>>> os.access(cwd, os.F_OK | os.R_OK)
True
>>> os.access(cwd, os.F_OK | os.R_OK | os.X_OK)
False
>>> os.chdir(cwd)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
PermissionError: [Errno 13] Permission denied: '/root'

Basically, this means that Ansible will fail every time you run a
"become" command from a directory with the mode o+r-x.

This commit fixes this issue by adding a requirement for X_OK when
checking the sanity of the current working directory.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

module_utils

@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. needs_triage Needs a first human triage before being processed. small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team. traceback This issue/PR includes a traceback. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Apr 27, 2020
@seirl
Copy link
Contributor Author

seirl commented Apr 27, 2020

The CI errors look like false alarms.

@seirl
Copy link
Contributor Author

seirl commented May 5, 2020

Closing and re-opening to trigger a new CI build, as explained in https://docs.ansible.com/ansible/latest/dev_guide/testing.html#rerunning-a-failing-ci-job

@seirl seirl closed this May 5, 2020
@seirl seirl reopened this May 5, 2020
@seirl seirl closed this May 5, 2020
@seirl seirl reopened this May 5, 2020
@seirl seirl closed this May 5, 2020
@seirl seirl reopened this May 5, 2020
@seirl
Copy link
Contributor Author

seirl commented May 5, 2020

/rebuild_failed

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 5, 2020
@jimi-c
Copy link
Member

jimi-c commented May 5, 2020

Hi @seirl, could you please add a changelog entry to this PR, as well as an integration test to show the new behavior is correct?

Thanks!

@jimi-c jimi-c added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_tests This PR lacks required integration or unit tests for its code. and removed needs_triage Needs a first human triage before being processed. labels May 5, 2020
@ansibot ansibot removed the core_review In order to be merged, this PR must follow the core review workflow. label May 5, 2020
@seirl
Copy link
Contributor Author

seirl commented May 6, 2020

Sure, could you point out where this test would fit? The commit that introduces this function doesn't have a test attached to it, so I'm not sure where the previous behavior was tested.

Every time an Ansible module is run, it checks that it is running from a
"sane" working directory, a.k.a where it has the permission to be. This
is because when running commands, it might temporarily change its
working directory, then later restore pop the cwd to the previous one.

Therefore, if the module is not run from a working directory where it
has the permission to be, the module will execute fine, but then fail
when trying to restore to the previous directory.

Up until now the way this cwd "sanity" was checked was by checking for
the F_OK and R_OK permissions, which respectively check that the
directory exists and is readable. However, this is actually not
sufficient to check whether you can cwd to it! You also need to check
for X_OK, which for directories mean that the directory is searchable.

See for example:

>>> cwd = os.getcwd()
>>> cwd
'/root'
>>> os.access(cwd, os.F_OK | os.R_OK)
True
>>> os.access(cwd, os.F_OK | os.R_OK | os.X_OK)
False
>>> os.chdir(cwd)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
PermissionError: [Errno 13] Permission denied: '/root'

Basically, this means that Ansible will fail every time you run a
"become" command from a directory with the mode o+r-x.

This commit fixes this issue by adding a requirement for X_OK when
checking the sanity of the current working directory.
@seirl
Copy link
Contributor Author

seirl commented May 8, 2020

I added the changelog entry.

@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label May 8, 2020
@seirl
Copy link
Contributor Author

seirl commented May 8, 2020

/rebuild_failed

@seirl
Copy link
Contributor Author

seirl commented May 8, 2020

/ready_for_review

To my knowledge this function isn't unit tested at all, so adding a test case would require writing tests for the whole function, which is outside the scope of my PR.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 8, 2020
@tadeboro
Copy link
Contributor

tadeboro commented May 8, 2020

To my knowledge this function isn't unit tested at all, so adding a test case would require writing tests for the whole function, which is outside the scope of my PR.

Why would you need to add tests for the whole function? Adding tests that cover the modified parts of the code should be enough for this PR. For example, https://gist.github.com/tadeboro/9d4d9229ce23b943d6a1e75ef05a7147 contains a few tests that cover the cases where the cwd candidates are directories with no executable bit set. (BTW, feel free to steal the code and modify it as needed.)

@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 May 17, 2020
@jimi-c
Copy link
Member

jimi-c commented Jun 1, 2020

@seirl I agree, it doesn't look like there are any unit tests for this specifically. It makes it doubly important to add some then, so we can catch change in behavior in the future.

@seirl
Copy link
Contributor Author

seirl commented Jun 1, 2020

This is still on my radar, I'll add the tests when I have the time.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. pre_azp This PR was last tested before migration to Azure Pipelines. and removed core_review In order to be merged, this PR must follow the core review workflow. 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
@s-hertel s-hertel assigned s-hertel and unassigned s-hertel Apr 6, 2022
@s-hertel s-hertel added the P3 Priority 3 - Approved, No Time Limitation label Apr 6, 2022
@mattclay
Copy link
Member

mattclay commented Jun 22, 2022

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jun 22, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. pre_azp This PR was last tested before migration to Azure Pipelines. labels Jun 22, 2022
@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jun 22, 2022
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jun 22, 2022
@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jun 23, 2022
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jun 23, 2022
@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 Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_tests This PR lacks required integration or unit tests for its code. P3 Priority 3 - Approved, No Time Limitation stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. traceback This issue/PR includes a traceback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants