feat: Kubeconfig file should not be world or group readable by default

[ishustava]

Description

Changes kubeconfig file permissions from 0644 to 0600 when write_kubeconfig is set to true. Kubeconfig file should not be world or group readable due to the sensitive information such as credentials it contains.

Checklist

• CI tests are passing
• README.md has been updated after any changes to variables and outputs. See https://github.com/terraform-aws-modules/terraform-aws-eks/#doc-generation

[barryib]

Thanks @ishustava for opening this PR. I don't think that there is creds in kubeconfig. We rely on aws-cli or aws-iam-authenticator to get dynamically temporary credentials.

BTW, I think your approach is very opinionated and will probably cause issue for other users. Instead of putting 0600, you can add new variable kubeconfig_ file_permission. This will let users define what they wants as permissions.

[ishustava]

Hey @barryib, sorry for the late reply - still catching up with all the things after the holiday break. Thanks so much for the review!

I don't think that there is creds in kubeconfig. We rely on aws-cli or aws-iam-authenticator to get dynamically temporary credentials.

That's a good point. However, there should still be no reason for this file to be world- or group-readable, if we follow the principle of least privilege. Can you think of any use-cases when users would want a more wide permission?

I think your approach is very opinionated and will probably cause issue for other users. Instead of putting 0600, you can add new variable kubeconfig_ file_permission. This will let users define what they wants as permissions.

I don't think this approach is very opinionated.

The CLIs for the other two major cloud providers (Azure and Google) create kubeconfig with 0600 permissions. eksctl, recommended by AWS, similarly creates the kubeconfig file with 0600 permissions, even though it uses aws-iam-authenticator if it exists on your PATH.

Furthermore, Helm versions v3.3.3+ will print a warning if the file is world or group readable, saying that this is insecure.

Given that all other commonly used CLIs also use and recommend the 0600 permission, I think it makes sense to follow that same pattern.

[barryib]

@ishustava Good points. Thanks for your clarification.

We can set the default to 0600, but I think we should let users change that default if they want or need.

[ishustava]

Hey @barryib

I've updated the PR as you suggested. Let me know if I missed anything.

Thanks again for taking a look!

[ishustava]

Hey @barryib, any chance we could merge this?

[ishustava]

Just a quick follow-up: are there any other changes I should make? Could we merge this PR?

[barryib]

Just a quick follow-up: are there any other changes I should make? Could we merge this PR?

Sorry, I reviewed it months ago but I didn't send it 😢

[barryib]

Thanks @ishustava for your contribution.

[ishustava]

Thanks so much @barryib for taking this on and addressing the comments. Apologies for not addressing them!

Really appreciate your help getting this merged 🙏

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.