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

CHEF-10987: Habitat installs on hardened systems #9401

Merged
merged 7 commits into from
Oct 21, 2024
Merged

CHEF-10987: Habitat installs on hardened systems #9401

merged 7 commits into from
Oct 21, 2024

Conversation

jasonheath
Copy link
Contributor

@jasonheath jasonheath commented Sep 20, 2024 •
edited
Loading

  • Adds umask 0222 to start of install.sh
  • aligns rustfmt between Makefile target and expeditor/buildkite/ci/cd
  • Ensures hab pkg installs have 755 dirs

@netlify Netlify
Copy link

netlify bot commented Sep 20, 2024 •
edited
Loading

👷 Deploy Preview for chef-habitat processing.

Name Link
🔨 Latest commit 600f8f9
🔍 Latest deploy log https://app.netlify.com/sites/chef-habitat/deploys/67164c5729699100089095c1

@jasonheath jasonheath force-pushed the jah/CHEF-10987_habitat-installs-hardened-systems branch 3 times, most recently from b0e719b to a4e7477 Compare September 24, 2024 14:36
@jasonheath
Copy link
Contributor Author

jasonheath commented Sep 24, 2024 •
edited
Loading

Didn't want to just delete this but this is no longer true and refers to a version of the solution that no longer exists.

There is currently a unit test that exercises setting the umask. The documentation states that setting the umask never fails but it still felt proper to try and exercise it. As it was first written it played poorly with another test. At this point I don't think it will misbehave in the future and I would leave it in but I also wanted to make sure I highlighted this for review.

mwrock
mwrock reviewed Sep 25, 2024
View reviewed changes
Copy link
Contributor

@mwrock mwrock left a comment

Choose a reason for hiding this comment

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

Setting the umask for the entire process seems heavy handed. I thought we had discussed this and concluded that hab pkg install or really common/src/command/package/install.rs would chmod via (posix_perm::set_permissions) the files it writes to disk with 755.

@jasonheath jasonheath force-pushed the jah/CHEF-10987_habitat-installs-hardened-systems branch from a4e7477 to ac4fff7 Compare October 9, 2024 15:30
@jasonheath jasonheath marked this pull request as draft October 9, 2024 15:33
@jasonheath
Adds umask 0222 to start of install.sh
660e202
@jasonheath
Aligns Makefile fmt target with buildkite pipeline
7733d7d 
I discovered that the arguments used in our buildkit pipeline was not
the same that was being used when we ran make fmt on the commandline.
This commit makes the Makefile target match what we use in buildkite.
@jasonheath jasonheath force-pushed the jah/CHEF-10987_habitat-installs-hardened-systems branch 3 times, most recently from 8250817 to 10024cf Compare October 15, 2024 14:20
@jasonheath
Ensures hab pkg installs have 755 dirs
8070c04 
Signed-off-by: Jason Heath <jh@jasonheath.com>
@jasonheath jasonheath force-pushed the jah/CHEF-10987_habitat-installs-hardened-systems branch from 10024cf to 8070c04 Compare October 15, 2024 18:44
@jasonheath jasonheath marked this pull request as ready for review October 15, 2024 19:31
@agadgil-progress agadgil-progress changed the title CHEF-1098: Habitat installs on hardened systems CHEF-10987: Habitat installs on hardened systems Oct 17, 2024
agadgil-progress
agadgil-progress requested changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@agadgil-progress agadgil-progress left a comment

Choose a reason for hiding this comment

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

We should document inline why we are doing the code changes - because the reader of the code will not have much context. Better to document

Also, the changes to Makefile are not required.

Makefile Outdated Show resolved Hide resolved
test/end-to-end/test_pkg_install.ps1 Outdated Show resolved Hide resolved
components/core/src/util/posix_perm.rs Outdated Show resolved Hide resolved
@jasonheath
Removes --all from make fmt target
4a6b057 
Signed-off-by: Jason Heath <jh@jasonheath.com>
@jasonheath
Corrects umask to 0077 in test case
b6e3291 
Signed-off-by: Jason Heath <jh@jasonheath.com>
@jasonheath
Adds pub(crate) and doc to ensure_path_permissions
b9cfb3a 
Signed-off-by: Jason Heath <jh@jasonheath.com>
@jasonheath
Adresses rust fmt check failures
600f8f9 
Signed-off-by: Jason Heath <jh@jasonheath.com>
@jasonheath jasonheath merged commit fce0cb2 into main Oct 21, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agadgil-progress agadgil-progress agadgil-progress approved these changes

@sajjaphani sajjaphani Awaiting requested review from sajjaphani

@mwrock mwrock Awaiting requested review from mwrock

Assignees

@jasonheath jasonheath

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jasonheath @mwrock @agadgil-progress