From 8554761c4e5434cf6f862f200188e24c1c1a02d4 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 | 7 ++ pkg/lib/cockpit.d.ts | 9 ++- pkg/lib/cockpit.js | 8 +- pkg/playground/test.html | 15 ++++ pkg/playground/test.js | 28 +++++++ src/cockpit/channels/filesystem.py | 80 ++++++++++++++++++- src/cockpit/jsonutil.py | 9 +++ test/pytest/test_bridge.py | 120 ++++++++++++++++++++++++++++- test/verify/check-pages | 69 +++++++++++++++++ 9 files changed, 338 insertions(+), 7 deletions(-) diff --git a/doc/protocol.md b/doc/protocol.md index 484f207f42cf..9c0e46d898e5 100644 --- a/doc/protocol.md +++ b/doc/protocol.md @@ -1072,6 +1072,13 @@ 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. Supporting the file ownership fields returned by `fsinfo`. + * `uid`: an integer, the uid of the file owner (`st_uid`) + * `owner`: a string, or an integer, the uid of the file owner (`st_uid`) + * `gid`: an integer, the gid of the file group (`st_gid`) + * `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..396bb72cd486 100644 --- a/pkg/lib/cockpit.d.ts +++ b/pkg/lib/cockpit.d.ts @@ -257,9 +257,16 @@ declare module 'cockpit' { remove(): void; } + interface ReplaceAttrs { + uid?: number; + user?: string | number; + gid?: 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 8d2d9e04ad8f..2d3b4be0523c 100644 --- a/pkg/lib/cockpit.js +++ b/pkg/lib/cockpit.js @@ -2231,7 +2231,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; @@ -2249,8 +2249,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..184bbd2bbfd3 100644 --- a/pkg/playground/test.html +++ b/pkg/playground/test.html @@ -45,6 +45,21 @@

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..507b9b088273 100644 --- a/pkg/playground/test.js +++ b/pkg/playground/test.js @@ -130,6 +130,34 @@ 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", "uid", "gid"]) { + let val = document.getElementById(`fsreplace1-${field}`).value; + if (!val) + continue; + + if (field.endsWith('id')) + val = Number.parseInt(val, 10); + + attrs[field] = val; + } + cockpit.file(filename, { superuser: "try" }).replace(content, undefined, attrs).then(() => { + fsreplace_btn.disabled = false; + }) + .catch(exc => { + console.log(exc); + fsreplace_error.textContent = exc; + 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..c722a2b7a34b 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,64 @@ def do_yield_data(self, options: JsonObject) -> Generator[bytes, None, JsonObjec raise ChannelError('internal-error', message=str(exc)) from exc +class FSReplaceAttrs: + supported_attrs = ['uid', 'gid', '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 + + self.uid = get_int(value, 'uid', None) + self.gid = get_int(value, 'gid', None) + + if self.uid is None and self.gid is not None: + raise ChannelError('protocol-error', message='cannot provide a gid without an uid') + + 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 None and group is not None: + raise ChannelError('protocol-error', message='cannot provide a group without an owner') + + if owner is not None: + if isinstance(owner, str): + try: + uid = pwd.getpwnam(owner).pw_uid + except KeyError: + raise ChannelError('internal-error', message=f'uid not found for {owner}') from None + + if self.uid is not None and self.uid != uid: + raise ChannelError('protocol-error', message='"owner" uid does not match passed "uid"') from None + + self.uid = uid + else: + self.uid = owner + + if group is not None: + if isinstance(group, str): + try: + gid = grp.getgrnam(group).gr_gid + except KeyError: + raise ChannelError('internal-error', message=f'gid not found for {group}') from None + + if self.gid is not None and self.gid != gid: + raise ChannelError('protocol-error', message='"group" gid does not match passed "gid"') from None + + self.gid = gid + else: + self.gid = group + + class FsReplaceChannel(AsyncChannel): payload = 'fsreplace1' @@ -168,10 +228,21 @@ 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: + return + + if attrs.uid is not None and attrs.gid is not None: + os.fchown(fd, attrs.uid, attrs.gid) + elif attrs.uid is not None: + os.fchown(fd, attrs.uid, attrs.uid) + try: if size is not None: logger.debug('fallocate(%s.tmp, %d)', path, size) @@ -195,12 +266,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 +298,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 +306,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 +314,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 678998619703..b38825d4782b 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 @@ -480,6 +480,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 @@ -638,6 +660,102 @@ 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={'gid': 1, '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={'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': []}, + problem='protocol-error', + reply_keys={ + 'message': '"owner" must be an integer or string' + }) + + await transport.check_open('fsreplace1', path=str(tmp_path / 'test'), attrs={'group': 1}, + problem='protocol-error', + reply_keys={ + 'message': 'cannot provide a group without an owner' + }) + + await transport.check_open('fsreplace1', path=str(tmp_path / 'test'), attrs={'gid': 1}, + problem='protocol-error', + reply_keys={ + 'message': 'cannot provide a gid without an uid' + }) + + with unittest.mock.patch('grp.getgrnam', return_value=GrpPwMock(uid=0, gid=1000)): + await transport.check_open('fsreplace1', path=str(tmp_path / 'test'), attrs={'uid': 9999, 'owner': 'root'}, + problem='protocol-error', + reply_keys={ + 'message': '"owner" uid does not match passed "uid"' + }) + + mock_val = GrpPwMock(uid=0, gid=1000) + with unittest.mock.patch('grp.getgrnam', return_value=mock_val): + with unittest.mock.patch('pwd.getpwnam', return_value=mock_val): + await transport.check_open('fsreplace1', path=str(tmp_path / 'test'), + attrs={'gid': 9999, 'uid': 0, 'owner': 'root', 'group': 'root'}, + problem='protocol-error', + reply_keys={ + 'message': '"group" gid does not match passed "gid"' + }) + + with unittest.mock.patch('pwd.getpwnam', side_effect=KeyError()): + await transport.check_open('fsreplace1', path=str(tmp_path / 'test'), attrs={'owner': 'bazinga'}, + 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), {'owner': 'monkey'}), + (GrpPwMock(1111), {'owner': 1111}), + (GrpPwMock(1111, 1110), {'owner': 1111, 'group': 1110}), + (GrpPwMock(1111, 1110), {'owner': 'monkey', 'group': 'group'}), + (GrpPwMock(1, 1005), {'uid': 1, 'gid': 1005}), + (GrpPwMock(1), {'uid': 1}), + (GrpPwMock(1111, 1110), {'owner': 'monkey', 'group': 'group', 'uid': 1111, 'gid': 1110}), +] + + +@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..88f94387eeb3 100755 --- a/test/verify/check-pages +++ b/test/verify/check-pages @@ -1002,6 +1002,75 @@ 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 = "", + uid: str = "", gid: 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.set_input_text("#fsreplace1-uid", uid) + b.set_input_text("#fsreplace1-gid", gid) + + 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) + + # only passing an owner becomes admin:admin + filename = "/home/admin/admin-special.txt" + content = "admin-special" + set_file_bits(filename, content, "admin") + assert_owner(filename, "admin:admin") + assert_content(filename, content) + + # uid/gid + admin_uid = self.machine.execute("id -u admin").strip() + filename = "/home/admin/uid.txt" + content = "uid" + set_file_bits(filename, content, uid=str(admin_uid), gid=str(admin_uid)) + assert_owner(filename, "admin:admin") + assert_content(filename, content) + + filename = "/home/admin/uid-only.txt" + content = "uid-only" + set_file_bits(filename, content, uid=admin_uid) + assert_owner(filename, "admin:admin") + assert_content(filename, content) + + cockpit_ws_gid = self.machine.execute("getent group cockpit-ws | cut -d: -f3").strip() + filename = "/home/admin/gid.txt" + content = "gid" + set_file_bits(filename, content, uid=admin_uid, gid=cockpit_ws_gid) + assert_owner(filename, "admin:cockpit-ws") + assert_content(filename, content) + if __name__ == '__main__': testlib.test_main()