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

Further restriction of child processes capabilities (part 2) #7703

Closed
wants to merge 10 commits into from
Closed

Further restriction of child processes capabilities (part 2) #7703

wants to merge 10 commits into from

Conversation

alexey-tikhonov
Copy link
Member

Minimizes capabilities required by 'krb5_child'.

@alexey-tikhonov alexey-tikhonov force-pushed the caps-again-krb5 branch 4 times, most recently from 8f69432 to 636f01e Compare November 20, 2024 11:16
@alexey-tikhonov alexey-tikhonov force-pushed the caps-again-krb5 branch 12 times, most recently from 4e7bdd4 to 59fbad5 Compare November 27, 2024 17:34
@alexey-tikhonov alexey-tikhonov marked this pull request as ready for review November 28, 2024 13:34
@alexey-tikhonov
Copy link
Member Author

Hm... Some tests still fail when run with service User=root:

[153555]] [k5c_setup_fast] (0x0100): [RID#7] Fast principal is set to [host/[email protected]]
[153555]] [find_principal_in_keytab] (0x4000): [RID#7] Trying to find principal host/[email protected] in keytab.
[153555]] [match_principal] (0x1000): [RID#7] Principal matched to the sample (host/[email protected]).
[153555]] [get_tgt_times] (0x0020): [RID#7] krb5_cc_retrieve_cred failed
[153555]] [get_tgt_times] (0x0020): [RID#7] 3284: [-1765328190][Credentials cache permissions incorrect]
[153556]] [get_fast_ccache_with_keytab] (0x2000): [RID#7] Running as [0][0].
[153556]] [set_canonicalize_option] (0x0100): [RID#7] Canonicalization is set to [true]
[153556]] [create_ccache] (0x4000): [RID#7] Initializing ccache of type [FILE]
[153556]] [create_ccache] (0x4000): [RID#7] krb5_cc_initialize failed
[153556]] [create_ccache] (0x0020): [RID#7] 1570: [-1765328190][Credentials cache permissions incorrect]
[153556]] [get_fast_ccache_with_keytab] (0x0020): [RID#7] get_and_save_tgt_with_keytab failed: -1765328190
[153555]] [get_fast_ccache_with_keytab] (0x0080): [RID#7] Creating FAST ccache failed, krb5_child will likely fail!
[153555]] [get_tgt_times] (0x0020): [RID#7] krb5_cc_retrieve_cred failed

This is despite

  Command: |
    sed -i "s/^User=.*/User=root/g" /usr/lib/systemd/system/sssd.service
    sed -i "s/^Group=.*/Group=root/g" /usr/lib/systemd/system/sssd.service
    sed -i "s/^#SupplementaryGroups=sssd$/SupplementaryGroups=sssd/g" /usr/lib/systemd/system/sssd.service
    sed -i "s/sssd:sssd/root:root/g" /usr/lib/systemd/system/sssd.service
    chown -f root:root /var/lib/sss/db/*.ldb || true
    rm -f /var/lib/sss/db/fast_ccache_* || true

Hm.. I think I have an idea...

@alexey-tikhonov
Copy link
Member Author

renew_tgt_child() is broken.

@spoore1
Copy link
Contributor

spoore1 commented Dec 3, 2024

FYI, I ran some tests against the copr build sssd-9.pr7703-05810.el10.x86_64 and they passed with SSSD running as both root and sssd.

@alexey-tikhonov alexey-tikhonov mentioned this pull request Dec 4, 2024
Open
@alexey-tikhonov
Copy link
Member Author

(1) why "TGT not found or expired" if sssd_kcm "Sending a reply with 711 bytes of payload"
Ticket was requested with args=["-r", "2s", "-l", "2s"], and krb5_renew_interval="1s". Is it considered expired?

This is not introduced by this PR.

Created #7742 to track this.

@alexey-tikhonov
Copy link
Member Author

A rebase.

spoore1
spoore1 approved these changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

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

Change seems to function as expected with the tests suggested.

sumit-bose
sumit-bose reviewed Dec 5, 2024
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,

thank you for the patch code/functionality-wise I'm fine.

The removal of the path creation for FILE: and DIR: should get a prominent release-note entry, especially since it will be removed in a minor release.

If would be nice to have a comment in the related commit message about the removal of krb5-child-test. I'm not sure if someone is really using it but maybe a release note entry might appropriate as well.

bye,
Sumit

@alexey-tikhonov
KRB5: don't pre-create parent dir(s) of wanted DIR:/FILE:
0fa54aa 
to match 'kinit' behavior and avoid the need for cap_chown and
cap_dac_override.

:relnote:SSSD doesn't create anymore missing path components of DIR:/FILE:
ccache types while acquiring user's TGT. The parent directory of requested
ccache directory must exist and the user trying to log in must have 'rwx'
access to this directory. This matches behavior of 'kinit'.
@alexey-tikhonov
KRB5: skip switch_creds() in PKINIT case
46f9fef 
Since 'krb5_child' has lost set-id bit and is run under uid/gid of
the backend, it was a no-op.
@alexey-tikhonov
KRB5: 'fast-ccache-uid/gid' args aren't used anymore
6d8800e
@alexey-tikhonov
KRB5: don't require effective CAP_DAC_READ_SEARCH
983aedf
@alexey-tikhonov
KRB5: verbosity
0d7cba9
@alexey-tikhonov
KRB5: drop cap_set*id as soon as possible
ece1574 
Set user uid/gid as real IDs as a first step in `privileged_krb5_setup()`
and drop cap_set*id afterwards.

Having real_ids == user_ids and set_ids == service_ids should be
enough to switch thru and back.

:relnote:`krb5-child-test` was removed. Corresponding tests under
'src/tests/system/' are aimed to provide a comprehensive test coverage
of 'krb5_child' functionality.
@alexey-tikhonov
KRB5: 'krb5_child' doesn't require effective capabilities
1b152da
@alexey-tikhonov
become_user() moved to src/monitor
771ecee 
Monitor is the only user of this function and only if built
with support of deprecated 'sssd.conf::user' option.
@alexey-tikhonov
KRB5: cosmetics
856ade4 
Remove non existent / private functions from a header.
@alexey-tikhonov
Copy link
Member Author

The removal of the path creation for FILE: and DIR: should get a prominent release-note entry, especially since it will be removed in a minor release.

If would be nice to have a comment in the related commit message about the removal of krb5-child-test. I'm not sure if someone is really using it but maybe a release note entry might appropriate as well.

Updated commit messages of corresponding patches.

sumit-bose
sumit-bose approved these changes Dec 5, 2024
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,

thank you for the updates, ACK.

bye,
Sumit

@alexey-tikhonov
Copy link
Member Author

Pushed PR: #7703

  • master
    • a406c1b - KRB5: cosmetics
    • 988e5fa - become_user() moved to src/monitor
    • 19a871a - KRB5: 'krb5_child' doesn't require effective capabilities
    • 6553877 - KRB5: drop cap_set*id as soon as possible
    • 89d61e6 - KRB5: verbosity
    • 19dd643 - KRB5: don't require effective CAP_DAC_READ_SEARCH
    • 947f791 - KRB5: 'fast-ccache-uid/gid' args aren't used anymore
    • 541c42b - KRB5: skip switch_creds() in PKINIT case
    • 5e17bc2 - KRB5: don't pre-create parent dir(s) of wanted DIR:/FILE:
    • 1ef3cf5 - KRB5: verbosity around ccname handling
  • sssd-2-10
    • 01bc370 - KRB5: cosmetics
    • 0890828 - become_user() moved to src/monitor
    • be5174d - KRB5: 'krb5_child' doesn't require effective capabilities
    • 29a8a22 - KRB5: drop cap_set*id as soon as possible
    • d2892fe - KRB5: verbosity
    • cfbb36e - KRB5: don't require effective CAP_DAC_READ_SEARCH
    • f21107a - KRB5: 'fast-ccache-uid/gid' args aren't used anymore
    • f0957bc - KRB5: skip switch_creds() in PKINIT case
    • ce85278 - KRB5: don't pre-create parent dir(s) of wanted DIR:/FILE:
    • 1e4bb21 - KRB5: verbosity around ccname handling

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

@spoore1 spoore1 spoore1 approved these changes

Assignees

@spoore1 spoore1

@sumit-bose sumit-bose

@pbrezina pbrezina

Labels
branch: sssd-2-10 non-privileged Pushed
Projects
None yet
Milestone
No milestone
Development

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