Set umask in main?

[sed-i]

Problem

By default, when charm code opens a file for writing without additional args, we get world-readable files (rw-r--r--).
We may want to avoid leaking sensitive files to other services that run next to ours.
One way of doing this is for ops to call e.g. umask o-rx,g=r as part of main. This would work for VM charms and the charm container, but not sure what's implemented for pebble-push into a workload container.

At first glance, the centralized default umask approach seems easy and harmless.

Additional context

• Pebble's layer spec supports starting services with different users.
• systemd services have User= and Group=, but not sure if this kind of control is available for snaps and charms.

[sed-i]

Additional input from @morphis:

if a service running in the same system has an issue and allows an attacker to come in, you now potentially get access to other systems and depending on how things are setup you may have now more privileges than before

[tonyandrewmeyer]

ops itself doesn't do much writing of files. Excluding the testing frameworks, podspec, and os.lib, there's:

• the state file, which we explicitly set to S_IRUSR | S_IWUSR
• secret contents, which has mkstmp style protection
• Container's pull_path, which uses the default

Then there's Pebble - it's not spawned by ops, so wouldn't be influenced by ops doing a umask. Remotely, exec could create files, but that should be managed by running as the appropriate user/group and whatever command is run; mkdir has user, group, and permissions arguments defaulting to 755 for permissions; push has user, group, and permissions arguments defaulting to 644 for permissions. Locally, other than Pebble internals (like the state file) I don't think anything opens files for writing (pull returns the content in memory, for example, and the user needs to do something with it). I don't think we would want to change ops.pebble (and suggestions for changes to Pebble would be better off in canonical/pebble).

That leaves the charm code, which could obviously include anything. I do agree that a more restrictive default would be better - having people opt into less secure options rather than the other way around. However, it seems likely that this would break things - maybe semi-official tools like jhack and probably some charms that are relying on the current behaviour. For the latter, I'll try running with this change against a bunch of charm unit tests and see what happens.

In the meantime, it seems like it would be trivial for charms to opt into this behaviour by just doing the umask call themselves (in the charm's __init__ for example, since there's no at-risk writing prior to that). Note that I'm not saying I think that it is the charm's responsibility: I do agree that it would be nicer if ops did it - just that it would be easy to adopt this in individual charms where there can be extensive testing and review.

[sed-i]

Thanks @tonyandrewmeyer!
Just for reference, a couple examples from anbox that already do this:

• https://github.com/canonical/anbox-cloud-charms/blob/e489442c371a66493464ae034da65e8e9ef037ac/layers/anbox/lib/charms/layer/pki.py#L44
• https://github.com/canonical/anbox-cloud-charms/blob/e489442c371a66493464ae034da65e8e9ef037ac/charms/anbox-stream-agent/src/agent.py#L287

[tonyandrewmeyer]

For the latter, I'll try running with this change against a bunch of charm unit tests and see what happens.

No failures over the 115 charms I tried. Unit tests aren't a great check for this - there's a reasonable chance that they're mocking out actual file creation, and/or aren't tying together pieces that separately create and use files, but I still don't have a good way of running a lot of charm integration tests against a branch. When I figure that out, I'll try it out with this too.