-
-
Notifications
You must be signed in to change notification settings - Fork 749
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
Fix config permissions on the config directory and the config file #4173
Conversation
st2client/st2client/base.py
Outdated
@@ -225,7 +225,7 @@ def _get_cached_auth_token(self, client, username, password): | |||
:rtype: ``str`` | |||
""" | |||
if not os.path.isdir(ST2_CONFIG_DIRECTORY): | |||
os.makedirs(ST2_CONFIG_DIRECTORY) | |||
os.makedirs(ST2_CONFIG_DIRECTORY, mode=0o770) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also set setgid
for the config dir? 2770
,
see #4144 and StackStorm/packer-st2#38 why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll do this.
I assume we also want the setuid
bit set, right? 0o6770
?
Edit: Nope, nevermind: https://en.wikipedia.org/wiki/Setuid#When_set_on_a_directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any opinions about the setuid
, since I didn't see that bit having effect on dir.
If you believe it can help in some use cases, - I'm good with adding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I don't think it's helpful.
Fixed.
My vote goes in favor of:
Create config dir and files with safe permissions and don't try to change them after that. Curious what others think. |
To fix the entire story, need also to take care about the StackStorm/st2-packages#558 (StackStorm/st2-packages#520 might help with it) |
I agree with @armab, I also think 1) is better (user might have a (valid) use case to change the permissions after the fact and we probably shouldn't mess with the permissions after the initial set up, but should definitely warn in case of unsafe permissions). |
92e8f5a
to
591e608
Compare
st2client/st2client/config_parser.py
Outdated
config_dir_path = os.path.dirname(self.config_file_path) | ||
|
||
if bool(os.stat(config_dir_path).st_mode ^ 0o770): | ||
warnings.warn('Setting StackStorm config directory permissions ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using warnings
module we should probably use logger.warn
since we also use logging in other places for such things.
This way it uses the existing framework and setting log level, etc works as expected (e.g. user wants to suppress warnings and only see messages with level ERROR and above).
33705c1
to
c3a65ae
Compare
c3a65ae
to
5479226
Compare
Can you please add a changelog entry? Besides that, LGTM 👍 |
st2client/st2client/config_parser.py
Outdated
self.LOG.warn( | ||
# TODO: Perfect place for an f-string | ||
"The StackStorm configuration directory permissions are " | ||
"insecure (SGID bit not set)." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a best to warn user about the absence of SGID bit.
It's a workaround that'll help creating file names with the same group under the config dir, but not really that much security-related.
Besides of that minor comment, looks good to me too 👍
st2client/st2client/base.py
Outdated
@@ -225,7 +225,7 @@ def _get_cached_auth_token(self, client, username, password): | |||
:rtype: ``str`` | |||
""" | |||
if not os.path.isdir(ST2_CONFIG_DIRECTORY): | |||
os.makedirs(ST2_CONFIG_DIRECTORY) | |||
os.makedirs(ST2_CONFIG_DIRECTORY, mode=0o770) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder to not forget to set setguid
bit before merging. Can't find it anywhere, except of tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, please re-review when you have time.
528d61d
to
fbebae3
Compare
os.makedirs(ST2_CONFIG_DIRECTORY, mode=0o2770) | ||
# os.makedirs straight up ignores the setgid bit, so we have to set | ||
# it manually | ||
os.chmod(ST2_CONFIG_DIRECTORY, 0o2770) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense in dir conext 👍
with os.fdopen(fd, 'w') as fp: | ||
fp.write(data) | ||
os.chmod(cached_token_path, 0o660) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this always try to chmod cached token file?
I think we previously agreed to do chmod operations only on initial file creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it would, but as far as I can tell, it's only run when the file is initially created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, thanks for explaining 👍
st2client/st2client/commands/auth.py
Outdated
@@ -140,6 +141,7 @@ def run(self, args, **kwargs): | |||
|
|||
with open(config_file, 'w') as cfg_file_out: | |||
config.write(cfg_file_out) | |||
os.chmod(config_file, mode=0o660) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. I think we previously agreed to set file permissions only on creation and don't try chmod afterwards.
st2client/st2client/config_parser.py
Outdated
"\n\n" | ||
"You can fix this by running:" | ||
"\n\n" | ||
"chmod g+s {config_dir}".format(config_dir=config_dir_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the LOG.info
message level, do you know/can you check where will it show while running st2
commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That depends ~~greatly on the configuration file itself~ on the --debug
flag, but the info
level is not output to stdout or stderr by default:
>>> import logging
>>> logger = logging.getLogger("_")
>>> logger.info("Hello World!")
>>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for verifying that 👍
fbebae3
to
1bce27b
Compare
I tried the packages produced from this PR and got the following error:
|
Also squashing all commits history in 1 during review makes it harder to follow the history, code diff from the most recent look and overall commenting. |
Can we get this resolved / finished today and merged in time for v2.8.0? |
1bce27b
to
6dfc81c
Compare
st2client/st2client/config_parser.py
Outdated
"\n\n" | ||
"You can fix this by running:" | ||
"\n\n" | ||
"chmod 770 {config_dir}".format(config_dir=config_dir_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that previous commit fixes st2client.config_parser
issue.
There is a new edge case, warning messages are duplicated:
$ st2 action list
2018-06-22 17:41:48,953 WARNING - The StackStorm configuration directory permissions are insecure (too permissive).
You can fix this by running:
chmod 770 /home/vagrant/.st2
2018-06-22 17:41:48,954 WARNING - The StackStorm configuration file permissions are insecure.
You can fix this by running:
chmod 660 /home/vagrant/.st2/config
2018-06-22 17:41:48,955 WARNING - The StackStorm configuration directory permissions are insecure (too permissive).
You can fix this by running:
chmod 770 /home/vagrant/.st2
2018-06-22 17:41:48,957 WARNING - The StackStorm configuration file permissions are insecure.
You can fix this by running:
chmod 660 /home/vagrant/.st2/config
2018-06-22 17:41:48,959 WARNING - Permissions (0664) for cached token file "/home/vagrant/.st2/token-st2admin" are to permissive. Please restrict the permissions and make sure only your own user can read from the file
To reproduce:
chmod -R o+r ~/.st2
st2 action list
98cb224
to
caa90ff
Compare
a23403f
to
fd5d3cf
Compare
CHANGELOG.rst
Outdated
@@ -45,6 +45,8 @@ Added | |||
Changed | |||
~~~~~~~ | |||
|
|||
* Update st2 CLI to create the configuration directory and file, and authentication tokens with | |||
secure permissions (eg: readable only to owner) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: To include the PR #number in the CHANGELOG.
st2client/st2client/base.py
Outdated
'from the file' % (file_st_mode, cached_token_path)) | ||
self.LOG.warn(message) | ||
'from or write to the file.' | ||
'\n\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I think that's too many new lines here which makes it pretty inconsistent with existing stackstorm warning messages by taking a lot of space. Example:
vagrant@stackstorm:~$ st2 action list
2018-06-26 14:50:42,463 WARNING - The StackStorm configuration directory permissions are insecure (too permissive).
You can fix this by running:
chmod 770 /home/vagrant/.st2
2018-06-26 14:50:42,463 WARNING - The StackStorm configuration file permissions are insecure.
You can fix this by running:
chmod 660 /home/vagrant/.st2/config
2018-06-26 14:50:42,463 WARNING - Permissions (0664) for cached token file "/home/vagrant/.st2/token-st2admin" are too permissive. Please restrict the permissions and make sure only your own user can read from or write to the file.
You can fix this by running:
chmod o-rw /home/vagrant/.st2/token-st2admin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nitpick - In some places in the log messages we use numerical permission levels and in other we use symbolic notation.
It's probably a good idea to unify it and use numerical / symbolic everywhere :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll simply remove the help text. I just usually like to give users some idea of how to fix a problem, which requires readable formatting, but it might not be the best for log messages.
The octal vs symbolic issue is more than just a notation difference, it's a functional one. The octal notation sets the permissions for the user, group, and "others", but the symbolic notation here performs operations on the permissions for just "others" and leaves the bits for the user and group intact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing all the corner cases. LGTM now 👍
Just left a couple of very minor non-blocking comments, if you'd prefer to further improve it for consistency.
Probably a good idea to get this merged today so we can include it in v2.8.0. Those non-blockers mentioned above can be fixed / improved later. |
Warnings and tests are turned off in #4215 (41769824b51aa19edb786a9e7e4b7f91b16f65d7 and e16a59ac029926f63fa1eb507653f19fef18f4c7). |
Fixes #4144.
This ensures that we create the config directory and file with the proper permissions.
It also tweaks the
st2 login
command to check the permissions on the config file (~/.st2/config
) and directory (~/.st2
) every time it is run, and then automatically fix permissions if they are too permissive. It does use thewarnings
module to warn the user about this, but does not give them a chance to opt out or turn the behavior off.Automatically fixing the directory/file permissions assumes that there is no good reason for a process owned by another user or group to need read access to the config file. That is a very large assumption, so I'm inviting discussion about that.
The options are:
I have chosen option 3 here.