Skip to content
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: misc zip-related fixes #44064

Merged
merged 4 commits into from
Jan 16, 2020
Merged

cli: misc zip-related fixes #44064

merged 4 commits into from
Jan 16, 2020

Conversation

knz
Copy link
Contributor

@knz knz commented Jan 16, 2020

Fixes #43966.
Mostly fixes the zip regression introduced in 19.2 by #36813.

Release note (bug fix): `cockroach zip` now emits the `goroutine`
file in the proper sub-directory when the corresponding call
fails with an error.
@knz knz requested review from ajwerner and tbg January 16, 2020 12:58
@knz knz requested a review from a team as a code owner January 16, 2020 12:58
@cockroach-teamcity
Copy link
Member

This change is Reviewable

The recent addition of the goroutine dump collection in cockroachdb#36813
introduced a bug where `debug zip` would prematurely stop if it could
not request the goroutine file list.

This patch fixes that.

Release note (bug fix): `cockroach debug zip` is again able to operate
correctly and continue to iterate over all nodes if one of the nodes
does not deliver its goroutine dumps. It would previously
prematurely and incorrectly stop with an incomplete dump; this
was a regression introduced in 19.2.
Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thank you @knz

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 2 of 2 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

@knz
Copy link
Contributor Author

knz commented Jan 16, 2020

TFYR

bors r=tbg

@knz knz added this to To do in DB Server & Security via automation Jan 16, 2020
@knz knz moved this from To do to In progress in DB Server & Security Jan 16, 2020
@craig
Copy link
Contributor

craig bot commented Jan 16, 2020

Build failed

@knz
Copy link
Contributor Author

knz commented Jan 16, 2020

let's queue this with the other one

bors r=tbg

@craig
Copy link
Contributor

craig bot commented Jan 16, 2020

Build failed (retrying...)

@knz knz moved this from In progress to Done 20.1 in DB Server & Security Jan 16, 2020
@craig
Copy link
Contributor

craig bot commented Jan 16, 2020

Build failed (retrying...)

@craig
Copy link
Contributor

craig bot commented Jan 16, 2020

Build failed (retrying...)

@knz
Copy link
Contributor Author

knz commented Jan 16, 2020

pulling this out until the bors queue clears

bors r-

@craig
Copy link
Contributor

craig bot commented Jan 16, 2020

Canceled

@knz
Copy link
Contributor Author

knz commented Jan 16, 2020

bors r=tbg

craig bot pushed a commit that referenced this pull request Jan 16, 2020
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 <knz@thaumogen.net>
@craig
Copy link
Contributor

craig bot commented Jan 16, 2020

Build succeeded

@craig craig bot merged commit 9c920eb into cockroachdb:master Jan 16, 2020
@knz knz deleted the 20200115-zip branch January 16, 2020 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cli: cockroach zip will not complete if there are decommissioned nodes
3 participants