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.

Sign up for GitHub

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

Jump to bottom

Vault-6037 making filesystem permissions check opt-in #15452

Merged
merged 3 commits into from
May 17, 2022
Merged

Vault-6037 making filesystem permissions check opt-in #15452

merged 3 commits into from
May 17, 2022

Conversation

akshya96
Copy link
Contributor

https://hashicorp.atlassian.net/browse/VAULT-6037
Making this change opt-in, by renaming the environment variable VAULT_DISABLE_FILE_PERMISSIONS_CHECK to VAULT_ENABLE_FILE_PERMISSIONS_CHECK and adjusting the logic

@akshya96 akshya96 requested review from a team May 16, 2022 20:55
HridoyRoy
HridoyRoy reviewed May 16, 2022
View reviewed changes
Copy link
Contributor

@HridoyRoy HridoyRoy left a comment

Choose a reason for hiding this comment

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

Looks good overall -- just wondering if we are ok with removing the consts.VaultDisableFilePermissionsCheckEnv environment variable entirely, as it seems like this change is also going to be backported into minor versions?

@akshya96
Copy link
Contributor Author

akshya96 commented May 16, 2022 •
edited
Loading

Looks good overall -- just wondering if we are ok with removing the consts.VaultDisableFilePermissionsCheckEnv environment variable entirely, as it seems like this change is also going to be backported into minor versions?
Looks like these changes were removed from previous backports

@akshya96 akshya96 requested a review from HridoyRoy May 16, 2022 21:01
HridoyRoy
HridoyRoy approved these changes May 16, 2022
View reviewed changes
Copy link
Contributor

@HridoyRoy HridoyRoy left a comment

Choose a reason for hiding this comment

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

Lgtm!

@cipherboy
Copy link
Contributor

cipherboy commented May 17, 2022 •
edited
Loading

@akshya96 As I mentioned in #15438, do you think we can switch this from a literal value to strconv.ParseBool like most of the other server envvars are? This'd give us a little more consistency :-)

I'm happy to open a PR once this lands if you'd prefer.

@akshya96
Copy link
Contributor Author

@akshya96 As I mentioned in #15438, do you think we can switch this from a literal value to strconv.ParseBool like most of the other server envvars are? This'd give us a little more consistency :-)

I'm happy to open a PR once this lands if you'd prefer.

Sure, It makes sense to be more consistent. Will make this change and update the PR

@cipherboy
Copy link
Contributor

Thank you!!

@akshya96 akshya96 requested a review from cipherboy May 17, 2022 17:44
cipherboy
cipherboy approved these changes May 17, 2022
View reviewed changes
Copy link
Contributor

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

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

Thank you! 💯

@akshya96 akshya96 merged commit e7f316b into main May 17, 2022
Gabrielopesantos pushed a commit to Gabrielopesantos/vault that referenced this pull request Jun 6, 2022
@akshya96 @Gabrielopesantos
Vault-6037 making filesystem permissions check opt-in (hashicorp#15452)
41c77af 
* adding env var changes

* adding changelog

* adding strcov.ParseBool
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cipherboy cipherboy cipherboy approved these changes

@HridoyRoy HridoyRoy HridoyRoy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@akshya96 @cipherboy @HridoyRoy