Added user and group checks. Auto create socket dir. #205

sbailey-arm · 2020-07-15T12:42:51Z

Parsec must be run as user parsec and parsec must be member of group parsec-client.
This is disabled if feature testing is specified.

Also auto created /tmp/parsec if it does not already exist with ownership set to user parsec and group parsec_clients. User parsec must be a member of parsec_clients in order for it to be able to change group ownership to it

Signed-off-by: Samuel Bailey [email protected]

sbailey-arm · 2020-07-15T12:45:45Z

I've tested this on my machine with a parsec user. If run as a user other than parsec, it complains about running as incorrect user. If run as parsec but parsec_clients group does not exist, throws error about not finding parsec_clients. If run as parsec and parsec_clients exists but parsec is not a member, throws error about parsec not being a member of parsec_clients. If run as parsec and parsec is a member of parsec_clients, it will create /tmp/parsec if required.

If I add my local user to parsec_clients and run Parsec as parsec, my local user can successfully run cargo test normal_tests. If my local user is not a member of parsec_clients, it fails.

sbailey-arm · 2020-07-16T07:49:34Z

(This isn't going to pass our stock CI as it won't be using the testing feature or the parsec user with parsec_clients group)

ionut-arm · 2020-07-16T10:20:04Z

I've tested this on my machine with a parsec user. If run as a user other than parsec, it complains about running as incorrect user. If run as parsec but parsec_clients group does not exist, throws error about not finding parsec_clients. If run as parsec and parsec_clients exists but parsec is not a member, throws error about parsec not being a member of parsec_clients. If run as parsec and parsec is a member of parsec_clients, it will create /tmp/parsec if required.

If I add my local user to parsec_clients and run Parsec as parsec, my local user can successfully run cargo test normal_tests. If my local user is not a member of parsec_clients, it fails.

I really like having the ability to do this! It'll make life much easier for admins... but, at the same time, I'd also like to give them an option - at the moment the only way to not have this run is by using the testing feature and that's clearly undesirable.

I was thinking you could add a new section in the config file, something like

[fs-permissions]
service-user = "parsec"
clients-group = "parsec-clients"

If the section exists, the feature is enabled, and it will be enabled in our example config file. We could add these fields (and a boolean flag tracking if it's enabled) in the GlobalConfig, so that other components can use it - Listener, maybe even OnDiskManager.

Thoughts?

sbailey-arm · 2020-07-17T13:08:10Z

If the section exists, the feature is enabled, and it will be enabled in our example config file. We could add these fields (and a boolean flag tracking if it's enabled) in the GlobalConfig, so that other components can use it - Listener, maybe even OnDiskManager.

That sounds like a good idea. It makes it easier for admins to enable or disable this feature, and they can also specify their own Parsec user and group.

hug-dev · 2020-07-17T13:13:04Z

I agree with having a separate fs-permissions feature for this but not sure with letting the possibility of changing the name of owner and group. These should ideally be hardcoded so that clients can check it as well.

ionut-arm · 2020-07-17T13:29:04Z

I agree with having a separate fs-permissions feature for this but not sure with letting the possibility of changing the name of owner and group. These should ideally be hardcoded so that clients can check it as well.

Makes sense. I really wish we could make this synchronisation problem disappear somehow - both around the paths and the usernames/groups

sbailey-arm · 2020-07-28T11:42:53Z

I agree with having a separate fs-permissions feature for this but not sure with letting the possibility of changing the name of owner and group. These should ideally be hardcoded so that clients can check it as well.

So would this mean we have a [fs-permissions] section with a single bool config like strict-user-and-groups without the service-user or clients-group, allowing the admin to turn on or off the security feature without changing names?

hug-dev · 2020-07-28T12:58:52Z

@ionut-arm If this feature is meant to be activated by default, do you think we could have it at build time? Like the one in the Rust client?

ionut-arm · 2020-07-28T13:01:04Z

@ionut-arm If this feature is meant to be activated by default, do you think we could have it at build time? Like the one in the Rust client?

Yeah, I think if we don't want to make any of the values configurable, just compiling it in (or not) makes sense

sbailey-arm · 2020-07-28T13:03:46Z

Isn't this what's already there? The compile time option testing will disable the check, which is enabled by default. (perhaps the name could be changed to parsec-user-and-clients group)

ionut-arm · 2020-07-28T13:26:39Z

Isn't this what's already there? The compile time option testing will disable the check, which is enabled by default. (perhaps the name could be changed to parsec-user-and-clients group)

Yep, that makes sense, just renaming it should be fine. I didn't realise it was a feature created by this PR, thought it was older (like in some other crates)

sbailey-arm · 2020-07-28T15:54:15Z

I've changed the feature to parsec-user-and-clients-group which is enabled as a default feature, instead of using the testing feature which would disable the extra security checks.

This commit adds a GlobalConfig structure with a `log_error_details` flag which controls if Rust error objects are logged along with the message. A `format_error` macro is also added to make the conditional logging easier. Signed-off-by: Ionut Mihalcea <[email protected]>

hug-dev · 2020-07-29T10:13:30Z

I guess on the CI we do not have to active the feature. But we should at some point add tests for a secure deployment where all checks are in place.

ionut-arm · 2020-07-29T10:27:48Z

src/front/domain_socket.rs

+                } else {
+                    // Invalid path - socket should not be created in root
+                    // `socket.path` returns None if there is no parent directory (i.e. is in root)
+                    error!("Socket path {} is invalid.", SOCKET_PATH);
+                    return Err(Error::new(
+                        ErrorKind::InvalidInput,
+                        "Socket path is invalid",
+                    ));
                }


Is this part actually needed, since the path is declared statically at the top?

I don't think so. It was added because socket.parent() can fail, but should never in our case. Does it make sense to remove the else? It should never be hit, but then if you look at the code without the else, it looks like it would continue as normal, even if it failed. Maybe converting the Option that parent() returns to a Result and returning the error (if there is one - which there never should be!) instead, to make the code more compact?

You can simply unwrap the result of socket.parent() - even if we ever change it to something where that fails, the service won't start so the CI will let us know.

sbailey-arm · 2020-08-03T11:21:07Z

I guess on the CI we do not have to active the feature. But we should at some point add tests for a secure deployment where all checks are in place.

I've modified the CI script to add --no-default-features which will remove parsec-user-and-clients-group. The ability to disable a single feature would be nice, but I'm not sure it'll happen any time soon...!

hug-dev · 2020-08-03T12:11:25Z

For the problem you described, it might be better to do something similar as in the client: have a no-fs-permission-check feature, disabled by default. You can check in the code if the feature is not there. That way it would be easier to selectively remove the checks 👍
See the Cargo.toml and the code.

hug-dev · 2020-08-04T10:08:12Z

There seems to have been two Travis Builds triggered by this PR. Weird.

src/front/domain_socket.rs

hug-dev · 2020-08-04T11:11:23Z

src/front/domain_socket.rs

+                "Parsec run as incorrect user",
+            ));
+        }
+        // Check Parsec client group exists and parsec user is a member of it


Does parsec need to be part of parsec-clients? Since parsec owns the socket dir, can't they use it how they want?

A user has to be a member of a group in order to change permissions of the group to that group. So if parsec is not a member of parsec-clients, it cannot change the group permission of the directory to parsec-clients.

I see, that makes sense, thanks.

hug-dev · 2020-08-04T11:20:22Z

With this PR, the parsec and parsec-clients user/group will be mandatory to even run Parsec. This will have to be documented clearly in our book.
Also I am thinking when we will have our first authenticators. The 750 permissions will then have to be 755, no longer needing the parsec-clients group. We would probably also prevent direct/none authentication.
By then this code will have to be modified to comply with that. Should we then stop at checking that the current user is parsec and the the permissions are 750?

sbailey-arm · 2020-08-04T11:35:20Z

With this PR, the parsec and parsec-clients user/group will be mandatory to even run Parsec. This will have to be documented clearly in our book.
Also I am thinking when we will have our first authenticators. The 750 permissions will then have to be 755, no longer needing the parsec-clients group. We would probably also prevent direct/none authentication.
By then this code will have to be modified to comply with that. Should we then stop at checking that the current user is parsec and the the permissions are 750?

Should we just get it into that state now instead of coming up with a stopgap that will definitely be changed at somepoint?

Parsec must be run as user `parsec` and `parsec` must be member of group `parsec-client`. This is disabled if feature `testing` is specified. Also auto created `/tmp/parsec` if it does not already exist. Signed-off-by: Samuel Bailey <[email protected]>

ionut-arm · 2020-08-04T11:46:46Z

With this PR, the parsec and parsec-clients user/group will be mandatory to even run Parsec. This will have to be documented clearly in our book.
Also I am thinking when we will have our first authenticators. The 750 permissions will then have to be 755, no longer needing the parsec-clients group. We would probably also prevent direct/none authentication.
By then this code will have to be modified to comply with that. Should we then stop at checking that the current user is parsec and the the permissions are 750?

I think some deployments might want to use this setup, even if we don't really recommend it. But maybe we should force them not to anyway - to at least use the peer credential authentication. That might be the "least secure" option we offer, converting the direct authentication.

none authentication is still needed for service discovery operations.

Should we just get it into that state now instead of coming up with a stopgap that will definitely be changed at somepoint?

Not really, because right now we need the parsec-clients group to do the "authentication" for us.

hug-dev · 2020-08-04T11:55:41Z

Oops sorry for closing 😅

I agree with your points and I think this is fine to go with this now. My main concern is that there will be some changes needed in this code if we remove the parsec-clients requirement when the peer credential authentication is used.

sbailey-arm force-pushed the add-user-and-group-checks-to-service branch 2 times, most recently from ab5de20 to 1404b42 Compare July 15, 2020 16:35

sbailey-arm force-pushed the add-user-and-group-checks-to-service branch 3 times, most recently from d42db07 to 2285d90 Compare July 28, 2020 15:51

sbailey-arm force-pushed the add-user-and-group-checks-to-service branch from 2285d90 to 91c838d Compare July 28, 2020 15:57

ionut-arm reviewed Jul 29, 2020

View reviewed changes

ionut-arm mentioned this pull request Jul 29, 2020

Better error message when file not found #210

Closed

sbailey-arm force-pushed the add-user-and-group-checks-to-service branch from 91c838d to f709548 Compare August 3, 2020 11:17

sbailey-arm force-pushed the add-user-and-group-checks-to-service branch from f709548 to 8f83866 Compare August 3, 2020 13:21

hug-dev reviewed Aug 4, 2020

View reviewed changes

sbailey-arm force-pushed the add-user-and-group-checks-to-service branch from 8f83866 to 6d9d9d9 Compare August 4, 2020 11:36

hug-dev closed this Aug 4, 2020

hug-dev reopened this Aug 4, 2020

hug-dev approved these changes Aug 4, 2020

View reviewed changes

hug-dev merged commit 50d6af5 into parallaxsecond:master Aug 4, 2020

hug-dev mentioned this pull request Aug 6, 2020

Auto create /tmp/parsec with correct permissions on startup #195

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added user and group checks. Auto create socket dir. #205

Added user and group checks. Auto create socket dir. #205

sbailey-arm commented Jul 15, 2020

sbailey-arm commented Jul 15, 2020

sbailey-arm commented Jul 16, 2020

ionut-arm commented Jul 16, 2020

sbailey-arm commented Jul 17, 2020

hug-dev commented Jul 17, 2020

ionut-arm commented Jul 17, 2020

sbailey-arm commented Jul 28, 2020

hug-dev commented Jul 28, 2020

ionut-arm commented Jul 28, 2020

sbailey-arm commented Jul 28, 2020

ionut-arm commented Jul 28, 2020 •

edited

Loading

sbailey-arm commented Jul 28, 2020

hug-dev commented Jul 29, 2020

ionut-arm Jul 29, 2020

sbailey-arm Aug 3, 2020

ionut-arm Aug 3, 2020

sbailey-arm commented Aug 3, 2020

hug-dev commented Aug 3, 2020 •

edited

Loading

hug-dev commented Aug 4, 2020

hug-dev Aug 4, 2020

sbailey-arm Aug 4, 2020

hug-dev Aug 4, 2020

hug-dev commented Aug 4, 2020

sbailey-arm commented Aug 4, 2020

ionut-arm commented Aug 4, 2020

hug-dev commented Aug 4, 2020

Added user and group checks. Auto create socket dir. #205

Added user and group checks. Auto create socket dir. #205

Conversation

sbailey-arm commented Jul 15, 2020

sbailey-arm commented Jul 15, 2020

sbailey-arm commented Jul 16, 2020

ionut-arm commented Jul 16, 2020

sbailey-arm commented Jul 17, 2020

hug-dev commented Jul 17, 2020

ionut-arm commented Jul 17, 2020

sbailey-arm commented Jul 28, 2020

hug-dev commented Jul 28, 2020

ionut-arm commented Jul 28, 2020

sbailey-arm commented Jul 28, 2020

ionut-arm commented Jul 28, 2020 • edited Loading

sbailey-arm commented Jul 28, 2020

hug-dev commented Jul 29, 2020

ionut-arm Jul 29, 2020

Choose a reason for hiding this comment

sbailey-arm Aug 3, 2020

Choose a reason for hiding this comment

ionut-arm Aug 3, 2020

Choose a reason for hiding this comment

sbailey-arm commented Aug 3, 2020

hug-dev commented Aug 3, 2020 • edited Loading

hug-dev commented Aug 4, 2020

hug-dev Aug 4, 2020

Choose a reason for hiding this comment

sbailey-arm Aug 4, 2020

Choose a reason for hiding this comment

hug-dev Aug 4, 2020

Choose a reason for hiding this comment

hug-dev commented Aug 4, 2020

sbailey-arm commented Aug 4, 2020

ionut-arm commented Aug 4, 2020

hug-dev commented Aug 4, 2020

ionut-arm commented Jul 28, 2020 •

edited

Loading

hug-dev commented Aug 3, 2020 •

edited

Loading