Fix configuration directory and file permissions #563

blag · 2018-06-14T23:52:53Z

Companion PR to st2#4173.

Fixes configuration directory permissions to 2770 and the configuration file permissions to 660.

Fixes #558
Closes #520
Closes #565

arm4b

Just setting dir permissions in specific installer won't help much, because there are many other installers too (Ansible, packages, Puppet, Chef, Docker, etc).

Instead we have to rely on st2 login -w, and avoid creating CLI config files in curl|bash installer.
The best is to keep this responsibility on st2 as single source of config creation logic.

See #520. We didn't had st2 login -w at that moment. Now it's a good time to switch.

arm4b · 2018-06-20T17:40:13Z

scripts/includes/common.sh

-username = ${USERNAME}
-password = ${PASSWORD}
-EOT"
+  sudo sh -c st2 login --config ${CURRENT_USER_CLI_CONFIG_PATH} \


Any reason why using sudo and sh is required? Ideally is to avoid something like that, making sure that st2 does the right thing.

Just st2 login would be enough.

Alright, as described in another comment, 1 run sudo st2 login -w is needed to write config for root and another st2 login -w is needed to write user's config.

Still adding sh -c is excessive in this scenario.

arm4b · 2018-06-20T17:44:30Z

scripts/includes/common.sh

-username = ${USERNAME}
-password = ${PASSWORD}
-EOT"
+  sudo sh -c st2 login --config ${ROOT_USER_CLI_CONFIG_PATH} \


Do we really need explicit ROOT_USER_CLI_CONFIG_PATH?

With that, the logic defined in configure_st2_cli_config() needs a cleanup. All dir/config file creation should be a st2 login -w responsibility.

Do we really need explicit ROOT_USER_CLI_CONFIG_PATH?

Technically, no, but it makes it easier to verify that the two calls to st2 login (one for root, one for the user) are doing similar things.

I have found it easier to make more variables in Bash than to hard code everything.

All dir/config file creation should be a st2 login -w responsibility.

Yep, fixed.

Thanks for explaining that.

To ensure st2 login config is created for both user and root, would it be enough to run

st2 login -w (for user)

sudo st2 login -w (for root)

Can st2 login handle that correctly?

Additionally, there is no such --config option for st2 login command.
We shouldn't hardcode the dirs in every installer. It's a st2 responsibility to decide on config dirs.

Can st2 login handle that correctly?

Should be able to, yes.

Additionally, there is no such --config option for st2 login command.

Good catch, thanks! Fixed.

We shouldn't hardcode the dirs in every installer. It's a st2 responsibility to decide on config dirs.

That's true, but I'm just trying to maintain backwards compatibility. We can take care of that in a different PR.

scripts/st2bootstrap-deb.sh

arm4b · 2018-06-21T11:47:13Z

scripts/st2bootstrap-deb.sh


  # Fix the permissions
-  sudo chown -R ${CURRENT_USER}:${CURRENT_USER} ${CURRENT_USER_CLI_CONFIG_DIRECTORY}
+  chown -R ${CURRENT_USER}:${CURRENT_USER} ${CURRENT_USER_CLI_CONFIG_DIRECTORY}


Don't need to chown anymore.

Keep in mind that curl|bash installer is not idempotent and runs only once (first install), and so the config files/dirs are created with the correct permissions on first run making chown excessive in this case.

Backwards compatibility you mentioned should be guaranteed by st2 login functionality.
That's why we moved this permissions logic into st2 login itself, see: StackStorm/st2#4173

…-file option to st2 instead of the login subcommand

blag · 2018-06-22T00:33:22Z

@armab I fixed the issues you pointed out. Let me know if there's anything else I need to fix.

arm4b · 2018-06-22T16:42:58Z

Thanks @blag! The logic LGTM.
The only requirement is to test the installer script end-to-end once we'll have the st2 StackStorm/st2#4173 merged.

blag · 2018-06-28T08:49:23Z

@armab Waiting on your approval to merge this.

arm4b · 2018-06-28T11:49:04Z

@blag Per previous discussion, did you end-to-end test this change manually and can confirm working behavior?
We need to ensure that it doesn't break anything prior merging.

Kami · 2018-06-29T08:50:14Z

@armab @blag We merged st2 change, but not this one so now if you install StackStorm using installer script it will produce warnings by default.

I would say merge that if it's working so we get rid of warnings on a clean installation. Everything needs to be tested end to end though, of course.

Kami · 2018-06-29T08:51:54Z

I will merge it and test it out.

If it doesn't work, I will revert it since we don't want to delay the release any longer.

Kami · 2018-06-29T09:05:16Z

I pushed a fix, yet things are still breaking (38fdd4e). This requires more work.

@blag for future reference, that's how you tested installer script changes from a branch:

wget https://stackstorm.com/packages/install.sh
bash install.sh --user=st2admin --password=Ch@ngeMe --staging --unstable --force-branch=fix-config-permissions

The problem seems to be that st2 login command depends on the config directory and config file to exist.

This means we will probably still need to manually create and chmod directories and config inside the installer script. Either that, or change st2 CLI to create directories and file if they don't exist yet, but that's a lot more involved.

Kami

As per my comment in #563 (comment), it needs more work.

arm4b · 2018-06-29T11:55:41Z

Either that, or change st2 CLI to create directories and file if they don't exist yet, but that's a lot more involved.

Agree, that's the point which I'd like us ideally to follow.
But it's easier, because st2 login -w CLI can already create config files and dirs if they don't exist yet.

We just need to get rid of all the config/dir paths in scripted installer and let the st2 handle it correctly. This leaves scripted installer with less logic to juggle with which is a good part.

arm4b · 2018-06-29T16:14:21Z

With this release timing pressure, I've made a change to this PR to have a more simplified & working st2 config behavior in scripted installer. Per quick manual tests it looks ok:

curl -sSL https://stackstorm.com/packages/install.sh | bash -s -- --user=st2admin --password=st2admin --staging --unstable --force-branch=fix-config-permissions

[...]

$ ls -la ~/.st2/
total 16
drwxrws--- 2 vagrant vagrant 4096 Jun 29 16:09 .
drwxr-xr-x 5 vagrant vagrant 4096 Jun 29 16:11 ..
-rw-rw---- 1 vagrant vagrant   55 Jun 29 16:09 config
-rw-rw---- 1 vagrant vagrant   77 Jun 29 16:09 token-st2admin

$ sudo ls -la /root/.st2/
total 16
drwxrws--- 2 root root 4096 Jun 29 16:09 .
drwx------ 5 root root 4096 Jun 29 16:10 ..
-rw-rw---- 1 root root   55 Jun 29 16:09 config
-rw-rw---- 1 root root   77 Jun 29 16:09 token-st2admin

@Kami Please take a look again.
@blag Please help to test it end-to-end as there may be corner cases, @Kami provided instructions above.

CLAassistant · 2022-05-11T11:00:11Z

All committers have signed the CLA.

arm4b suggested changes Jun 15, 2018

View reviewed changes

arm4b added the bash installer label Jun 15, 2018

blag force-pushed the fix-config-permissions branch 2 times, most recently from 49cec89 to c16ba54 Compare June 19, 2018 22:49

arm4b suggested changes Jun 20, 2018

View reviewed changes

Fix configuration directory and file permissions

de8a668

blag force-pushed the fix-config-permissions branch from c16ba54 to de8a668 Compare June 20, 2018 21:48

arm4b reviewed Jun 21, 2018

View reviewed changes

scripts/st2bootstrap-deb.sh Outdated Show resolved Hide resolved

arm4b reviewed Jun 21, 2018

View reviewed changes

Don't redundantly chown the configuration file, and move the --config…

2c2cad3

…-file option to st2 instead of the login subcommand

blag force-pushed the fix-config-permissions branch from 8269ead to 2c2cad3 Compare June 21, 2018 20:20

Remove unnecessary chown

4c6aff4

Kami added 3 commits June 29, 2018 10:56

Merge branch 'master' into fix-config-permissions

6413687

Fix syntax, username is a position argument.

38fdd4e

Re-generate scripts.

6c58eb4

Kami requested changes Jun 29, 2018

View reviewed changes

Fix scripted installer to use 'st2 login' to write password

ef10ecc

arm4b added this to the 2.9.0 milestone Jul 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix configuration directory and file permissions #563

Fix configuration directory and file permissions #563

blag commented Jun 14, 2018 •

edited by arm4b

arm4b left a comment •

edited

arm4b Jun 20, 2018

arm4b Jun 20, 2018 •

edited

arm4b Jun 20, 2018 •

edited

blag Jun 20, 2018

arm4b Jun 20, 2018 •

edited

blag Jun 20, 2018

arm4b Jun 21, 2018 •

edited

blag commented Jun 22, 2018

arm4b commented Jun 22, 2018

blag commented Jun 28, 2018

arm4b commented Jun 28, 2018 •

edited

Kami commented Jun 29, 2018

Kami commented Jun 29, 2018

Kami commented Jun 29, 2018 •

edited

Kami left a comment

arm4b commented Jun 29, 2018 •

edited

arm4b commented Jun 29, 2018 •

edited

CLAassistant commented May 11, 2022 •

edited

Fix configuration directory and file permissions #563

Are you sure you want to change the base?

Fix configuration directory and file permissions #563

Conversation

blag commented Jun 14, 2018 • edited by arm4b

arm4b left a comment • edited

Choose a reason for hiding this comment

arm4b Jun 20, 2018

Choose a reason for hiding this comment

arm4b Jun 20, 2018 • edited

Choose a reason for hiding this comment

arm4b Jun 20, 2018 • edited

Choose a reason for hiding this comment

blag Jun 20, 2018

Choose a reason for hiding this comment

arm4b Jun 20, 2018 • edited

Choose a reason for hiding this comment

blag Jun 20, 2018

Choose a reason for hiding this comment

arm4b Jun 21, 2018 • edited

Choose a reason for hiding this comment

blag commented Jun 22, 2018

arm4b commented Jun 22, 2018

blag commented Jun 28, 2018

arm4b commented Jun 28, 2018 • edited

Kami commented Jun 29, 2018

Kami commented Jun 29, 2018

Kami commented Jun 29, 2018 • edited

Kami left a comment

Choose a reason for hiding this comment

arm4b commented Jun 29, 2018 • edited

arm4b commented Jun 29, 2018 • edited

CLAassistant commented May 11, 2022 • edited

blag commented Jun 14, 2018 •

edited by arm4b

arm4b left a comment •

edited

arm4b Jun 20, 2018 •

edited

arm4b Jun 20, 2018 •

edited

arm4b Jun 20, 2018 •

edited

arm4b Jun 21, 2018 •

edited

arm4b commented Jun 28, 2018 •

edited

Kami commented Jun 29, 2018 •

edited

arm4b commented Jun 29, 2018 •

edited

arm4b commented Jun 29, 2018 •

edited

CLAassistant commented May 11, 2022 •

edited