carvel-dev / ytt

YAML templating tool that works on YAML structure instead of text
https://carvel.dev/ytt
Apache License 2.0
1.62k stars 136 forks source link

Output file access permissions are unusual #302

Open chris-chambers opened 3 years ago

chris-chambers commented 3 years ago

Output file permissions are currently set to 0700 on creation for both files and directories. Typically, files are created as 0644 and directories as 0755, at least that's the default behavior for most shells on Unix-y systems. Would it be possible to change the way files are created by default, or provide flags to control the behavior?

https://github.com/vmware-tanzu/carvel-ytt/blob/0b8c690e3dc3edeba1e819d13de67ec3c22aa322/pkg/files/output_file.go#L35

https://github.com/vmware-tanzu/carvel-ytt/blob/0b8c690e3dc3edeba1e819d13de67ec3c22aa322/pkg/files/output_file.go#L40

Thanks for the great tool!

joaopapereira commented 3 years ago

Hello @chris-chambers thanks 😄 I started looking around into other tools to see if there was any kind of "industry standards" around the file/folder permissions. To be fair what I found was that this goes a little different depending on which tools you look at. Just anecdotally I looked at terraform, helm and kostumize and they behave in different ways. Two of them used the default Unix behavior and the other uses the same permissions as ytt.

Why did we decide to be very strict with these output files? The main reason for it was that ytt is used to generate Kubernetes manifests that in some cases will include passwords and other sensitive information. From a security perspective, we believe that it is better to have the permissions as contained as possible.

Can you provide more context on your use case? Are these permissions making it hard for you to use the tool in your use case?

Note: I did realize that we are creating files with 0700 and we do not need execution permission on them so we should lower that permission to read&write. Thanks for pointing it out 😄

chris-chambers commented 3 years ago

Thanks for the info, @joaopapereira! 0600 for files is perfectly fine, and I agree with your rationale. The execute flag was my main complaint. I should have been more clear about that. 🙂

joaopapereira commented 3 years ago

Going to write some acceptance criteria for this issue

Scenario Create configuration and save to a disk Given I am in the ytt folder When I run ytt -f examples/data-values --output-files /tmp/output And I execute ls -l /tmp/output Then I should see the created files without execution permission

total 8
-rwx------ 1 user group  58 Feb 17 17:46 config.yml
-rwx------ 1 user group 281 Feb 17 17:46 expected.txt

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this bug fixed as soon as possible" 👎 "There are more important bugs to focus on right now"

We are also happy to receive and review Pull Requests if you want to help fix this issue.

aaronshurley commented 3 years ago

@chris-chambers I'm curious, does this impact a use case of yours? If so, could you please share more. Or is this just surprising to you?

chris-chambers commented 3 years ago

@aaronshurley, there's no major impact from having the executable bit set on non-executable files. We lint for things like this because it's common for files created on Windows to have strange permissions. But, removing the executable bit files seems like the easy part to agree on (seems like having it set was unintentional).

0755 vs 0700 for directories and 0644 vs 0600 for files doesn't really matter too much. Though, you could argue that it would be nice to respect the user's umask value, which is what typically controls permissions for created files and directories. For example:

umask 077
touch foo
ls -lh foo

# gives:

-rw-------  1 chris  staff     0B Feb 19 15:43 foo
umask 022 # The default value on my system, and for many users.
touch bar
ls -lh bar

# gives:

-rw-r--r--  1 chris  staff     0B Feb 19 15:44 bar

The situation is similar for directory creation, but since the starting value is 0777, the results would have been 0700 and 0755, respectively.

ytt can't participate in a workflow like that, since it explicitly controls the file permissions. The default behavior of creat, open, and mkdir is to respect the current umask value. I believe Go's os.Create, os.OpenFile, and os.Mkdir also respect it, but I would need to double-check that.

aaronshurley commented 3 years ago

@chris-chambers Thank you for providing this detailed response. It's really helpful in understanding your expectations and why you have those expectations. I think we have all the information we need.

Just a heads up, this might not jump to the top of our backlog but we'd happily accept a PR for it. Just let us know if you hit any issues if you feel like diving in.