Skip to content

cephadm: Give iscsci a RO /lib/modules bind mounted#35141

Merged
sebastian-philipp merged 1 commit intoceph:masterfrom
matthewoliver:cephadm_iscsi_lib_module_mount
Jun 19, 2020
Merged

cephadm: Give iscsci a RO /lib/modules bind mounted#35141
sebastian-philipp merged 1 commit intoceph:masterfrom
matthewoliver:cephadm_iscsi_lib_module_mount

Conversation

@matthewoliver
Copy link
Contributor

@matthewoliver matthewoliver commented May 20, 2020

blocked by

The ceph iscsi container needs to be able to insert the iscsi_target_mod
but it doesn't exist in the container. for security reasons bind
mounting /lib/modules seems a little dangerous unless we can mount it
RO.
Unfortuntly the docker volume mount (-v) doesn't allow you mount
readonly, adding a --read-only actaully does the opposite, makes the
root on the container RO and expects you to write to the mounted volumes
(-v).

However, we get more grainular control over bind mount options if we use
--mount[0]. Here we can still bind mound the volume into the container,
but can also add additional options, like bind mounting RO.

This patch adds at addiontal bind_mounts option to the CephContainer
along side volume_mounts. The bind_mounts take a List[List[str]]:

binds = []
lib_modules = ['type=bind',
'source=/lib/modules',
'destination=/lib/modules',
'ro=true']
binds.append(lib_modules)

And this is plumbed through into cephadm. Bind_mounts only needs to be
used if you need a little more control over the mounting, otherwise the
volume_mounts are easier to use.

[0] - https://docs.docker.com/engine/reference/commandline/service_create/#add-bind-mounts-volumes-or-memory-filesystems

Fixes: https://tracker.ceph.com/issues/45252
Signed-off-by: Matthew Oliver moliver@suse.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard backend
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@matthewoliver matthewoliver requested a review from a team as a code owner May 20, 2020 00:51
@matthewoliver
Copy link
Contributor Author

with this patch applied, when I deploy ceph-iscsi I can:

cephadm enter iscsi.iscsi.ironic-moliver.dgqkba
[ceph: root@ironic-moliver /]# mount |grep modules
/dev/sda1 on /usr/lib/modules type ext4 (ro,relatime,data=ordered)
[ceph: root@ironic-moliver /]# ls /lib/modules/4.12.14-lp15
4.12.14-lp150.12.82-default/ 4.12.14-lp151.28.36-default/

The ceph iscsi container needs to be able to insert the iscsi_target_mod
but it doesn't exist in the container. for security reasons bind
mounting /lib/modules seems a little dangerous unless we can mount it
RO.
Unfortuntly the docker volume mount (-v) doesn't allow you mount
readonly, adding a `--read-only` actaully does the opposite, makes the
root on the container RO and expects you to write to the mounted volumes
(-v).

However, we get more grainular control over bind mount options if we use
`--mount`[0]. Here we can still bind mound the volume into the container,
but can also add additional options, like bind mounting RO.

This patch adds at addiontal `bind_mounts` option to the CephContainer
along side `volume_mounts`. The `bind_mounts` take a List[List[str]]:

 binds = []
 lib_modules = ['type=bind',
                'source=/lib/modules',
                'destination=/lib/modules',
                'ro=true']
 binds.append(lib_modules)

And this is plumbed through into cephadm. Bind_mounts only needs to be
used if you need a little more control over the mounting, otherwise the
volume_mounts are easier to use.

[0] - https://docs.docker.com/engine/reference/commandline/service_create/#add-bind-mounts-volumes-or-memory-filesystems

Fixes: https://tracker.ceph.com/issues/45252
Signed-off-by: Matthew Oliver <moliver@suse.com>
@matthewoliver matthewoliver force-pushed the cephadm_iscsi_lib_module_mount branch from 5cccfaa to d9b5371 Compare May 20, 2020 01:06
Copy link
Contributor

@mgfritch mgfritch left a comment

Choose a reason for hiding this comment

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

[0] - https://docs.docker.com/engine/reference/commandline/service_create/#add-bind-mounts-volumes-or-memory-filesystems

any ideas on what the implications might be for SELinux?

Per the doc:

The --mount flag does not allow you to relabel a volume with Z or z flags

@bk201
Copy link
Contributor

bk201 commented May 20, 2020

Thanks. Tested and targets are created successfully.

@matthewoliver
Copy link
Contributor Author

[0] - https://docs.docker.com/engine/reference/commandline/service_create/#add-bind-mounts-volumes-or-memory-filesystems

any ideas on what the implications might be for SELinux?

Per the doc:

The --mount flag does not allow you to relabel a volume with Z or z flags

Good question. To be honest, I don't know. But we don't need write access only readonly, so hopefully selinux let's us do that?

I guess I'll have to test it with selinux and find out

@matthewoliver
Copy link
Contributor Author

Thanks. Tested and targets are created successfully.

Sweet, thanks Kiefer!

@sebastian-philipp
Copy link
Contributor

@sebastian-philipp sebastian-philipp requested a review from b-ranto May 27, 2020 11:39
@sebastian-philipp
Copy link
Contributor

@b-ranto as Teuthology doesn't test this yet, I'll need your input here. Do you think we'll run into SELinux issues?

@b-ranto
Copy link
Contributor

b-ranto commented Jun 3, 2020

@sebastian-philipp There is a lot of going on when you are doing bind mounts inside containers so it is really hard to tell if this will produce any SELinux denials. The best way to make sure we don't get any denials would be test it, even if we only test it manually and check the audit.log for any new denials. You might also want to tune the way you are bind-mounting the directory inside the container. There are SELinux modifiers that might cause denials while some other modifier might do what we need (z, Z or no modifier).

@sebastian-philipp
Copy link
Contributor

yeah, there is just one minor thing:

The --mount flag does not allow you to relabel a volume with Z or z flags

that's why I pinged you.

@b-ranto
Copy link
Contributor

b-ranto commented Jun 3, 2020

In that case, it would definitely be better to get this tested, SELinux can deny even read access (not just read, even open or any other syscall) unless we explicitly allow it in the policy.

@matthewoliver
Copy link
Contributor Author

OK, I can try and test it. I guess all I can do it attempt to test it on the "default" policy, because who knows what sets for selinux rules others are using. Although I guess if worse somes to worse we might be able to add something to the ceph/selinux/

@matthewoliver
Copy link
Contributor Author

I wonder if https://github.com/ceph/ceph/blob/master/selinux/ceph.te#L74-L76 already has things covered.

@b-ranto
Copy link
Contributor

b-ranto commented Jun 5, 2020

it should have it covered. However, there are few intricacies when doing SELinux inside containers so it is safer to actually test it out.

@sebastian-philipp
Copy link
Contributor

(still on my todo list)

@sebastian-philipp
Copy link
Contributor

blocked by #35543

@matthewoliver
Copy link
Contributor Author

I've built up an environment and installed the reference selinux policy. Currently have it in permissive mode and watching the audit log. So far I've only "added" the iscsi daemon, I see some docker warnings pre adding (but that's probably because docker isn't covered in the reference policy). Nothing major I see initially, will look a bit closer.

I'll try and use it, in case the module insert hasn't happened yet (which is where I expect the RO bindmount to fail in selinux). I thought it loaded at daemon start, but will confirm.

@matthewoliver
Copy link
Contributor Author

OK, connecting the dashboard to the ceph-iscsi server and then creating a target didn't throw anything to the audit log.

And I can see that the iscsi_target_mod kernel module has been loaded. So it seems loading from a RO bindmount seems fine, at least with just the selinux reference policy enabled.

I'll try again with the targetted policy, now that it seems I've found: https://build.opensuse.org/package/show/security:SELinux/selinux-policy

But suspect it'll be the same.

@matthewoliver
Copy link
Contributor Author

OK reran with the targeted policy. Run ceph orch apply -i iscsi.yaml. Deploys ok, at this point iscsi_target_mod isn't loaded. No issues in audit.log (some mentions of NETFILTER and iptables but audit2why doesn't care about them, maybe just information?).

Connect the dashboard to the running instance, go create a target. Works fine.
Nothing in the audit log and the iscsi_arget_mod has been loaded, which was done from the container. So no issues here.

Copy link
Contributor

@mgfritch mgfritch left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for the test!

@sebastian-philipp
Copy link
Contributor

jenkins test dashboard backend

@mgfritch mgfritch mentioned this pull request Jun 18, 2020
3 tasks
@mgfritch
Copy link
Contributor

jenkins test dashboard backend

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants