Maintenance socket: set filesystem permissions to 660 #17113

margdoc · 2024-02-01T14:54:22Z

Set filesystem permissions for the maintenance socket to 660 (previously it was 755) to allow a scyllaadm's group to connect.
Split the logic of creating sockets into two separate functions, one for each case: when it is a regular cql controller or used by maintenance_socket.

Fixes #16487.

avikivity · 2024-02-01T15:12:11Z

What were the permissions before the patch? I'm surprised if "others" had write permission, which is needed to connect.

margdoc · 2024-02-02T11:22:43Z

What were the permissions before the patch? I'm surprised if "others" had write permission, which is needed to connect.

It was 755, so only the owner could connect to the socket.

kbr-scylla · 2024-02-02T12:58:30Z

@ptrsmrn please assign someone from your team to review this (I think this can be considered a "frontend" feature)

dawmd

LGTM

ptrsmrn · 2024-02-02T17:26:16Z

transport/controller.cc

@@ -65,6 +66,124 @@ future<> controller::start_server() {
    return do_start_server().finally([this] { _ops_sem.signal(); });
 }

+// Wrapper for cql_server::listen to invoke it on all shards and log address and listen options.
+static future<> listen(sharded<cql_server>& cserver, socket_address addr, std::shared_ptr<seastar::tls::credentials_builder> creds, bool is_shard_aware, bool keepalive, std::optional<file_permissions> unix_domain_socket_permissions) {


Maybe better to assign unix_domain_socket_permissions a default value instead of making it optional, that way you won't have to pass an extra std::nullopt if you don't intend to specify permissions.

not sure if this makes sense because afacs seastar part of this patch is either doing some calls or not (use defaults) so later it would require extra check if unix_domain_socket_permissions != default

Documennthing this in some way in the comment in generic_server.hh could be nice though

There's a difference between setting permissions to a default value, and not changing permissions.

transport/controller.cc

ptrsmrn · 2024-02-02T17:34:13Z

transport/controller.cc

+    logger.info("Starting listening for CQL clients on {} ({}, {})"
+            , addr, creds ? "encrypted" : "unencrypted", is_shard_aware ? "shard-aware" : "non-shard-aware"


Consider changing to something shorter like:

Suggested change

logger.info("Starting listening for CQL clients on {} ({}, {})"

, addr, creds ? "encrypted" : "unencrypted", is_shard_aware ? "shard-aware" : "non-shard-aware"

logger.info("Starting listening for CQL clients on {} (encrypted: {}, shard-aware: {})", addr, creds, is_shard_aware)

(or better print "true" or "false")

it's code already existing on master, I'd not change it in this PR

ptrsmrn · 2024-02-02T17:35:22Z

transport/controller.cc

+    auto& cfg = _config;
+    auto preferred = cfg.rpc_interface_prefer_ipv6() ? std::make_optional(net::inet_address::family::INET6) : std::nullopt;
+    auto family = cfg.enable_ipv6_dns_lookup() || preferred ? std::nullopt : std::make_optional(net::inet_address::family::INET);
+    auto ceo = cfg.client_encryption_options();


The last I checked @dorlaor was still our CEO :D

I didn't know you can reassign this position so easily!

transport/controller.cc

avikivity · 2024-02-05T11:05:50Z

What were the permissions before the patch? I'm surprised if "others" had write permission, which is needed to connect.

It was 755, so only the owner could connect to the socket.

Aha. But is changing permissions enough? We need to chgrp it too.

TCP sockets and unix domain sockets don't share common listen options excluding `socket_address`. For unix domain sockets, available options will be expanded to cover also filesystem permissions and owner for the socket. Storing listen options for both types of sockets in one structure would become messy. For now, both use `listen_cfg`. In a singular cql controller, only sockets of one type are created, thus it can be easily split into two cases. Isolate maintenance socket from `listen_cfg`.

…server::listen

margdoc · 2024-02-05T13:34:17Z

v2:

rebase on master
listen -> listen_on_all_shards
get rid of the magic number for the socket path's maximal length

margdoc · 2024-02-05T17:02:25Z

v3:

added option to change owning group

nuivall · 2024-02-05T17:17:39Z

@margdoc it's not immediately visible, is 1st commit pure code move?

avikivity · 2024-02-05T17:22:06Z

transport/controller.cc

+    }
+
+    struct stat statbuf;
+    auto stat_result = ::stat(socket.c_str(), &statbuf);


Don't bypass Seastar APIs.

I see it's a preexisting problem. Please fix it later in a follow-up.

avikivity · 2024-02-05T17:22:28Z

transport/controller.cc

+
+    // Remove the socket if it already exists, otherwise when the server
+    // tries to listen on it, it will hang on bind().
+    auto unlink_result = ::unlink(socket.c_str());


nuivall · 2024-02-05T17:27:11Z

It would be good to add to cover letter why the change to 660.

transport/controller.cc

avikivity · 2024-02-05T17:32:48Z

db/config.cc

@@ -764,6 +764,8 @@ db::config::config(std::shared_ptr<db::extensions> exts)
        "\tworkdir        the node will open the maintenance socket on the path <scylla's workdir>/cql.m,\n"
        "\t               where <scylla's workdir> is a path defined by the workdir configuration option\n"
        "\t<socket path>  the node will open the maintenance socket on the path <socket path>")
+    , maintenance_socket_group(this, "maintenance_socket_group", value_status::Used, "",
+        "The group that the maintenance socket will be owned by. If not set, the group will be the same as the user running the scylla node.")


We need to default it to scyllaadm in installations.

This means to set it scyllaadm in conf/scylla.yaml, but that's used in un-installed runs (./build/dev/scylla ...) (is it really? please check)

I think this is the most flexible wrt. packaging.

add --admin-group option to install.sh, and if set, use its value to adjust the installed scylla.aml.

use the new option in dist/redhat/scylla.spec and dist/debian/debian/rules, with scyllaadm as the group

also adjust those files to create scyllaadm and make it the scylla user's group (these files create the scylla user via useradd/adduser), also install.sh plays with users too

/cc @syuu1228

I guess this is quite some work (esp. testing) so can be done in a follow-up.

I think we need to use the scylla group. scyllaadm only exists in scylla-machine-image. Later, we'll need to adjust it to make scyllaadm a member of the scylla group.

nuivall · 2024-02-05T17:43:45Z

transport/controller.cc

+        struct group *grp;
+        grp = ::getgrnam(_config.maintenance_socket_group().c_str());
+        if (grp) {
+            auto chown_result = ::chown(socket.c_str(), ::geteuid(), grp->gr_gid);


if current user is not member of the group I guess this fails? Perhaps worth to document that or display more friendly message. Looks like there is a func for that: explain_message_errno_chown

Looks like there is a func for that: explain_message_errno_chown

It would need library libexplain to be installed. I don't know if it will always be true.

then perhaps some documentation and mention maintenance socket in the error message? If user sets this option he needs to run scylla as a part of this group, maybe it's obvious when you read the code, but as an user you get random chown it may be less obvious.

I think the message currently would look similar to:
"Failed to chown /path/cql.m: EPERM: Not owner"

Don't know exactly how strerror output look like in this case. But as you can see there is no link to maintenance socket feature and user may be a bit lost.

transport/controller.cc

margdoc · 2024-02-08T12:55:12Z

@margdoc it's not immediately visible, is 1st commit pure code move?

Yes.

It would be good to add to cover letter why the change to 660.

It is explained in the linked issue #16487.
Added also to the cover letter why it is needed.

…nce_socket Set filesystem permissions for the maintenance socket to 660. Fixes scylladb#16487

…al length Calculate `max_socket_length` from the size of the structure representing the Unix domain socket address.

Option `maintenance-socket-group` sets the owning group of the maintenance socket. If not set, the group will be the same as the user running the scylla node.

margdoc · 2024-02-19T09:29:28Z

v4:

more descriptive error messages
added a note in maintenance socket's docs
move the unix_domain_socket_permissions definition closer to its usage
more readable if statement

scylladb-promoter · 2024-02-19T09:34:24Z

Docs Preview 📖

Docs Preview for this pull request is available here

Changed Files:

operating-scylla/admin-tools/maintenance-socket

Note: This preview will be available for 30 days and will be automatically deleted after that period. You can manually trigger a new build by committing changes.

scylladb-promoter · 2024-02-19T14:36:08Z

🔴 CI State: FAILURE

✅ - Build
✅ - Container Test
✅ - dtest
❌ - Unit Tests

Failed Tests (4/25863):

Build Details:

Duration: 5 hr 3 min
Builder: i-01ebd67647fc95bec (m5ad.8xlarge)

margdoc · 2024-02-20T10:22:28Z

Failed tests:
#17422
#17423

Rerunning CI.

scylladb-promoter · 2024-02-20T12:25:13Z

🟢 CI State: SUCCESS

✅ - Build
✅ - Container Test
✅ - dtest
✅ - Unit Tests

Build Details:

Duration: 1 hr 59 min
Builder: spider2.cloudius-systems.com

margdoc force-pushed the maintenance_socket_permissions branch from a1fc469 to 7e540ee Compare February 1, 2024 14:58

kbr-scylla requested a review from avikivity February 2, 2024 12:55

dawmd reviewed Feb 2, 2024

View reviewed changes

ptrsmrn reviewed Feb 2, 2024

View reviewed changes

transport/controller.cc Outdated Show resolved Hide resolved

ptrsmrn reviewed Feb 2, 2024

View reviewed changes

transport/controller.cc Outdated Show resolved Hide resolved

margdoc added 2 commits February 5, 2024 14:20

transport/controller: pass unix_domain_socket_permissions to generic_…

4cecda7

…server::listen

margdoc force-pushed the maintenance_socket_permissions branch 2 times, most recently from 2e0f271 to 92e08c3 Compare February 5, 2024 13:32

avikivity reviewed Feb 5, 2024

View reviewed changes

nuivall reviewed Feb 5, 2024

View reviewed changes

transport/controller.cc Outdated Show resolved Hide resolved

avikivity reviewed Feb 5, 2024

View reviewed changes

nuivall reviewed Feb 5, 2024

View reviewed changes

transport/controller.cc Outdated Show resolved Hide resolved

nuivall approved these changes Feb 5, 2024

View reviewed changes

margdoc added 2 commits February 9, 2024 12:32

transport/controller: set unix_domain_socket_permissions for maintena…

fffb732

…nce_socket Set filesystem permissions for the maintenance socket to 660. Fixes scylladb#16487

transport/controller: get rid of magic number for socket path's maxim…

3819114

…al length Calculate `max_socket_length` from the size of the structure representing the Unix domain socket address.

maintenance_socket: add option to set owning group

182cfeb

Option `maintenance-socket-group` sets the owning group of the maintenance socket. If not set, the group will be the same as the user running the scylla node.

margdoc force-pushed the maintenance_socket_permissions branch from edff0df to 182cfeb Compare February 19, 2024 09:25

margdoc marked this pull request as ready for review February 19, 2024 09:31

avikivity approved these changes Feb 19, 2024

View reviewed changes

scylladb-promoter merged commit 93af3dd into scylladb:master Feb 20, 2024
6 checks passed

This was referenced Feb 21, 2024

Maintenance socket: set the default value of the maintenance-socket-group option #17442

Closed

Maintenance socket: don't bypass Seastar APIs when making system calls #17443

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintenance socket: set filesystem permissions to 660 #17113

Maintenance socket: set filesystem permissions to 660 #17113

margdoc commented Feb 1, 2024 •

edited

avikivity commented Feb 1, 2024

margdoc commented Feb 2, 2024

kbr-scylla commented Feb 2, 2024

dawmd left a comment

ptrsmrn Feb 2, 2024

nuivall Feb 5, 2024

avikivity Feb 20, 2024

ptrsmrn Feb 2, 2024

nuivall Feb 5, 2024

ptrsmrn Feb 2, 2024

ptrsmrn Feb 2, 2024

avikivity commented Feb 5, 2024

margdoc commented Feb 5, 2024

margdoc commented Feb 5, 2024

nuivall commented Feb 5, 2024

avikivity Feb 5, 2024

avikivity Feb 5, 2024

avikivity Feb 5, 2024

nuivall commented Feb 5, 2024

avikivity Feb 5, 2024

avikivity Feb 5, 2024

nuivall Feb 5, 2024

margdoc Feb 8, 2024

nuivall Feb 8, 2024

margdoc commented Feb 8, 2024 •

edited

margdoc commented Feb 19, 2024

scylladb-promoter commented Feb 19, 2024

scylladb-promoter commented Feb 19, 2024

margdoc commented Feb 20, 2024

scylladb-promoter commented Feb 20, 2024

		logger.info("Starting listening for CQL clients on {} ({}, {})"
		, addr, creds ? "encrypted" : "unencrypted", is_shard_aware ? "shard-aware" : "non-shard-aware"

Maintenance socket: set filesystem permissions to 660 #17113

Maintenance socket: set filesystem permissions to 660 #17113

Conversation

margdoc commented Feb 1, 2024 • edited

avikivity commented Feb 1, 2024

margdoc commented Feb 2, 2024

kbr-scylla commented Feb 2, 2024

dawmd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avikivity commented Feb 5, 2024

margdoc commented Feb 5, 2024

margdoc commented Feb 5, 2024

nuivall commented Feb 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuivall commented Feb 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

margdoc commented Feb 8, 2024 • edited

margdoc commented Feb 19, 2024

scylladb-promoter commented Feb 19, 2024

Docs Preview 📖

scylladb-promoter commented Feb 19, 2024

🔴 CI State: FAILURE

Failed Tests (4/25863):

Build Details:

margdoc commented Feb 20, 2024

scylladb-promoter commented Feb 20, 2024

🟢 CI State: SUCCESS

Build Details:

margdoc commented Feb 1, 2024 •

edited

margdoc commented Feb 8, 2024 •

edited