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

add prod dirs via .gitkeep #174

Merged
merged 1 commit into from
Aug 10, 2021
Merged

add prod dirs via .gitkeep #174

merged 1 commit into from
Aug 10, 2021

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jul 28, 2021

part of ManageIQ/manageiq#20394

requires:

Goal

Manipulate the files less. Letting them get created by git and staying as hands off as possible.

Changes

  • Create directories in main repo (via .gitkeep) instead of using mkdir in the rpmspec
  • ensure world can not read sensitive files (by locking down the directory)
  • don't bother locking down files since the directories are locked.
  • set both group and user bits so files created by root will be available for processes running as manageiq.
  • put directories in core and ui sections to better designate purpose of directories.

(the changes are listed as final result, some of the changes were partially in place before this PR)

@kbrock kbrock added the enhancement New feature or request label Jul 28, 2021
@Fryguy
Copy link
Member

Fryguy commented Jul 29, 2021

@bdunne please review.

@kbrock kbrock changed the title add prod dirs [wip] add prod dirs Aug 4, 2021
@kbrock
Copy link
Member Author

kbrock commented Aug 5, 2021

update: moved chmod for core over to ui.

unwip: rpm working

@kbrock kbrock changed the title [wip] add prod dirs add prod dirs via .gitignore Aug 5, 2021
@kbrock kbrock changed the title add prod dirs via .gitignore add prod dirs via .gitkeep Aug 5, 2021
@kbrock
Copy link
Member Author

kbrock commented Aug 9, 2021

removed a few of the chown lines to hopefully make this less opinionated and make easier to merge.
We can address those changes at a later date

Comment on lines 72 to 73
%{__chmod} 6750 %{buildroot}%{app_root}/{log,config,certs}
%{__chmod} 6750 %{buildroot}%{app_root}/data/git_repos
Copy link
Member

Choose a reason for hiding this comment

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

Would it be simpler to just 6750 the entire data dir into the previous line?

Suggested change
%{__chmod} 6750 %{buildroot}%{app_root}/{log,config,certs}
%{__chmod} 6750 %{buildroot}%{app_root}/data/git_repos
%{__chmod} 6750 %{buildroot}%{app_root}/{log,config,certs,data}

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what else is in there, but I assume anything else needs writability as well

Copy link
Member Author

Choose a reason for hiding this comment

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

by default, the directories are 755. So all the lines above changing to manageiq.manageiq makes them writeable and readable.

This change says that root (basically just evmserverd and appliance_console) will write to these directories, and we want manageiq to be able to read and write to those files. (the umask for root is 755 and makes our processes blow up.)

There area also some ui directories that need access. I have moved those over to the ui spec per previous request in this pr

Copy link
Member Author

@kbrock kbrock Aug 9, 2021

Choose a reason for hiding this comment

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

^ ignore.

So we can chmod -r data which will change anything that we add at a later time.
Just need to be aware if we add any files to github, they will potentially be running with escalated privileges

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should chmod -r, because the content inside the git repos directory are external git repos. Not sure what side effects that might cause. Maybe @NickLaMuro has some thoughts here.

Copy link
Member

Choose a reason for hiding this comment

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

So, seems that git will only track the executable permissions, not the full set of nix permission levels:

https://stackoverflow.com/questions/10516201/updating-and-committing-only-a-files-permissions-using-git-version-control

So, with that said, I assume what ever we do here, it needs to be able to allow anyone that executes playbooks that might come with a role (and I assume that could include a shell/python executable) to be able to run those scripts. But that might be less of an issue since the repos here are just bare repos, and so the actually checkout happens into /tmp.

So we might be okay with whatever is done here as /tmp is where any actually interaction would come from from a potentially different and less privileged user.

Copy link
Member Author

Choose a reason for hiding this comment

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

I chmod the data dir and the git_repo

let me know if this is not what you were asking for

rpm_spec/manageiq.spec.in Outdated Show resolved Hide resolved
@kbrock
Copy link
Member Author

kbrock commented Aug 10, 2021

@Fryguy I'm confused what you want

%{__chmod} 6750 %{buildroot}%{app_root}/data/git_repos

Would it be simpler to just 6750 the entire data dir

ok

%{__chmod} -r 6750 %{buildroot}%{app_root}/data

I don't think we should chmod -r, because the content inside the git repos directory are external git repos

My goal was to chmod 6750 data/git_repos
I can chmod 750 git_repos, chown manageiq and require all manipulations to be from manageiq.
It will not be that hard to test. But I need to set git_repos.'

What exactly are you asking for?

Put something in place. Hope that is what you were asking for.

These directories are created in the core repo (via .gitkeep)
no need to create them here.

The rpm is making them writeable by user manageiq so the app
will run fine as a non-root user
@miq-bot
Copy link
Member

miq-bot commented Aug 10, 2021

Checked commit kbrock@3df8dbd with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@bdunne bdunne merged commit 1d172a5 into ManageIQ:master Aug 10, 2021
@kbrock kbrock deleted the more_dirs branch August 10, 2021 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants