Skip to content

Conversation

@gabenp
Copy link
Contributor

@gabenp gabenp commented May 2, 2022

discourse-setup will now ensure container definitions are
installed with 0600 permissions mode only.

launcher will now throw a warning when an existing container
definition is world-readable.

Also clean up leftover launcher setup logic which no longer exists.
Merge pre-existing logic into check_prereqs function.

`discourse-setup` will now ensure container definitions are
installed with `0600` permissions mode only.

`launcher` will now throw a warning when an existing container
definition is world-readable.

Also clean up leftover `launcher setup` logic which no longer exists.
Merge pre-existing logic into `check_prereqs` function.
@gabenp gabenp requested review from Supermathie May 3, 2022 00:58
@gabenp gabenp merged commit 241a42c into main May 3, 2022
@gabenp gabenp deleted the gabe/secure-file-permissions branch May 3, 2022 15:54
@pfaffman
Copy link
Contributor

If it's important for containers to be 0600 all of a sudden, maybe it would be OK to just set them?

Most people who run ./launcher don't know what permissions are.

If it's not appropriate for launcher to change the permissions, would it be OK to at least have discourse-setup set them for people? Adding one more mysterious command to the install sequence seems unnecessary.

@CvX
Copy link
Contributor

CvX commented Jun 20, 2022

would it be OK to at least have discourse-setup set them for people?

Doesn't the change in discourse-setup handle this?

@pfaffman
Copy link
Contributor

Oh. Sorry. Perhaps I missed that too. But I just did a git pull and don't see a chmod in discourse-setup.

If you want to /ensure/ secure file permissions, is there a reason not to change them instead of complaining about them with a cryptic-to-most-people message about chmod? (Perhaps there was discussion about this that I wasn't privy or or just overlooked).

https://meta.discourse.org/t/warning-containers-app-yml-file-is-world-readable-you-can-secure-this-file-by-running-chmod-o-rwx-containers-app-yml/230439/

@CvX
Copy link
Contributor

CvX commented Jun 20, 2022

@gabenp Please take a look at that topic ^ - can we automatically handle permissions issues like that one? 😃

@gabenp
Copy link
Contributor Author

gabenp commented Jun 21, 2022

Oh. Sorry. Perhaps I missed that too. But I just did a git pull and don't see a chmod in discourse-setup.

It uses the install command to copy and set the permissions in one command, for example:

  install -v -m0600 $web_template $web_file

The idea was that any new setup will get configured securely (with restricted permissions) but we will not affect any existing setups, which may or may not rely on file access by other users.

@pfaffman
Copy link
Contributor

A thousand apologies! If I'm to offer criticism I should at least pay enough attention that I see what's going on.

I think that all makes sense. My concern started when someone noticed that warning and because of a whole bunch of other stuff that looks sort-of like warnings he went back to try to understand the rest of the stuff, at least some of which, I think, is spurious. I'm pretty sure that discourse-setup prints out a bunch of stuff that's part of a test, which is confusing (and I wrote it, so I am not blaming anyone else).

But I think I should just hush on this one.

Thanks very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants