From 98c455f7c9cdeef79ea7b3a1bbeae4c733c04b73 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Wed, 16 Oct 2024 18:00:46 +0200 Subject: [PATCH] cockpit: support setting owner/group in fsreplace1 Cockpit-files wants to support uploading or creating a file owned by the current directory which might be different from the logged in user. For example as superuser uploading a database into `/var/lib/postgresql` which would be owned by `postgres` and the database file should receive the same permissions. --- doc/protocol.md | 5 ++ pkg/lib/cockpit.d.ts | 7 +- pkg/lib/cockpit.js | 8 ++- pkg/playground/test.html | 13 ++++ pkg/playground/test.js | 24 +++++++ src/cockpit/channels/filesystem.py | 63 +++++++++++++++++- src/cockpit/jsonutil.py | 9 +++ test/pytest/test_bridge.py | 101 ++++++++++++++++++++++++++++- test/verify/check-pages | 38 +++++++++++ 9 files changed, 261 insertions(+), 7 deletions(-) diff --git a/doc/protocol.md b/doc/protocol.md index 484f207f42cf..1750b4d9b69e 100644 --- a/doc/protocol.md +++ b/doc/protocol.md @@ -1072,6 +1072,11 @@ The following options can be specified in the "open" control message: you don't set this field, the actual tag will not be checked. To express that you expect the file to not exist, use "-" as the tag. +* "attrs": a JSON object containing file owner and group information + to set. Both `owner` and `group` are required. + - `owner`: a string, or an integer, the uid of the file owner (`st_uid`) + - `group`: a string, or an integer, the gid of the file group (`st_gid`) + You should write the new content to the channel as one or more messages. To indicate the end of the content, send a "done" message. diff --git a/pkg/lib/cockpit.d.ts b/pkg/lib/cockpit.d.ts index f371d563cee6..81629e2f395e 100644 --- a/pkg/lib/cockpit.d.ts +++ b/pkg/lib/cockpit.d.ts @@ -257,9 +257,14 @@ declare module 'cockpit' { remove(): void; } + interface ReplaceAttrs { + owner: string | number; + group: string | number; + } + interface FileHandle { read(): Promise; - replace(new_content: T | null, expected_tag?: FileTag): Promise; + replace(new_content: T | null, expected_tag?: FileTag, attrs?: ReplaceAttrs): Promise; watch(callback: FileWatchCallback, options?: { read?: boolean }): FileWatchHandle; modify(callback: (data: T | null) => T | null, initial_content?: string, initial_tag?: FileTag): Promise<[T, FileTag]>; close(): void; diff --git a/pkg/lib/cockpit.js b/pkg/lib/cockpit.js index 94bd7e11886d..a4b2f0e2846f 100644 --- a/pkg/lib/cockpit.js +++ b/pkg/lib/cockpit.js @@ -2241,7 +2241,7 @@ function factory() { let replace_channel = null; - function replace(new_content, expected_tag) { + function replace(new_content, expected_tag, attrs) { const dfd = cockpit.defer(); let file_content; @@ -2259,8 +2259,12 @@ function factory() { ...base_channel_options, payload: "fsreplace1", path, - tag: expected_tag + tag: expected_tag, }; + + if (attrs) + opts.attrs = attrs; + replace_channel = cockpit.channel(opts); replace_channel.addEventListener("close", function (event, message) { diff --git a/pkg/playground/test.html b/pkg/playground/test.html index e74f89c655a8..acdbe8c889ed 100644 --- a/pkg/playground/test.html +++ b/pkg/playground/test.html @@ -45,6 +45,19 @@

cockpit.user() information

+
+
+

fsreplace1 test

+

new filename

+ +

text content

+ +

owner mode

+ + + +
+
diff --git a/pkg/playground/test.js b/pkg/playground/test.js index 7c7c67758f28..a84a9d1b3bf4 100644 --- a/pkg/playground/test.js +++ b/pkg/playground/test.js @@ -130,6 +130,30 @@ document.addEventListener("DOMContentLoaded", () => { document.getElementById("user-info").textContent = JSON.stringify(info); }); + const fsreplace_btn = document.getElementById("fsreplace1-create"); + const fsreplace_error = document.getElementById("fsreplace1-error"); + fsreplace_btn.addEventListener("click", e => { + fsreplace_btn.disabled = true; + fsreplace_error.textContent = ''; + const filename = document.getElementById("fsreplace1-filename").value; + const content = document.getElementById("fsreplace1-content").value; + const attrs = { }; + for (const field of ["owner", "group"]) { + const val = document.getElementById(`fsreplace1-${field}`).value; + if (!val) + continue; + + attrs[field] = val; + } + cockpit.file(filename, { superuser: "try" }).replace(content, undefined, attrs) + .catch(exc => { + fsreplace_error.textContent = exc.toString(); + }) + .finally(() => { + fsreplace_btn.disabled = false; + }); + }); + cockpit.addEventListener("visibilitychange", show_hidden); show_hidden(); }); diff --git a/src/cockpit/channels/filesystem.py b/src/cockpit/channels/filesystem.py index 7d7afdbf6c59..3d5a481e8883 100644 --- a/src/cockpit/channels/filesystem.py +++ b/src/cockpit/channels/filesystem.py @@ -43,7 +43,9 @@ JsonObject, get_bool, get_int, + get_object, get_str, + get_str_or_int, get_strv, json_merge_and_filter_patch, ) @@ -158,6 +160,50 @@ def do_yield_data(self, options: JsonObject) -> Generator[bytes, None, JsonObjec raise ChannelError('internal-error', message=str(exc)) from exc +class FSReplaceAttrs: + uid: 'int | None' = None + gid: 'int | None' = None + supported_attrs = ['owner', 'group'] + + def __init__(self, value: JsonObject) -> None: + # Any unknown keys throw an error + unsupported_attrs = list(value.keys() - self.supported_attrs) + if unsupported_attrs: + raise ChannelError('protocol-error', + message=f'"attrs" contains unsupported key(s) {unsupported_attrs}') from None + + try: + owner = get_str_or_int(value, 'owner', None) + except JsonError: + raise ChannelError('protocol-error', message='"owner" must be an integer or string') from None + + try: + group = get_str_or_int(value, 'group', None) + except JsonError: + raise ChannelError('protocol-error', message='"group" must be an integer or string') from None + + if owner is not None and group is None: + raise ChannelError('protocol-error', message='"group" attribute is empty while "owner" is provided') + if group is not None and owner is None: + raise ChannelError('protocol-error', message='"owner" attribute is empty while "group" is provided') + + if isinstance(owner, str): + try: + self.uid = pwd.getpwnam(owner).pw_uid + except KeyError: + raise ChannelError('internal-error', message=f'uid not found for {owner}') from None + else: + self.uid = owner + + if isinstance(group, str): + try: + self.gid = grp.getgrnam(group).gr_gid + except KeyError: + raise ChannelError('internal-error', message=f'gid not found for {group}') from None + else: + self.gid = group + + class FsReplaceChannel(AsyncChannel): payload = 'fsreplace1' @@ -168,10 +214,18 @@ def delete(self, path: str, tag: 'str | None') -> str: os.unlink(path) return '-' - async def set_contents(self, path: str, tag: 'str | None', data: 'bytes | None', size: 'int | None') -> str: + async def set_contents(self, path: str, tag: 'str | None', data: 'bytes | None', size: 'int | None', + attrs: 'FSReplaceAttrs | None') -> str: dirname, basename = os.path.split(path) tmpname: str | None fd, tmpname = tempfile.mkstemp(dir=dirname, prefix=f'.{basename}-') + + def chown_if_required(fd: 'int'): + if attrs is None or attrs.uid is None or attrs.gid is None: + return + + os.fchown(fd, attrs.uid, attrs.gid) + try: if size is not None: logger.debug('fallocate(%s.tmp, %d)', path, size) @@ -195,12 +249,14 @@ async def set_contents(self, path: str, tag: 'str | None', data: 'bytes | None', # no preconditions about what currently exists or not # calculate the file mode from the umask os.fchmod(fd, 0o666 & ~my_umask()) + chown_if_required(fd) os.rename(tmpname, path) tmpname = None elif tag == '-': # the file must not exist. file mode from umask. os.fchmod(fd, 0o666 & ~my_umask()) + chown_if_required(fd) os.link(tmpname, path) # will fail if file exists else: @@ -225,6 +281,7 @@ async def run(self, options: JsonObject) -> JsonObject: path = get_str(options, 'path') size = get_int(options, 'size', None) tag = get_str(options, 'tag', None) + attrs = get_object(options, 'attrs', FSReplaceAttrs, None) try: # In the `size` case, .set_contents() sends the ready only after @@ -232,7 +289,7 @@ async def run(self, options: JsonObject) -> JsonObject: # `size`, we need to send the ready() up front in order to # receive the first frame and decide if we're creating or deleting. if size is not None: - tag = await self.set_contents(path, tag, b'', size) + tag = await self.set_contents(path, tag, b'', size, attrs) else: self.ready() data = await self.read() @@ -240,7 +297,7 @@ async def run(self, options: JsonObject) -> JsonObject: if data is None: tag = self.delete(path, tag) else: - tag = await self.set_contents(path, tag, data, None) + tag = await self.set_contents(path, tag, data, None, attrs) self.done() return {'tag': tag} diff --git a/src/cockpit/jsonutil.py b/src/cockpit/jsonutil.py index 7df905c4e6c2..80807eb2e82c 100644 --- a/src/cockpit/jsonutil.py +++ b/src/cockpit/jsonutil.py @@ -87,6 +87,15 @@ def get_str_or_none(obj: JsonObject, key: str, default: Optional[str]) -> Option return _get(obj, lambda v: None if v is None else typechecked(v, str), key, default) +def get_str_or_int(obj: JsonObject, key: str, default: Optional[Union[str, int]]) -> Optional[Union[str, int]]: + def as_str_or_int(value: JsonValue) -> Union[str, int]: + if not isinstance(value, (str, int)): + raise JsonError(value, 'must be a string or integer') + return value + + return _get(obj, as_str_or_int, key, default) + + def get_dict(obj: JsonObject, key: str, default: Union[DT, _Empty] = _empty) -> Union[DT, JsonObject]: return _get(obj, lambda v: typechecked(v, dict), key, default) diff --git a/test/pytest/test_bridge.py b/test/pytest/test_bridge.py index 91d8f2b66664..69cc9163ffbf 100644 --- a/test/pytest/test_bridge.py +++ b/test/pytest/test_bridge.py @@ -14,7 +14,7 @@ import unittest.mock from collections import deque from pathlib import Path -from typing import Dict, Iterable, Iterator, Sequence +from typing import Dict, Generator, Iterable, Iterator, Optional, Sequence, Union import pytest @@ -482,6 +482,28 @@ async def test_fslist1_notexist(transport: MockTransport) -> None: reply_keys={'message': "[Errno 2] No such file or directory: '/nonexisting'"}) +class GrpPwMock: + def __init__(self, uid: int, gid: Optional[int] = None) -> None: + self.pw_uid = uid + self.gr_gid = gid + + def __str__(self) -> str: + return f'uid={self.pw_uid},gid={self.gr_gid}' + + +@pytest.fixture +def fchown_mock(): + with unittest.mock.patch('os.fchown', return_value=True) as fchown_mock: + yield fchown_mock + + +@pytest.fixture +def owner_group_mock(owner_group_mock_arg: GrpPwMock) -> Generator[GrpPwMock, GrpPwMock, None]: + with unittest.mock.patch('pwd.getpwnam', return_value=owner_group_mock_arg): + with unittest.mock.patch('grp.getgrnam', return_value=owner_group_mock_arg): + yield owner_group_mock_arg + + @pytest.mark.asyncio async def test_fsreplace1(transport: MockTransport, tmp_path: Path) -> None: # create non-existing file @@ -655,6 +677,83 @@ async def test_fsreplace1_error(transport: MockTransport, tmp_path: Path) -> Non 'message': """attribute 'send-acks': invalid value "not-valid" not in ['bytes']""" }) + await transport.check_open('fsreplace1', path=str(tmp_path / 'test'), + attrs={'owner': 'cockpit', 'group': 'cockpit', 'selinux': True}, + problem='protocol-error', + reply_keys={ + 'message': '"attrs" contains unsupported key(s) [\'selinux\']' + }) + + await transport.check_open('fsreplace1', path=str(tmp_path / 'test'), attrs={'owner': 'cockpit'}, + problem='protocol-error', + reply_keys={ + 'message': '"group" attribute is empty while "owner" is provided' + }) + + await transport.check_open('fsreplace1', path=str(tmp_path / 'test'), attrs={'group': 'cockpit'}, + problem='protocol-error', + reply_keys={ + 'message': '"owner" attribute is empty while "group" is provided' + }) + + await transport.check_open('fsreplace1', path=str(tmp_path / 'test'), attrs={'owner': 'cockpit', 'group': []}, + problem='protocol-error', + reply_keys={ + 'message': '"group" must be an integer or string' + }) + + await transport.check_open('fsreplace1', path=str(tmp_path / 'test'), attrs={'owner': [], 'group': 'test'}, + problem='protocol-error', + reply_keys={ + 'message': '"owner" must be an integer or string' + }) + + mock_val = GrpPwMock(uid=0, gid=1000) + with unittest.mock.patch('pwd.getpwnam', side_effect=KeyError()): + await transport.check_open('fsreplace1', path=str(tmp_path / 'test'), + attrs={'owner': 'bazinga', 'group': 'foo'}, + problem='internal-error', + reply_keys={ + 'message': 'uid not found for bazinga' + }) + + with unittest.mock.patch('pwd.getpwnam', return_value=mock_val): + with unittest.mock.patch('grp.getgrnam', side_effect=KeyError()): + await transport.check_open('fsreplace1', path=str(tmp_path / 'test'), + attrs={'owner': 'bazinga', 'group': 'test'}, + problem='internal-error', + reply_keys={ + 'message': 'gid not found for test' + }) + + +ATTRS_TEST_DATA = [ + (GrpPwMock(1111, 1110), {'owner': 1111, 'group': 1110}), + (GrpPwMock(1111, 1110), {'owner': 'monkey', 'group': 'group'}), +] + + +@pytest.mark.parametrize(('owner_group_mock_arg', 'attrs'), ATTRS_TEST_DATA) +@pytest.mark.asyncio +async def test_fsreplace1_attrs(transport: MockTransport, fchown_mock: unittest.mock.MagicMock, + tmp_path: Path, owner_group_mock, attrs) -> None: + async def create_file_with_attrs(filename: str, attrs: Dict[str, Union[int, str]]) -> None: + test_file = str(tmp_path / filename) + ch = await transport.check_open('fsreplace1', path=str(test_file), attrs=attrs) + transport.send_data(ch, b'content') + transport.send_done(ch) + await transport.assert_msg('', command='done', channel=ch) + await transport.check_close(ch) + + await create_file_with_attrs('test', attrs) + fchown_mock.assert_called() + _, call_uid, call_gid = fchown_mock.call_args[0] + assert call_uid == owner_group_mock.pw_uid + if owner_group_mock.gr_gid is None: + assert call_gid == owner_group_mock.pw_uid + else: + assert call_gid == owner_group_mock.gr_gid + @pytest.mark.asyncio async def test_fsreplace1_size_hint(transport: MockTransport, tmp_path: Path) -> None: diff --git a/test/verify/check-pages b/test/verify/check-pages index 9f57345fa63a..9918ae29df7a 100755 --- a/test/verify/check-pages +++ b/test/verify/check-pages @@ -1002,6 +1002,44 @@ OnCalendar=daily self.assertEqual(str(user_info["id"]), m.execute("id -u admin").strip()) self.assertEqual(str(user_info["gid"]), m.execute("id -g admin").strip()) + def testFsreplaceOwnership(self) -> None: + b = self.browser + + self.login_and_go("/playground/test") + + def stat(fmt: str, path: str) -> str: + return self.machine.execute(['stat', f'--format={fmt}', path]).strip() + + def assert_stat(fmt: str, path: str, expected: str) -> None: + self.assertEqual(stat(fmt, path), expected) + + def assert_owner(path: str, owner: str) -> None: + assert_stat('%U:%G', path, owner) + + def assert_content(path: str, expected: str) -> None: + self.assertEqual(self.machine.execute(f"cat {path}").strip(), expected) + + def set_file_bits(filename: str, content: str, owner: str = "", group: str = "") -> None: + b.set_input_text("#fsreplace1-filename", filename) + b.set_input_text("#fsreplace1-content", content) + b.set_input_text("#fsreplace1-owner", owner) + b.set_input_text("#fsreplace1-group", group) + + b.click("#fsreplace1-create") + b.wait_visible("#fsreplace1-create:not(:disabled)") + + filename = "/home/admin/admin.txt" + content = "adminfile" + set_file_bits(filename, content, "admin", "root") + assert_owner(filename, "admin:root") + assert_content(filename, content) + + filename = "/home/admin/root.txt" + content = "rootfile" + set_file_bits(filename, content, "root", "root") + assert_owner(filename, "root:root") + assert_content(filename, content) + if __name__ == '__main__': testlib.test_main()