Breaking change in the "stable" releases

[yoctozepto]

SUMMARY

#70221, having been backported to stable branches (2.9: #70825, 2.8: #70827), killed my beloved kitties.

ISSUE TYPE

• Bug Report
• General rant about stability of "stable"

COMPONENT NAME

ANSIBLE VERSION

ansible 2.9.12

ansible 2.8.14

CONFIGURATION

OS / ENVIRONMENT

POSIX platforms

STEPS TO REPRODUCE

- name: Template out my beloved kitty
  template:
    src: "kitty.j2"
    dest: "/my_kitty"

EXPECTED RESULTS

I expected my kitty to be readable by all users as it was before (before 2.9.12, 2.8.14; mode=0644).

ACTUAL RESULTS

Other users cannot read my kitty (mode=0600).

[ansibot]

Files identified in the description:
None

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

[bcoca]

Security fixes are allowed to create 'breaking changes'.

Most bugfixes are a 'breaking change' if you relied on the buggy behaviour. We try to avoid backporting those that would be too impactfull, but that consideration is normally trumped by security concerns.

[yoctozepto]

I disagree, using default (which is also well-established one, i.e. using umask; normally anyone would assume the files to be created with such mode) is not equal to relying on a bug. I can accept that the tasks that do not allow the mode to be set should rather default to best security practices than not but changing the default mode for just about any task feels wrong. Having looked at the CVE, I'm a bit surprised this was the solution to solve it in the first place. Feels a bit like using hammer. I understand it might have been due to time constraints. I'd upvote the default change even in the now-stabilizing 2.10 (hopefully with some tooling to allow easier catching of affected tasks) but this going 1:1 to stable branches feels wrong.

[ssbarnea]

@bcoca As this is effectively a breaking change with big changes of breaking usage of 8 core modules, please take few minutes to give some feedback on the newly propose linter rule, one that should prevent such silent failures.

[samdoran]

After some further discussion about ways to solve this, I'm going to look at using splitting this fix into two parts: 1) to solve the issue of permissions of the temporary and 2) use the umask of the system to determine the final permissions of the file if no mode was specified.

It seems I took the initial fix too far of preserving the very strict permissions of the temporary file on the final file when no mode was specified.

[yoctozepto]

Thanks, Sam. I appreciate your attitude towards this.

[mwhahaha]

I can confirm that this also breaks things like copy where one would expect that the file be written out using the default umask and not the more restrictive 0600. e.g. uri -> copy to output file to disk. I agree the tightened restrictions on temporary files is important, but I disagree that all file operations should get something more restrictive than the system umask.

[samdoran]

We've been dinged in the past for using the system umask when it is set to an insecure value. The previous behavior would probably be pretty surprising if it was scrutinized more. But since it worked, few probably noticed they could end up with world readable files in some cases.

I wouldn't say it breaks copy tasks, just that the default has changed and the desired permissions should be specified. copy at least has a mechanism to specify mode.

Explicit is better than implicit.

Tasks this change does break are those that provide no mechanism for setting the mode and therefore provide no warning, which was my biggest concern with the change. Balancing usability and security is tough.

[mwhahaha]

I would be more ok if the new restrictions went in with a warning to be correct over several releases however doing so in a backport was a bit much. The security issue is not necessarily one ansible should be fixing for users magically. Ansible should take care of it's own files that it uses during execution however.

[SpComb]

The 2.9.12 changelog only mentions temporary files:

security issue atomic_move - change default permissions when creating temporary files so they are not world readable (#67794) (CVE-2020-1736)

My understanding of the impact of this change is that the real non-temporary files created by copy, template etc. no longer respect the system umask by default, and will thus change from the common case of 0644 to always being 0600 when no mode is given?

[nikoIkonen]

This change made my dev env unreachable (think networkd files) and broke plenty of my roles which relied on correctly set umask on the system. I do accept that security fixes can be breaking changes, but it should be clearly stated in a change log and also mentioned in documentation what happens if you don't set mode for a file. Though still a bit heavy change for a minor version security fix.