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.

Sign up for GitHub

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

Jump to bottom

[collect] update for strict confinement for juju #3422

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[collect] update for strict confinement for juju #3422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 20 additions & 3 deletions sos/collector/transports/juju.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

from sos.collector.exceptions import JujuNotInstalledException
from sos.collector.transports import RemoteTransport
from sos.utilities import sos_get_command_output
from sos.utilities import sos_get_command_output, parse_version


class JujuSSH(RemoteTransport):
Expand Down Expand Up @@ -72,12 +72,29 @@ def remote_exec(self):
option = f"{model_option} {target_option}"
return f"juju ssh {option}"

def _get_juju_version(self):
"""Grab the version of juju"""
res = sos_get_command_output("juju version")
return res['output'].split("-")[0]

def _retrieve_file(self, fname, dest):
self._chmod(fname) # juju scp needs the archive to be world-readable
model, unit = self.address.split(":")
model_option = f"-m {model}" if model else ""
cmd = f"juju scp {model_option} -- -r {unit}:{fname} {dest}"
res = sos_get_command_output(cmd)
if parse_version(self._get_juju_version()) > parse_version("3"):
# juju version above 3 is strictly confined, and therefore
# the way we grab the data is slightly different
juju_tmpdir = f"/tmp/snap-private-tmp/snap.juju/{self.tmpdir}"
sos_get_command_output(f"sudo mkdir {juju_tmpdir}")
sos_get_command_output(f"sudo chmod o+rwx {juju_tmpdir}")
cmd = f"juju scp {model_option} -- -r {unit}:{fname} {self.tmpdir}"
res = sos_get_command_output(cmd)
cmd2 = f"sudo cp {juju_tmpdir}/{fname.split('/')[-1]} {dest}"
sos_get_command_output(cmd2)
sos_get_command_output(f"sudo chmod 644 {dest}")
Comment on lines +88 to +94
Copy link
Member

Choose a reason for hiding this comment

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

I am really not a fan of embedding sudo into a flow here, or shelling out to mess with the permissions and directory structure.

Let's take a step back and review. What does "strict confinement" mean for juju? As a regular user running, what causes the juju commands I run to pull data to write as root? If I, as a regular user, can leverage juju commands to execute within a machine, why do I need to do special steps to access certain data within that machine? I.E. why does "juju-root" matter when pulling the data but not when executing the commands on behalf of the local user?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can totally understand, and this is nothing to do with juju itself, but how juju is installed. juju is installed as a snap, and snaps are either classic confinement or strict confinement.

juju 2.9 and below were classic confinement, and hence it had the capability of writing to the classic /tmp. As soon as an application is strictly confined (and is the case for juju >=3), and you specify /tmp, this will automatically expand to /tmp/snap-private-tmp/snap.<snap-name>/tmp. So, when we do a juju scp and bring it onto the collector machine, it will copy it to /tmp/snap-private-tmp/snap.<snap-name>/tmp/sos.XXXXX.

If, however, we were using $HOME and some directory there to copy the file to, we should be able to, due to the following connections we have. The /tmp is a special area, that security confinement does not allow it to access files from other snaps and applications

❱❱❱ snap connections juju | grep home
home             juju:home                  :home             -

Below is a link to a similar question on this

https://askubuntu.com/questions/1227248/how-to-make-snaps-see-the-real-tmp

I hope that makes sense

Copy link
Member Author

Choose a reason for hiding this comment

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

@TurboTurtle thoughts on this, I am keen for this to land for 4.7.0 if possible. Unless, you can advise on a different way to handle this?

@dnegreira anything you would like to chime in on?

Copy link
Member

Choose a reason for hiding this comment

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

@arif-ali I'm still not keen on embedding sudo and the like.

If we're writing to a private /tmp location due to it being packaged as a snap, would it suffice to have the snap packaging set sos' entire tmpdir to that private location? As in, in sos.conf overriding --tmp-dir to /tmp/snap-private-tmp/snap.<snap-name>/ with the snap packaging, leaving deb packaging as the "normal" /tmp?

Alternatively, is juju non-root capable? If not, would it be acceptable to say sos collect now requires root for juju collections?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not a snap issue of the sos and hence changing the default for the snap does not make sense here. This issue would exist on any machine that even has the deb or rpm version of sos, and is trying to access a juju cluster using v3

This is ultimately an issue with how juju is packaged and not sos. As mentioned above, there are 2 ways to package a snap, strict and classic. strict is the recommended way, and hence juju is like this from 3 onwards. The reason why sos is not is due to the nature that it is a debugging tool, and it doesn't make sense for it to be strictly confined.

The folder /tmp/snap-private-tmp is only written by strictly confined snaps, in this case this is the juju snap that is doing this and not sos. When we scp files from the remote host using juju scp< machine-id>:<source> /tmp/sos.xxxx/. That command is actually copying the file into /tmp/snap-private-tmp/snap.juju/tmp/sos.xxxx and not into /tmp/sos.xxxx. This is the essence of strictly confined snaps. This folder is only accessible to root users unfortunately, and hence the need to use sudo.

I appreciate this is not ideal and goes against in what this is happening here, but unfortunately this would be the only way we can easily resolve this.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only way I see, instead of running sudo, which I am also not very keen on doing, is forcing the collect to run as root, as we already do for report ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dnegreira that will defeat the purpose, as juju may not be configured for the root user, and hence will not be able to actually do sudo juju ssh or sudo juju scp right?

Copy link
Contributor

@dnegreira dnegreira Feb 3, 2024 •
edited

Choose a reason for hiding this comment

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

By default no, but one could point the env variable JUJU_DATA or directly point to the ssh key under ~/.local/share/juju/ and it -should- work.

Copy link
Member

Choose a reason for hiding this comment

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

The only way I see, instead of running sudo, which I am also not very keen on doing, is forcing the collect to run as root, as we already do for report ?

To be clear, we'd make this root required for the juju cluster profile/transport specifically within collect, not for the entirety of collect. Unfortunately I think this is the only path forward, as I simply cannot get myself over the embedding of sudo here.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear, we'd make this root required for the juju cluster profile/transport specifically within collect, not for the entirety of collect. Unfortunately I think this is the only path forward, as I simply cannot get myself over the embedding of sudo here.

Cool, I'll have to re-think how we do this then, leave it with me; thanks for the inputs

else:
cmd = f"juju scp {model_option} -- -r {unit}:{fname} {dest}"
res = sos_get_command_output(cmd)
return res["status"] == 0


Expand Down
9 changes: 8 additions & 1 deletion tests/unittests/juju/juju_transports_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ def setUp(self):
address="model_abc:unit_abc",
)

def get_juju_version():
return "2.9.45"

@patch("sos.collector.transports.juju.subprocess.check_output")
def test_check_juju_installed_err(self, mock_subprocess_check_output):
"""Raise error if juju is not installed."""
Expand Down Expand Up @@ -70,12 +73,16 @@ def test_remote_exec(self):
self.juju_ssh.remote_exec == "juju ssh -m model_abc unit_abc"
)

@patch(
"sos.collector.transports.juju.JujuSSH._get_juju_version",
side_effect=get_juju_version,
)
@patch(
"sos.collector.transports.juju.sos_get_command_output",
return_value={"status": 0},
)
@patch("sos.collector.transports.juju.JujuSSH._chmod", return_value=True)
def test_retrieve_file(self, mock_chmod, mock_sos_get_cmd_output):
def test_retrieve_file(self, mock_chmod, mock_sos_get_cmd_output, mock_get_juju_version):
self.juju_ssh._retrieve_file(fname="file_abc", dest="/tmp/sos-juju/")
mock_sos_get_cmd_output.assert_called_with(
"juju scp -m model_abc -- -r unit_abc:file_abc /tmp/sos-juju/"
Expand Down