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

Check for common permissions issues with scratch area #765

Merged
merged 15 commits into from
Jan 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/how-to/edit-live.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ Blueapi can be configured to install editable Python packages from a chosen dire

scratch:
root: /path/to/my/scratch/directory
# Required GID for the scratch area
required_gid: 12345
repositories:
# Repository for DLS devices
- name: dodal
Expand All @@ -21,6 +23,9 @@ scratch:
remote_url: https://github.com/DiamondLightSource/mx-bluesky.git
```

Note the `required_gid` field, which is useful for stopping blueapi from locking the files it clones
to a particular owner.

## Synchronization

Blueapi will synchronize reality with the configuration if you run
Expand Down
5 changes: 5 additions & 0 deletions src/blueapi/cli/cli.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import json
import logging
import os
import stat
import sys
from functools import wraps
from pathlib import Path
Expand Down Expand Up @@ -39,6 +41,9 @@
def main(ctx: click.Context, config: Path | None | tuple[Path, ...]) -> None:
# if no command is supplied, run with the options passed

# Set umask to DLS standard
os.umask(stat.S_IWOTH)

config_loader = ConfigLoader(ApplicationConfig)
if config is not None:
configs = (config,) if isinstance(config, Path) else config
Expand Down
38 changes: 34 additions & 4 deletions src/blueapi/cli/scratch.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import logging
import os
import stat
import textwrap
from pathlib import Path
from subprocess import Popen

from git import Repo

from blueapi.config import ScratchConfig
from blueapi.utils import get_owner_gid, is_sgid_set

_DEFAULT_INSTALL_TIMEOUT: float = 300.0

Expand All @@ -23,7 +25,7 @@ def setup_scratch(
install_timeout: Timeout for installing packages
"""

_validate_directory(config.root)
_validate_root_directory(config.root, config.required_gid)

logging.info(f"Setting up scratch area: {config.root}")

Expand Down Expand Up @@ -74,9 +76,6 @@ def scratch_install(path: Path, timeout: float = _DEFAULT_INSTALL_TIMEOUT) -> No

_validate_directory(path)

# Set umask to DLS standard
os.umask(stat.S_IWOTH)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be removed from ensure_repo as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, there's another test which clones to a tempdir that only passes because of this line (because the test calls ensure_repo directly rather than invoking the CLI. Maybe the test should use mocks instead, although I do worry that sticking the umask set in the CLI isn't as robust as I thought. Might be better to pepper the code with some ensure_correct_umask function in strategic places...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...I now think we'll have to mock out all the directories in these tests anyway because of a separate issue that is breaking the CI with test_setup_scratch_succeeds_on_required_gid. I'll make an issue for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


logging.info(f"Installing {path}")
process = Popen(
[
Expand All @@ -94,6 +93,37 @@ def scratch_install(path: Path, timeout: float = _DEFAULT_INSTALL_TIMEOUT) -> No
raise RuntimeError(f"Failed to install {path}: Exit Code: {process.returncode}")


def _validate_root_directory(root_path: Path, required_gid: int | None) -> None:
_validate_directory(root_path)

if not is_sgid_set(root_path):
raise PermissionError(
textwrap.dedent(f"""
The scratch area root directory ({root_path}) needs to have the
SGID permission bit set. This allows blueapi to clone
repositories into it while retaining the ability for
other users in an approved group to edit/delete them.

See https://www.redhat.com/en/blog/suid-sgid-sticky-bit for how to
set the SGID bit.
""")
)
elif required_gid is not None and get_owner_gid(root_path) != required_gid:
raise PermissionError(
textwrap.dedent(f"""
The configuration requires that {root_path} be owned by the group with
ID {required_gid}.
You may be able to find this group's name by running the following
in the terminal.

getent group 1000 | cut -d: -f1

You can transfer ownership, if you have sufficient permissions, with the chgrp
command.
""")
)


def _validate_directory(path: Path) -> None:
if not path.exists():
raise KeyError(f"{path}: No such file or directory")
Expand Down
30 changes: 26 additions & 4 deletions src/blueapi/config.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import textwrap
from collections.abc import Mapping
from enum import Enum
from functools import cached_property
Expand Down Expand Up @@ -75,13 +76,34 @@ class RestConfig(BlueapiBaseModel):


class ScratchRepository(BlueapiBaseModel):
name: str = "example"
remote_url: str = "https://github.com/example/example.git"
name: str = Field(
description="Unique name for this repository in the scratch directory",
default="example",
)
remote_url: str = Field(
description="URL to clone from",
default="https://github.com/example/example.git",
)


class ScratchConfig(BlueapiBaseModel):
root: Path = Path("/tmp/scratch/blueapi")
repositories: list[ScratchRepository] = Field(default_factory=list)
root: Path = Field(
description="The root directory of the scratch area, all repositories will "
"be cloned under this directory.",
default=Path("/tmp/scratch/blueapi"),
)
required_gid: int | None = Field(
description=textwrap.dedent("""
Required owner GID for the scratch directory. If supplied the setup-scratch
command will check the scratch area ownership and raise an error if it is
not owned by <GID>.
"""),
default=None,
)
repositories: list[ScratchRepository] = Field(
description="Details of repositories to be cloned and imported into blueapi",
default_factory=list,
)


class OIDCConfig(BlueapiBaseModel):
Expand Down
3 changes: 3 additions & 0 deletions src/blueapi/utils/__init__.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from .base_model import BlueapiBaseModel, BlueapiModelConfig, BlueapiPlanModelConfig
from .connect_devices import connect_devices
from .file_permissions import get_owner_gid, is_sgid_set
from .invalid_config_error import InvalidConfigError
from .modules import load_module_all
from .serialization import serialize
Expand All @@ -14,4 +15,6 @@
"BlueapiPlanModelConfig",
"InvalidConfigError",
"connect_devices",
"is_sgid_set",
"get_owner_gid",
]
32 changes: 32 additions & 0 deletions src/blueapi/utils/file_permissions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import stat
from pathlib import Path


def is_sgid_set(path: Path) -> bool:
"""Check if the SGID bit is set so that new files created
under a directory owned by a group are owned by that same group.

See https://www.redhat.com/en/blog/suid-sgid-sticky-bit

Args:
path: Path to the file to check

Returns:
bool: True if the SGID bit is set
"""

mask = path.stat().st_mode
return bool(mask & stat.S_ISGID)


def get_owner_gid(path: Path) -> int:
"""Get the GID of the owner of a file

Args:
path: Path to the file to check

Returns:
bool: The GID of the file owner
"""

return path.stat().st_gid
Loading
Loading