-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
cli,log: change the default file permissions #44043
Conversation
5c50055
to
5687e3a
Compare
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.
Fine by me. I wonder if/when we're going to find that we broke somebody's log ingestion flow.
Reviewed 4 of 4 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)
a8fc921
to
9d33982
Compare
Prior to this patch, files created by a CockroachDB node were created with the default umask (usualy 0777) and permission bits 0644, i.e. `-rw-r--r--`. This situation is defective because in most common deployments umask does not exclude the "other" bits and the resulting data, log and temp files are world-readable. This is unsafe because any of these files can contain sensitive information. As Aaron puts it: > If another service on the system has an arbitrary file read > vulnerability but it is running under its own user account, limiting > these permissions would limit the potential impact of such a > vulnerability. Whereas in the current setup, an attacker could real > CRDB sensitive information from any other compromised user access. This patch rectifies this situation by enforcing at least bits 0007 in the umask early in `cockroach start`, so that created files, directories, symlinks etc have at most `-rw-rw----` (most will be `-rw-r-----`). Release note (security update): A CockroachDB node process (`start` / `start-single-node`) now configures its umask to create all its files without unix permission bits for "others", so that data/log/etc files do not become world-readable by default in systems that do not otherwise customize umask to enforce log file visibility. The files produced by other `cockroach` commands (e.g. the CLI commands) do not force their umask. Note that after upgrading to this release, in sites where permissions matter administrators should be careful to run `chmod -R o-rwx` in directories where files were created by a previous version. Release note (backward-incompatible change): CockroachDB now creates files without read permissions for the "others" group. Sites that automate file management (e.g. log collection) using multiple user accounts now must ensure that the CockroachDB server and the management tools running on the same system are part of a shared unix group.
Release note (bug fix): The file generated by `cockroach gen haproxy` does not get an executable bit any more. The executable bit was previously placed in error.
Release note: None
9d33982
to
3b9cffc
Compare
So for reference none of our CI test suite is affected by this change. Yay. |
bors r=ajwerner |
Build failed (retrying...) |
Build failed (retrying...) |
temporarily pulling out until bors queue is cleared bors r- |
Canceled |
bors r=ajwerner |
44043: cli,log: change the default file permissions r=ajwerner a=knz Requested/recommended by @aaron-crl in cockroachdb/docs#5444 Prior to this patch, files created by a CockroachDB node were created with the default umask (usualy 0777) and permission bits 0644, i.e. `-rw-r--r--`. This situation is defective because in most common deployments umask does not exclude the "other" bits and the resulting data, log and temp files are world-readable. This is unsafe because any of these files can contain sensitive information. As Aaron puts it: > If another service on the system has an arbitrary file read > vulnerability but it is running under its own user account, limiting > these permissions would limit the potential impact of such a > vulnerability. Whereas in the current setup, an attacker could real > CRDB sensitive information from any other compromised user access. This patch rectifies this situation by enforcing at least bits 0007 in the umask early in `cockroach start`, so that created files, directories, symlinks etc have at most `-rw-rw----` (most will be `-rw-r-----`). Release note (security update): A CockroachDB node process (`start` / `start-single-node`) now configures its umask to create all its files without unix permission bits for "others", so that data/log/etc files do not become world-readable by default in systems that do not otherwise customize umask to enforce log file visibility. The files produced by other `cockroach` commands (e.g. the CLI commands) do not force their umask. Release note (backward-incompatible change): CockroachDB now creates files without read permissions for the "others" group. Sites that automate file management (e.g. log collection) using multiple user accounts now must ensure that the CockroachDB server and the management tools running on the same system are part of a shared unix group. 44064: cli: misc zip-related fixes r=tbg a=knz Fixes #43966. Mostly fixes the `zip` regression introduced in 19.2 by #36813. Co-authored-by: Raphael 'kena' Poss <[email protected]>
Build succeeded |
Prior to this patch, the code in `runStartSQL` was using a different sequence of steps than in `runStart`, even for those steps that are beneficial to run in both cases. This commit fixes that. In particular it adds the following missing bits to `cockroach mt start-sql`: - it fixes support for the (test-only) `COCKROACH_EXPERIMENTAL_LINEARIZABLE` env var. (from cockroachdb#4754) - it adds a tracing span for the startup code. (from cockroachdb#8712) - it properly supports `--listening-url-file`. (from cockroachdb#15468) - it properly does sanitization of `--external-io-dir`. (from cockroachdb#19725) - it sets the proper log severity level for gRPC. (from cockroachdb#20308) - it reports the command-line and env config to logs. (from cockroachdb#21344) - it prevents startup if there is a `_CRITICAL_ALERT.txt` file in the store directory. (from cockroachdb#42401) - sets the umask for newly created file to remove "other" permission bits. This was a security team request originally. (from cockroachdb#44043) - it sets `GOMAXPROCS` from current cgroup limits. (from cockroachdb#57390) - it stops the server early if the storage disk is full. (from cockroachdb#66893) - it fixes support for the `COCKROACH_DISABLE_SPAN_CONFIGS` config env var. (from cockroachdb#73876) To review this commit, it is useful to open the files `start.go` and `mt_start_sql.go` side-by-side, which clarifies how much closer they have become to each other. Looking at the `mt_start_sql.go` changes without that context likely makes the review more difficult. A later commit will actually merge the two code paths together so there is just one thing to maintain. Release note: None
Prior to this patch, the code in `runStartSQL` was using a different sequence of steps than in `runStart`, even for those steps that are beneficial to run in both cases. This commit fixes that. In particular it adds the following missing bits to `cockroach mt start-sql`: - it fixes support for the (test-only) `COCKROACH_EXPERIMENTAL_LINEARIZABLE` env var. (from cockroachdb#4754) - it adds a tracing span for the startup code. (from cockroachdb#8712) - it properly supports `--listening-url-file`. (from cockroachdb#15468) - it properly does sanitization of `--external-io-dir`. (from cockroachdb#19725) - it sets the proper log severity level for gRPC. (from cockroachdb#20308) - it reports the command-line and env config to logs. (from cockroachdb#21344) - it prevents startup if there is a `_CRITICAL_ALERT.txt` file in the store directory. (from cockroachdb#42401) - sets the umask for newly created file to remove "other" permission bits. This was a security team request originally. (from cockroachdb#44043) - it sets `GOMAXPROCS` from current cgroup limits. (from cockroachdb#57390) - it stops the server early if the storage disk is full. (from cockroachdb#66893) - it fixes support for the `COCKROACH_DISABLE_SPAN_CONFIGS` config env var. (from cockroachdb#73876) To review this commit, it is useful to open the files `start.go` and `mt_start_sql.go` side-by-side, which clarifies how much closer they have become to each other. Looking at the `mt_start_sql.go` changes without that context likely makes the review more difficult. A later commit will actually merge the two code paths together so there is just one thing to maintain. Release note: None
Prior to this patch, the code in `runStartSQL` was using a different sequence of steps than in `runStart`, even for those steps that are beneficial to run in both cases. This commit fixes that. In particular it adds the following missing bits to `cockroach mt start-sql`: - it fixes support for the (test-only) `COCKROACH_EXPERIMENTAL_LINEARIZABLE` env var. (from cockroachdb#4754) - it adds a tracing span for the startup code. (from cockroachdb#8712) - it properly supports `--listening-url-file`. (from cockroachdb#15468) - it properly does sanitization of `--external-io-dir`. (from cockroachdb#19725) - it sets the proper log severity level for gRPC. (from cockroachdb#20308) - it reports the command-line and env config to logs. (from cockroachdb#21344) - it prevents startup if there is a `_CRITICAL_ALERT.txt` file in the store directory. (from cockroachdb#42401) - sets the umask for newly created file to remove "other" permission bits. This was a security team request originally. (from cockroachdb#44043) - it sets `GOMAXPROCS` from current cgroup limits. (from cockroachdb#57390) - it stops the server early if the storage disk is full. (from cockroachdb#66893) - it fixes support for the `COCKROACH_DISABLE_SPAN_CONFIGS` config env var. (from cockroachdb#73876) To review this commit, it is useful to open the files `start.go` and `mt_start_sql.go` side-by-side, which clarifies how much closer they have become to each other. Looking at the `mt_start_sql.go` changes without that context likely makes the review more difficult. A later commit will actually merge the two code paths together so there is just one thing to maintain. Release note: None
Prior to this patch, the code in `runStartSQL` was using a different sequence of steps than in `runStart`, even for those steps that are beneficial to run in both cases. This commit fixes that. In particular it adds the following missing bits to `cockroach mt start-sql`: - it fixes support for the (test-only) `COCKROACH_EXPERIMENTAL_LINEARIZABLE` env var. (from cockroachdb#4754) - it adds a tracing span for the startup code. (from cockroachdb#8712) - it properly supports `--listening-url-file`. (from cockroachdb#15468) - it properly does sanitization of `--external-io-dir`. (from cockroachdb#19725) - it sets the proper log severity level for gRPC. (from cockroachdb#20308) - it reports the command-line and env config to logs. (from cockroachdb#21344) - it prevents startup if there is a `_CRITICAL_ALERT.txt` file in the store directory. (from cockroachdb#42401) - sets the umask for newly created file to remove "other" permission bits. This was a security team request originally. (from cockroachdb#44043) - it sets `GOMAXPROCS` from current cgroup limits. (from cockroachdb#57390) - it stops the server early if the storage disk is full. (from cockroachdb#66893) - it fixes support for the `COCKROACH_DISABLE_SPAN_CONFIGS` config env var. (from cockroachdb#73876) To review this commit, it is useful to open the files `start.go` and `mt_start_sql.go` side-by-side, which clarifies how much closer they have become to each other. Looking at the `mt_start_sql.go` changes without that context likely makes the review more difficult. A later commit will actually merge the two code paths together so there is just one thing to maintain. Release note: None
90176: cli/start: unify code between `cockroach start` and `cockroach mt start-sql` r=andreimatei a=knz Fixes #89974. Fixes #90831. Fixes #90524. This PR merges the server initialization code between `cockroach start` and `cockroach mt start-sql`. In doing so, it brings `cockroach mt start-sql` closer to what we expect from proper CockroachDB server processes: - it fixes support for the (test-only) `COCKROACH_EXPERIMENTAL_LINEARIZABLE` env var. (from #4754) - it adds a tracing span for the startup code. (from #8712!!) - it properly supports `--listening-url-file`. (from #15468) - it properly does sanitization of `--external-io-dir`. (from #19725) - it sets the proper log severity level for gRPC. (from #20308) - it reports the command-line and env config to logs. (from #21344) - it prevents startup if there is a `_CRITICAL_ALERT.txt` file in the store directory. (from #42401) - sets the umask for newly created file to remove "other" permission bits. This was a security team request originally. (from #44043) - it recovers support for `DISABLE_STARTING_BACKGROUND_JOBS`. (from #44786) - it sets `GOMAXPROCS` from current cgroup limits. (from #57390) - it stops the server early if the storage disk is full. (from #66893) - it fixes support for the `COCKROACH_DISABLE_SPAN_CONFIGS` config env var. (from #73876) See the individual commit for details. 90660: sql: add contention_events to cluster_execution_insights r=j82w a=j82w The original contention column will remain to make query operations faster. The events are being put into a json column because it's possible there could be multiple blocking events for a single statement. The json column avoids the complexity of adding another table and keeping it in sync with the insights table. The table can be joined with index_columns and tables to get the database name, table name, and index name that contention occurred on. This does not contain the blocking statement information, and the blocking fingerprint id. closes: #88561 Release note (sql change): Adds contention_events to cluster_execution_insights. This is used to see which transaction is blocking the specific statement. 90719: opgen: added a bool field in struct opgen.transition r=Xiang-Gu a=Xiang-Gu This PR adds a bool field in struct opgen.transition that indicates whether it results from a `equiv(xx)` transition spec in the opgen file. It will be useful for a test where we need to find the inital status on a adding/dropping path. Without such a change, it can be problematic if we have a `equiv(xx)` spec as the first transition. E.g. ``` ToAbsent( PUBLIC, equiv(VALIDATED), to(WRITE_ONLY), to(ABSENT), ) ``` Without this change, the inital status will confusingly be `VALIDATED`, and the next status will be `PUBLIC`. With this change, the initial status will be `PUBLIC`, and the next status will be `WRITE_ONLY`. We also added some comments when we make transitions from the specs. Epic: None Release note: None 90865: sql: use bare name string of new pk to compare with pk name when altering primary key r=chengxiong-ruan a=chengxiong-ruan Fixes #90836 Release note (sql change): previously, the `DROP CONSTRAINT, ADD CONSTRAINT` in one trick to have a new primary key without moving old primary key to be a secondary index didn't work if the primary key name is a reserved SQL keyword. A `constraint already exists` error was returned. This patch fixed the bug, the trick now also works with primary key named as reserved keywords. Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: j82w <[email protected]> Co-authored-by: Xiang Gu <[email protected]> Co-authored-by: Chengxiong Ruan <[email protected]>
Requested/recommended by @aaron-crl in cockroachdb/docs#5444
Prior to this patch, files created by a CockroachDB node were created
with the default umask (usualy 0777) and permission bits 0644,
i.e.
-rw-r--r--
.This situation is defective because in most common deployments umask
does not exclude the "other" bits and the resulting data, log and temp
files are world-readable. This is unsafe because any of these files
can contain sensitive information.
As Aaron puts it:
This patch rectifies this situation by enforcing at least bits 0007 in
the umask early in
cockroach start
, so that created files,directories, symlinks etc have at most
-rw-rw----
(most will be-rw-r-----
).Release note (security update): A CockroachDB node process (
start
/start-single-node
) now configures its umask to create all its fileswithout unix permission bits for "others", so that data/log/etc files
do not become world-readable by default in systems that do not
otherwise customize umask to enforce log file visibility. The files
produced by other
cockroach
commands (e.g. the CLI commands) do notforce their umask.
Release note (backward-incompatible change): CockroachDB now creates
files without read permissions for the "others" group. Sites that
automate file management (e.g. log collection) using multiple user
accounts now must ensure that the CockroachDB server and the
management tools running on the same system are part of a shared unix
group.