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.

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

755 permissions when creating directory #180

Merged
merged 1 commit into from
Apr 12, 2023
Merged

755 permissions when creating directory #180

merged 1 commit into from
Apr 12, 2023

Conversation

vshah23
Copy link
Contributor

@vshah23 vshah23 commented Apr 6, 2023

Context

[CI-763] A customer pointed out that our plugin executables have 777 permissions which was triggering their security scanner and recommended us to change permissions to 755.

We also thought why not change the directory permissions to 755 to be on the safe side as a proactive security measure.

Changes

  • Set directory permissions to 755 as a proactive security measure

Investigation

We found that directories already have 755 permission, but we don’t know how 😳

Upon further inspection we found from the bitrise-cli we are calling the EnsureDirExist function in pathutil.go which calls os.MkdirAll(dir, 0777) so we’re explicitly saying to use 777 permission, but the ~/.bitrise directory is still being created with 755 permission.

We thought there was something happening during the image creation process, but it turns out it’s due to umask that the folder permissions are defaulting to 755.

Decisions

We decided to avoid setting the umask using syscall.Umask() and instead just set the file permissions here. This will not have much of an affect since the directories are already set with 755 permissions, but we want to be explicit about it.

@vshah23 vshah23 requested a review from tothszabi April 6, 2023 16:14
@vshah23 vshah23 marked this pull request as ready for review April 6, 2023 16:14
@vshah23 vshah23 merged commit 3738bfd into v1 Apr 12, 2023
@vshah23 vshah23 deleted the CI-763 branch April 12, 2023 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants