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.

Sign up for GitHub

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

Jump to bottom

A bunch of assorted patches mostly around sss_client #7042

Closed
wants to merge 14 commits into from
Closed

A bunch of assorted patches mostly around sss_client #7042

wants to merge 14 commits into from

Conversation

alexey-tikhonov
Copy link
Member

@alexey-tikhonov alexey-tikhonov commented Nov 16, 2023 •
edited
Loading

with the main goal to avoid the need to set ':root' ownership to 'sssd_sudo' socket.

I'm not sure if sudosrv_cmd() is the best place to check client creds (91d2f0f)
Additionally I'm not sure but probably it's worth to add an artificial delay before closing client's socket - to avoid potential "dos"?

Comments are welcome.

@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Nov 16, 2023
@alexey-tikhonov alexey-tikhonov force-pushed the sudo-responder branch 5 times, most recently from bd6ab76 to 0cd99fb Compare November 20, 2023 14:18
@alexey-tikhonov alexey-tikhonov marked this pull request as ready for review November 20, 2023 14:28
justin-stephenson
justin-stephenson approved these changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@justin-stephenson justin-stephenson left a comment

Choose a reason for hiding this comment

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

Changes LGTM, Ack. Thank you.

@alexey-tikhonov
Copy link
Member Author

'system' tests fails in PR CI if those patches are built '--with-sssd-user=sssd' and SSSD is run with "sssd.conf::user=sssd".

@alexey-tikhonov
Copy link
Member Author

I found and fixed one bug - aeda855 , but 'system' tests still fail miserably when SSSD runs under 'sssd' user on centos-8 (only) - #7044

@alexey-tikhonov
Copy link
Member Author

but 'system' tests still fail miserably when SSSD runs under 'sssd' user on centos-8 (only) - #7044

Ah, I got it - this is because code relies on HAVE_PTHREAD_EXT that centos-8 doesn't have.
But sssd-2.10 won't be shipped on centos-8, so that's ok.

pbrezina
pbrezina requested changes Dec 4, 2023
View reviewed changes
Copy link
Member

@pbrezina pbrezina left a comment

Choose a reason for hiding this comment

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

Hi, thank you for your work. Please, see minor comments inside.

  • CLIENT: remove check for rw-rw-rw- : can you describe in the commit message why it does not make sense?
  • PAM: no need in root:root owned socket: s/in/for?

src/tests/intg/getsockopt_wrapper.c Outdated Show resolved Hide resolved
src/sss_client/autofs/sss_autofs.c Outdated Show resolved Hide resolved
src/responder/sudo/sudosrv_cmd.c Show resolved Hide resolved
@alexey-tikhonov
Copy link
Member Author 

* CLIENT: remove check for rw-rw-rw- : can you describe in the commit message why it does not make sense?

Hm... actually I'm ok to drop this patch.
The things is, I just can't understand rationale for this check (i.e. "check doesn't make sense from my point of view", as I wrote in the commit message).

I mean, what does this check ensure? That file doesn't have 'x'?
That file is 'rw' by 'other' (besides 'ug')? But why?

* PAM: no need in root:root owned socket: s/in/for?

Changed.

pbrezina
pbrezina approved these changes Dec 5, 2023
View reviewed changes
Copy link
Member

@pbrezina pbrezina left a comment

Choose a reason for hiding this comment

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

CI error is unrelated.

sumit-bose
sumit-bose reviewed Dec 5, 2023
View reviewed changes
src/sss_client/common.c Outdated Show resolved Hide resolved
@alexey-tikhonov
CLIENT: move all socket paths checks to a single function
aee0cc1
@alexey-tikhonov
CLIENT: remove check for rw-rw-rw-
c866321 
as it doesn't make much sense anyway.
@alexey-tikhonov
KRB5: a comment to explain the need for explicit `sss_pac_check_and_o…
8c1469b 
…pen()`
@alexey-tikhonov
CLIENT: reduce code duplication
7f8d897
@alexey-tikhonov
CLIENT: add an optional check of server credentials
8e98fd0 
to `sss_cli_make_request_with_checks()`

This requires to make sure 'sss_sssd_*id' are initialized in
`check_server_cred()`
@alexey-tikhonov
CLIENT: reduce code duplication
1e93de7
@alexey-tikhonov
CLIENT: SUDO: force check of server credentials
63cf148 
as a general hardening
@alexey-tikhonov
CLIENT: move sudo/autofs/ssh related code
156a18b 
out of common module
@alexey-tikhonov
SUDO: refuse to serve clients running under non-root
4182d2a
@alexey-tikhonov
SUDO: make 'sssd_sudo' socket sssd:sssd owned
62b1130 
The only intended client of 'sssd_sudo' is 'sudo' that is suid
binary and thus still can access socket.
But if for whatever reason it's undesirable to make 'sudo' use
its CAP_DAC_OVERRIDE capability then socket mode can be changed
to rw-rw-rw -- previous patch will restrict access to the socket
for root only.

The reason for this change is to avoid the need for CAP_CHOWN for
SSSD itself.
@alexey-tikhonov
PAM: no need for root:root owned socket
8a81fad 
since 1451c6e
@alexey-tikhonov
RESPONDER: remove support for custom pipe_fd
9b6b528 
from `sss_process_init()` as it's not used anymore
@alexey-tikhonov
SUDO: don't overwrite major error code with minor one
bbb1d50 
The latter can be zero (example: socket closed during
`sss_cli_recv_rep()`)
@alexey-tikhonov
CLIENT: fixed a mistype in check_socket_cred()
d56db42
@alexey-tikhonov
Copy link
Member Author

Hi @sumit-bose,

I addressed your comments in the latest version.

sumit-bose
sumit-bose approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

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

Hi,

thanks for the updates, ACK.

bye,
Sumit

@alexey-tikhonov
Copy link
Member Author

Pushed PR: #7042

  • master
    • ad70f15 - CLIENT: fixed a mistype in check_socket_cred()
    • 8f58e22 - SUDO: don't overwrite major error code with minor one
    • 4d6551e - RESPONDER: remove support for custom pipe_fd
    • 4a01583 - PAM: no need for root:root owned socket
    • ff2a711 - SUDO: make 'sssd_sudo' socket sssd:sssd owned
    • 8d0a88e - SUDO: refuse to serve clients running under non-root
    • 32b67e6 - CLIENT: move sudo/autofs/ssh related code
    • 4e1a794 - CLIENT: SUDO: force check of server credentials
    • 1f8ec39 - CLIENT: reduce code duplication
    • 57ed0de - CLIENT: add an optional check of server credentials
    • 079f433 - CLIENT: reduce code duplication
    • 4255a0f - KRB5: a comment to explain the need for explicit sss_pac_check_and_open()
    • 41f8a68 - CLIENT: remove check for rw-rw-rw-
    • 2a3e47a - CLIENT: move all socket paths checks to a single function

@alexey-tikhonov alexey-tikhonov mentioned this pull request Mar 6, 2024
Closed
@alexey-tikhonov alexey-tikhonov mentioned this pull request Mar 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pbrezina pbrezina pbrezina approved these changes

@sumit-bose sumit-bose sumit-bose approved these changes

@justin-stephenson justin-stephenson justin-stephenson approved these changes

Assignees

@justin-stephenson justin-stephenson

@sumit-bose sumit-bose

@pbrezina pbrezina

Labels
no-backport This should go to target branch only. non-privileged Pushed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexey-tikhonov @justin-stephenson @sumit-bose @pbrezina