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

ignore ansible.cfg in world writable cwd #42070

Merged
merged 2 commits into from
Jun 29, 2018
Merged

Conversation

bcoca
Copy link
Member

@bcoca bcoca commented Jun 28, 2018

SUMMARY
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

config

ANSIBLE VERSION
2.x

perms1 = os.stat(path1)
if perms1.st_mode & stat.S_IWOTH:
# Ansible is in a world writable directory, ignoring it as ansible.cfg source.
path1 = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to do a display.warning() here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bcoca says that we don't have a display object created yet. If we attempt to use a display object, it will throw an error. I think we should add a commented out call to display.warn() here so that someone can remember to add this once we're able to. (I'm planning to replace/reimplement the backend of display with logging infrastructure for 2.8. And once we have that, we can either queue early updates for later logging or we can push the earliest messages to a log that does not depend on configuration.)

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 28, 2018
@abadger
Copy link
Contributor

abadger commented Jun 28, 2018

The two failures here were unrelated. Rebuilding those two jobs (windows and the rhel7 vm builder).

@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Jun 28, 2018
@abadger
Copy link
Contributor

abadger commented Jun 28, 2018

Okay, down to one failure. Still looks unrelated (It was unstable last time and failed this time). The build target is windows/2012-R2/1 I'm hitting rebuild in shippable again.

@abadger
Copy link
Contributor

abadger commented Jun 29, 2018

tests completed successfully this time. Unstable is from windows test. Unrelated to this change.

The only things left are to add a commented out call to display.warn() so that we can remember to add that once we have the capability and a changelog file. The logic looks good. Please merge and backport all the way to 2.4 ASAP.

@abadger
Copy link
Contributor

abadger commented Jun 29, 2018

Talked with @misc who noted that ansible.cfg locations are not noted in the documentation (seems like it used to be but was accidentally omitted when the documentation wa sreorganized.).

So we need to add to documentation (Either intro_configuration or somewhere under the reference_guide... from the (broken) link name, it appears that dharmabumstead was intending to move it under reference guide). We also need to add ansible.cfg in current working directory to the FILES section of the man pages (the other locations are listed there but not CWD). That section is in /docs/templates/man.j2

So list of changes to be made:

  • Commented out display.warn()
  • changelog entry
  • intro_configuration or reference guide section
  • Add to the FILES section of docs/templates/man.j2

@abadger
Copy link
Contributor

abadger commented Jun 29, 2018

The old documentation is right under the table of contents here: https://github.com/ansible/ansible/blob/stable-2.3/docs/docsite/rst/intro_configuration.rst

@ansibot ansibot added the docs This issue/PR relates to or includes documentation. label Jun 29, 2018
@ansibot
Copy link
Contributor

ansibot commented Jun 29, 2018

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/config/manager.py:38:1: E302 expected 2 blank lines, found 1
lib/ansible/constants.py:29:1: E302 expected 2 blank lines, found 1

click here for bot help

 * also added 'warnings' to config
 * upadted man page template
@@ -0,0 +1,2 @@
bugfixes:
- avoid using ansible.cfg in a world readable dir, readd missing docs
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we don't have a dedicated category for security fixes, please prepend **Security Fix** - to this:

bugfixes:
    - ** Security Fix ** - avoid using ansible.cfg in a world readable dir, readd missing docs

@acozine
Copy link
Contributor

acozine commented Jun 29, 2018

@mscherer
Copy link
Contributor

@misc is not me, that's @mscherer in github land. I think I wasn't clear when we discuss, my point was that maybe we should write in the doc something around what to watch for, from a security point of view when you have a shared server to run Ansible with multiple users.
As Ansible is quite flexible, some people do that, some others do run from laptop, some have a jenkins runner, etc. We never pushed for the one right way to do things, but I think the best that could be done is to at least show the pitfalls to watch for and anti patterns to avoid.

@abadger
Copy link
Contributor

abadger commented Jun 29, 2018

@mscherer, What should that section have to say about this issue?

@abadger
Copy link
Contributor

abadger commented Jun 29, 2018

The following builders failed but all look unrelated:

  • T=windows/2012-R2/1
  • T=rhel/7.4/1
  • T=rhel/7.4/2
  • T=cloud/default/2.7/2
  • T=cloud/default/3.6/2

Rebuildig them to see whether they come up green.

@abadger abadger merged commit b6f2aad into ansible:devel Jun 29, 2018
abadger pushed a commit to abadger/ansible that referenced this pull request Jun 29, 2018
* ignore ansible.cfg in world writable cwd
 * also added 'warnings' to config
 * updated man page template
(cherry picked from commit b6f2aad)

Co-authored-by: Brian Coca <bcoca@users.noreply.github.com>
abadger pushed a commit to abadger/ansible that referenced this pull request Jul 3, 2018
* ignore ansible.cfg in world writable cwd
 * also added 'warnings' to config
 * updated man page template.
(cherry picked from commit b6f2aad)

Co-authored-by: Brian Coca <bcoca@users.noreply.github.com>
abadger pushed a commit that referenced this pull request Jul 3, 2018
* ignore ansible.cfg in world writable cwd
 * also added 'warnings' to config
 * updated man page template.
(cherry picked from commit b6f2aad)

Co-authored-by: Brian Coca <bcoca@users.noreply.github.com>
@bcoca bcoca deleted the avoid_world_cfg branch July 3, 2018 20:07
nitzmahone pushed a commit that referenced this pull request Jul 3, 2018
* [stable-2.5] ignore ansible.cfg in world writable cwd (#42070)

* ignore ansible.cfg in world writable cwd
 * also added 'warnings' to config
 * updated man page template
(cherry picked from commit b6f2aad)

Co-authored-by: Brian Coca <bcoca@users.noreply.github.com>

* Update wrcwd_ansible.cfg.yml
abadger pushed a commit to abadger/ansible that referenced this pull request Jul 3, 2018
* ignore ansible.cfg in world writable cwd
 * also added 'warnings' to config
 * updated man page template
(cherry picked from commit b6f2aad)

Co-authored-by: Brian Coca <bcoca@users.noreply.github.com>
mattclay pushed a commit that referenced this pull request Jul 3, 2018
* ignore ansible.cfg in world writable cwd
 * also added 'warnings' to config
 * updated man page template
(cherry picked from commit b6f2aad)

Co-authored-by: Brian Coca <bcoca@users.noreply.github.com>
@mcepl
Copy link

mcepl commented Oct 10, 2018

Just for the administrative purposes ... this is fix for CVE-2018-10874, right?

You are not making it easy for non-Red Hat maintainers to find patches they need :(. Oh well.

@bcoca
Copy link
Member Author

bcoca commented Oct 12, 2018

@mcepl to be fair, 'redhat' maintainers don't get any extra info than what you see here, this is fix for CVE-2018-10875, the ticket you want is 42067

we normally don't tag the tickets with cve info, just our releases

FYI, this ticket has some followups as the conditions were too strict and unintentionally impaired functionality. It is not normally good to take our commits in isolation.

@mcepl
Copy link

mcepl commented Oct 12, 2018

@mcepl to be fair, 'redhat' maintainers don't get any extra info than what you see here, this is fix for CVE-2018-10875, the ticket you want is #42067

Yeah, I remember how disintegrated Red Hat sometimes tends to be. Oh well. I will take a look at that ticket. Thank you.

@bcoca
Copy link
Member Author

bcoca commented Oct 12, 2018

@mcepl not a question of integration/disintegration, the other teams just consume our releases, which would include these patches, so there is no need for them to find them on their own.

@ansible ansible locked and limited conversation to collaborators Jul 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 bug This issue/PR relates to a bug. docs This issue/PR relates to or includes documentation. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants