security: Run the crash collector as ceph user #11219

travisn · 2022-10-26T22:35:52Z

Description of your changes:
The crash collector does not have the command line arguments to run as ceph user id 167, so we set the security context to run as the ceph user in the main crash collector container.

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide).
Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

leseb

Ideally, this is something the Ceph image would fix as well in its DockerFile, but that approach LGTM. Also, all the container specs could potentially use this as a default and we could stop using "run-as" flags altogether. Something to keep in mind for a future refactor maybe.
Last question, is this fixing a bug, or is it "just" to run non-root? Final point, I think the log rotate sidecar as the same problem.

leseb · 2022-10-27T07:18:03Z

pkg/operator/ceph/controller/spec.go

+func CephSecurityContext() *v1.SecurityContext {
+	// The ceph user has user id 167
+	context := PodSecurityContext()
+	context.RunAsUser = pointer.Int64(167)


nit: IIRC there is a constant somewhere like CephUserID.

After a quick search I didn't find one, but I'll look again, thanks

Couldn't find it, so defined one

pkg/operator/ceph/controller/spec.go

lerminou · 2022-10-27T07:37:30Z

Ideally, this is something the Ceph image would fix as well in its DockerFile,

Yes there is a security issue in their backlog for this, I sent an email yesterday to Travis to mitigate the CVE directly in Rook
I don't know if there is some logic in Ceph behind this, it seems "they suffers some delays in handling the issue"

Last question, is this fixing a bug, or is it "just" to run non-root?

yes a CVE was discovered recently in Ceph, I don't know if I can link it here ?

travisn

Ideally, this is something the Ceph image would fix as well in its DockerFile, but that approach LGTM. Also, all the container specs could potentially use this as a default and we could stop using "run-as" flags altogether. Something to keep in mind for a future refactor maybe.

I was thinking about removing the run-as flags and using the security context like this in a separate PR. It does seem like a better approach if we can just use the K8s context and no need for the ceph flags.

Last question, is this fixing a bug, or is it "just" to run non-root? Final point, I think the log rotate sidecar as the same problem.

Thanks, I'll look at the log rotate sidecar too.

travisn · 2022-10-27T13:50:04Z

pkg/operator/ceph/controller/spec.go

+func CephSecurityContext() *v1.SecurityContext {
+	// The ceph user has user id 167
+	context := PodSecurityContext()
+	context.RunAsUser = pointer.Int64(167)


After a quick search I didn't find one, but I'll look again, thanks

leseb · 2022-10-27T13:57:18Z

Ideally, this is something the Ceph image would fix as well in its DockerFile, but that approach LGTM. Also, all the container specs could potentially use this as a default and we could stop using "run-as" flags altogether. Something to keep in mind for a future refactor maybe.

I was thinking about removing the run-as flags and using the security context like this in a separate PR. It does seem like a better approach if we can just use the K8s context and no need for the ceph flags.

After thoughts, this won't be super trivial though, I had an attempt here #9175 but never had time to finish it.
I had most of working IIRC, but it's tricky so be careful :).

Last question, is this fixing a bug, or is it "just" to run non-root? Final point, I think the log rotate sidecar as the same problem.

Thanks, I'll look at the log rotate sidecar too.

The crash collector does not have the command line arguments to run as ceph user id 167, so we set the security context to run as the ceph user in the main crash collector container. Signed-off-by: Travis Nielsen <tnielsen@redhat.com>

travisn · 2022-10-27T19:17:45Z

Thanks, I'll look at the log rotate sidecar too.

The log rotate sidecar will take a bit more investigation, will work on it in a follow-up issue with #11235.

leseb · 2022-10-28T07:09:52Z

Thanks, I'll look at the log rotate sidecar too.

The log rotate sidecar will take a bit more investigation, will work on it in a follow-up issue with #11235.

Yes, I think my old PR should have what's necessary to fix it, feel free to ping me when you tackle it.

leseb

One last thing, did you verify that the crash files can be uploaded by the ceph-crash script when they appear? Normally the crash logs should be owned by ceph so this should not be a problem. Better safe than sorry though.

travisn · 2022-10-28T17:58:32Z

One last thing, did you verify that the crash files can be uploaded by the ceph-crash script when they appear? Normally the crash logs should be owned by ceph so this should not be a problem. Better safe than sorry though.

Verified both in minikube and openshift. If I look at the crash directory, the crash moved into the posted subdir, and the toolbox shows the crash was reported:

sh-4.4$ ceph crash ls
ID                                                                ENTITY  NEW  
2022-10-28T17:31:32.037827Z_c5308e72-cdee-4c4d-8158-e24476180554  mon.a    *

However, the logging in the crash collector pod makes it look like the operator failed. But I see this same log whether the crash collector is running as ceph or root (the same behavior before or after this fix). @leseb Have you seen this before?

INFO:ceph-crash:monitoring path /var/lib/ceph/crash, delay 600s
WARNING:ceph-crash:post /var/lib/ceph/crash/2022-10-28T17:31:32.037827Z_c5308e72-cdee-4c4d-8158-e24476180554 as client.crash.rook-ceph-crashcollector-7949e6200e610c9b3d040f70a100f3f4-cq5rx failed: (None, b'2022-10-28T17:36:22.538+0000 7f14db7fe700 -1 monclient(hunting): handle_auth_bad_method server allowed_methods [2] but i only support [2,1]\n2022-10-28T17:36:22.540+0000 7f14e0edb700 -1 monclient(hunting): handle_auth_bad_method server allowed_methods [2] but i only support [2,1]\n2022-10-28T17:36:22.543+0000 7f14dbfff700 -1 monclient(hunting): handle_auth_bad_method server allowed_methods [2] but i only support [2,1]\n[errno 13] RADOS permission denied (error connecting to the cluster)\n')
WARNING:ceph-crash:post /var/lib/ceph/crash/2022-10-28T17:31:32.037827Z_c5308e72-cdee-4c4d-8158-e24476180554 as client.crash failed: (None, b

leseb · 2022-10-31T09:26:43Z

One last thing, did you verify that the crash files can be uploaded by the ceph-crash script when they appear? Normally the crash logs should be owned by ceph so this should not be a problem. Better safe than sorry though.

Verified both in minikube and openshift. If I look at the crash directory, the crash moved into the posted subdir, and the toolbox shows the crash was reported:
sh-4.4$ ceph crash ls
ID                                                                ENTITY  NEW  
2022-10-28T17:31:32.037827Z_c5308e72-cdee-4c4d-8158-e24476180554  mon.a    *   
However, the logging in the crash collector pod makes it look like the operator failed. But I see this same log whether the crash collector is running as ceph or root (the same behavior before or after this fix). @leseb Have you seen this before?
INFO:ceph-crash:monitoring path /var/lib/ceph/crash, delay 600s
WARNING:ceph-crash:post /var/lib/ceph/crash/2022-10-28T17:31:32.037827Z_c5308e72-cdee-4c4d-8158-e24476180554 as client.crash.rook-ceph-crashcollector-7949e6200e610c9b3d040f70a100f3f4-cq5rx failed: (None, b'2022-10-28T17:36:22.538+0000 7f14db7fe700 -1 monclient(hunting): handle_auth_bad_method server allowed_methods [2] but i only support [2,1]\n2022-10-28T17:36:22.540+0000 7f14e0edb700 -1 monclient(hunting): handle_auth_bad_method server allowed_methods [2] but i only support [2,1]\n2022-10-28T17:36:22.543+0000 7f14dbfff700 -1 monclient(hunting): handle_auth_bad_method server allowed_methods [2] but i only support [2,1]\n[errno 13] RADOS permission denied (error connecting to the cluster)\n')
WARNING:ceph-crash:post /var/lib/ceph/crash/2022-10-28T17:31:32.037827Z_c5308e72-cdee-4c4d-8158-e24476180554 as client.crash failed: (None, b

Yes, this is very confusing but harmless. Essentially, the ceph-crash tries a bunch of users, and some fail but eventually one succeeds. That's why you see the crash being present in Ceph.

BlaineEXE

👏 👏 👏 👏 👏 👏 👏 👏 👏 👏

security: Run the crash collector as ceph user (backport #11219)

travisn requested review from leseb and BlaineEXE October 26, 2022 22:35

travisn added backport-release-1.9 backport-release-1.10 labels Oct 26, 2022

leseb reviewed Oct 27, 2022

View reviewed changes

pkg/operator/ceph/controller/spec.go Show resolved Hide resolved

travisn commented Oct 27, 2022

View reviewed changes

security: run the crash collector as ceph user

df6d7af

The crash collector does not have the command line arguments to run as ceph user id 167, so we set the security context to run as the ceph user in the main crash collector container. Signed-off-by: Travis Nielsen <tnielsen@redhat.com>

travisn force-pushed the crash-collector-user branch from eac2932 to df6d7af Compare October 27, 2022 19:13

leseb reviewed Oct 28, 2022

View reviewed changes

travisn requested a review from leseb October 28, 2022 17:58

BlaineEXE approved these changes Oct 31, 2022

View reviewed changes

travisn merged commit bb91d56 into rook:master Oct 31, 2022
50 checks passed

This was referenced Oct 31, 2022

security: Run the crash collector as ceph user (backport #11219) #11249

Merged

security: Run the crash collector as ceph user (backport #11219) #11250

Merged

mergify bot added a commit that referenced this pull request Oct 31, 2022

Merge pull request #11250 from rook/mergify/bp/release-1.10/pr-11219

771e206

security: Run the crash collector as ceph user (backport #11219)

mergify bot added a commit that referenced this pull request Oct 31, 2022

Merge pull request #11249 from rook/mergify/bp/release-1.9/pr-11219

759f6a2

security: Run the crash collector as ceph user (backport #11219)

travisn deleted the crash-collector-user branch November 1, 2022 15:12

travisn mentioned this pull request Nov 1, 2022

Log rotate sidecar should run as ceph user #11235

Closed

travisn mentioned this pull request Nov 14, 2022

Snyk security report incorrectly reporting a failure #11312

Closed

security: Run the crash collector as ceph user #11219

security: Run the crash collector as ceph user #11219

travisn commented Oct 26, 2022

leseb left a comment

leseb Oct 27, 2022

travisn Oct 27, 2022

travisn Oct 27, 2022

lerminou commented Oct 27, 2022 •

edited

travisn left a comment

travisn Oct 27, 2022

leseb commented Oct 27, 2022

travisn commented Oct 27, 2022

leseb commented Oct 28, 2022

leseb left a comment

travisn commented Oct 28, 2022

leseb commented Oct 31, 2022

BlaineEXE left a comment

security: Run the crash collector as ceph user #11219

security: Run the crash collector as ceph user #11219

Conversation

travisn commented Oct 26, 2022

leseb left a comment

Choose a reason for hiding this comment

leseb Oct 27, 2022

Choose a reason for hiding this comment

travisn Oct 27, 2022

Choose a reason for hiding this comment

travisn Oct 27, 2022

Choose a reason for hiding this comment

lerminou commented Oct 27, 2022 • edited

travisn left a comment

Choose a reason for hiding this comment

travisn Oct 27, 2022

Choose a reason for hiding this comment

leseb commented Oct 27, 2022

travisn commented Oct 27, 2022

leseb commented Oct 28, 2022

leseb left a comment

Choose a reason for hiding this comment

travisn commented Oct 28, 2022

leseb commented Oct 31, 2022

BlaineEXE left a comment

Choose a reason for hiding this comment

lerminou commented Oct 27, 2022 •

edited