Fix configuration directory and file permissions

[blag]

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]

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.

[arm4b]

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]

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.

[blag]

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.

[arm4b]

Thanks for explaining that.

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

1. st2 login -w (for user)
2. 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.

[blag]

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.

[arm4b]

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

[blag]

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

[arm4b]

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]

@armab Waiting on your approval to merge this.

[arm4b]

@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]

@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]

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]

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]

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]

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]

All committers have signed the CLA.