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.

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

SELINUX: Use getseuserbyname to get IPA seuser #189

Closed
wants to merge 1 commit into from
Closed

SELINUX: Use getseuserbyname to get IPA seuser #189

wants to merge 1 commit into from

Conversation

justin-stephenson
Copy link
Contributor

Retrieve SELinux username utilizing libselinux API as a more reliable method than libsemanage calls and remove get_seuser function which is no longer needed.

Resolves:
https://pagure.io/SSSD/sssd/issue/3308

Tested on IPA client with:

  • running semanage login -d testuser
  • login as testuser and check /var/log/sssd/selinux_child.log

@centos-ci
Copy link

Can one of the admins verify this patch?

1 similar comment
@centos-ci
Copy link

Can one of the admins verify this patch?

@justin-stephenson
SELINUX: Use getseuserbyname to get IPA seuser
503be66 
libsemanage API function semanage_seuser_query can fail in
certain circumstances preventing user logins. The libselinux function
getseuserbyname will be a more reliable method to retrieve SELinux
usernames and is recommended by SELinux developers. Replace get_seuser
function with getseuserbyname.

Resolves:
https://pagure.io/SSSD/sssd/issue/3308
@justin-stephenson justin-stephenson force-pushed the jstephen-getseuserbyname-seuser-change branch from cc5400e to 503be66 Compare March 10, 2017 00:13
@jhrozek
Copy link
Contributor

jhrozek commented Mar 10, 2017

ok to test

@lslebodn
Copy link
Contributor

lslebodn commented Apr 3, 2017

@justin-stephenson are you able to reproduce bug with semanage login -d testuser
Because I used a little bit complicated test-case and I still can reproduce bug from comment
https://pagure.io/SSSD/sssd/issue/3308#comment-220396

(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [main] (0x2000): Running with effective IDs: [0][0].
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [main] (0x2000): Running with real IDs [0][0].
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [main] (0x0400): context initialized
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [unpack_buffer] (0x2000): seuser length: 12
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [unpack_buffer] (0x2000): seuser: unconfined_u
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [unpack_buffer] (0x2000): mls_range length: 14
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [unpack_buffer] (0x2000): mls_range: s0-s0:c0.c1023
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [unpack_buffer] (0x2000): username length: 5
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [unpack_buffer] (0x2000): username: admin
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [main] (0x0400): performing selinux operations
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [seuser_needs_update] (0x2000): getseuserbyname: ret: 0 seuser: admin mls: unknown
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [sss_semanage_init] (0x0020): SELinux policy not managed
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [set_seuser] (0x0020): Cannot init SELinux management
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [main] (0x0020): Cannot set SELinux login context.
(Mon Apr  3 13:07:22 2017) [[sssd[selinux_child[1578]]]] [main] (0x0020): selinux_child failed!

@justin-stephenson
Copy link
Contributor Author

@lslebodn in my testing, the SELinux child process gets called twice during IPA client login. Before the patch the first call would error with similar libsemanage errors but the second would be successful. These are just cosmetic errors however, I could not reproduce any failed login problem.

[[sssd[selinux_child[3047]]]] [main] (0x0400): selinux_child started.
[[sssd[selinux_child[3047]]]] [main] (0x2000): Running with effective IDs: [0][0].
[[sssd[selinux_child[3047]]]] [main] (0x2000): Running with real IDs [0][0].
[[sssd[selinux_child[3047]]]] [main] (0x0400): context initialized
[[sssd[selinux_child[3047]]]] [unpack_buffer] (0x2000): seuser length: 12
[[sssd[selinux_child[3047]]]] [unpack_buffer] (0x2000): seuser: unconfined_u
[[sssd[selinux_child[3047]]]] [unpack_buffer] (0x2000): mls_range length: 14
[[sssd[selinux_child[3047]]]] [unpack_buffer] (0x2000): mls_range: s0-s0:c0.c1023
[[sssd[selinux_child[3047]]]] [unpack_buffer] (0x2000): username length: 9
[[sssd[selinux_child[3047]]]] [unpack_buffer] (0x2000): username: testuser1
[[sssd[selinux_child[3047]]]] [main] (0x0400): performing selinux operations
[[sssd[selinux_child[3047]]]] [libsemanage] (0x0020): could not query record value
[[sssd[selinux_child[3047]]]] [get_seuser] (0x0020): Cannot query for testuser1
[[sssd[selinux_child[3047]]]] [seuser_needs_update] (0x2000): get_seuser: ret: 5 seuser: unknown mls: unknown
[[sssd[selinux_child[3047]]]] [pack_buffer] (0x0400): result [0]
[[sssd[selinux_child[3047]]]] [prepare_response] (0x4000): r->size: 4
[[sssd[selinux_child[3047]]]] [main] (0x0400): selinux_child completed successfully
[[sssd[selinux_child[3063]]]] [main] (0x0400): selinux_child started.
[[sssd[selinux_child[3063]]]] [main] (0x2000): Running with effective IDs: [0][0].
[[sssd[selinux_child[3063]]]] [main] (0x2000): Running with real IDs [0][0].
[[sssd[selinux_child[3063]]]] [main] (0x0400): context initialized
[[sssd[selinux_child[3063]]]] [unpack_buffer] (0x2000): seuser length: 12
[[sssd[selinux_child[3063]]]] [unpack_buffer] (0x2000): seuser: unconfined_u
[[sssd[selinux_child[3063]]]] [unpack_buffer] (0x2000): mls_range length: 14
[[sssd[selinux_child[3063]]]] [unpack_buffer] (0x2000): mls_range: s0-s0:c0.c1023
[[sssd[selinux_child[3063]]]] [unpack_buffer] (0x2000): username length: 9
[[sssd[selinux_child[3063]]]] [unpack_buffer] (0x2000): username: testuser1
[[sssd[selinux_child[3063]]]] [main] (0x0400): performing selinux operations
[[sssd[selinux_child[3063]]]] [get_seuser] (0x0040): SELinux user for testuser1: unconfined_u
[[sssd[selinux_child[3063]]]] [get_seuser] (0x0040): SELinux range for testuser1: s0-s0:c0.c1023
[[sssd[selinux_child[3063]]]] [seuser_needs_update] (0x2000): get_seuser: ret: 0 seuser: unconfined_u mls: s0-s0:c0.c1023
[[sssd[selinux_child[3063]]]] [pack_buffer] (0x0400): result [0]
[[sssd[selinux_child[3063]]]] [prepare_response] (0x4000): r->size: 4
[[sssd[selinux_child[3063]]]] [main] (0x0400): selinux_child completed successfully

After the patch, both calls are successful and the libsemanage errors never happen. Do you have some reproducer instructions for the failures you mention?

@lslebodn
Copy link
Contributor

lslebodn commented Apr 3, 2017

@justin-stephenson If you have a time could you test patch #165 with your use-case.
If you are busy then I will have a time tomorrow.

@justin-stephenson
Copy link
Contributor Author

@lslebodn I tested the patch in #165 and it successfully resolves the original sssd errors [libsemanage] (0x0020): could not query record value however I don't know if it would solve the issue reported downstream BZ 1412717, this was the main reason I submitted this PR.

I could also modify this PR to not touch get_seuser() code and only call getseuserbyname() if get_seuser() fails.

@fidencio
Copy link
Contributor

@lslebodn, @justin-stephenson: What's the state of this PR? Is this still valid?
In case it's still valid, @justin-stephenson, may I ask you to rebase the patches based on our git master as currently they have some conflicts?

@justin-stephenson
Copy link
Contributor Author

@fidencio I don't really know if this ticket is required anymore to be honest, it may not be required after https://pagure.io/SSSD/sssd/issue/3297 was fixed.

I don't think any user is waiting for a fix, I will go ahead and close this PR and I will leave the decision to close upstream ticket 3308 to your team.

@justin-stephenson justin-stephenson deleted the jstephen-getseuserbyname-seuser-change branch July 28, 2017 17:51
@jhrozek
Copy link
Contributor

jhrozek commented Jul 30, 2017

Well, not so fast :) @mzidek-rh don't we want to use the libsemanage API anyway? Didn't this solve some real world bug?

@mzidek-gh
Copy link
Contributor

@jhrozek this patch replaces function from libsemanage with function from libselinux... The commit message says that libselinux is recommended over libsemanage by SELinux developers. If that is the case, I think it makes sense to use the preferred version. So I would not close this PR. Also this patch removes more code than it adds, which is welcomed.

@mzidek-gh
Copy link
Contributor

By the way in this issue: https://pagure.io/SSSD/sssd/issue/3308

it states that Petr Lautrbach recommended to use the libselinux function. I think that is reason enough to reopen this PR, even though it does not have high priority, because the more important selinux bug we had was resolved differently. (@justin-stephenson, you already deleted the branch so I can not reopen it, would you mind creating the branch again?)

@justin-stephenson justin-stephenson restored the jstephen-getseuserbyname-seuser-change branch July 31, 2017 12:38
@justin-stephenson
Copy link
Contributor Author

@mzidek-rh I pushed my local copy of the branch to my fork but a new PR was created(sorry for that).

I rebased the patch and tested it again to be sure it still works.

New PR is #342

@pbrezina pbrezina mentioned this pull request Apr 28, 2020
Closed
@sssd-bot sssd-bot mentioned this pull request May 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lslebodn lslebodn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@justin-stephenson @centos-ci @jhrozek @lslebodn @fidencio @mzidek-gh