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

DESKPROFILE: Add checks for user and host category #495

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@fidencio
Contributor

fidencio commented Jan 21, 2018

freeipa-deskprofile-plugin can have both user and host category set as
"all" and when it happens, no users and groups or hosts or hostgroups
are going to be set.

Let's treat this expected (but so far missed) situation on SSSD side.

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

Signed-off-by: Fabiano Fidêncio fidencio@redhat.com

@fidencio

This comment has been minimized.

Contributor

fidencio commented Jan 22, 2018

Steps to reproduce the issue:

  1. git clone https://github.com/fidencio/fleet-commander-vagans
  2. In the project's folder do: cd fleet-commander; vagrant up
  3. Once the VMs are provisioned, access cockpit: https://master.ipa.example:9090/
  4. Click in the "FleetCommander" tab
  5. Close the windows that will open
  6. Click in "Add Profile"
  7. Add a profile without filling out Hosts and Hostgroups
  8. From the fleet-commander folder, do: vagrant ssh ipaclient
    8.1: ssh to the machine using admin's user: ssh -l admin localhost
    8.2: Close the admin session
    8.3 A root, take a look at journalctl -xe and a message about a crash will be seen.

With the patch, 8.3 won't happen and your profile will be downloaded to /var/lib/deskprofile/ipa.example/admin/

NOTE: If your way to test it is through a fedpkg build, be aware of: https://bugzilla.redhat.com/show_bug.cgi?id=1536854

@jhrozek jhrozek self-assigned this Jan 24, 2018

fidencio added a commit to fidencio/fleet-commander-vagans that referenced this pull request Jan 29, 2018

builder: Enable fidencio's SSSD copr
This is just a workaround while we don't have PRs #495 and #497 merged
and backported to Fedora.

PR #495: SSSD/sssd#495
PR #497: SSSD/sssd#497

Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org>
@jhrozek

Just change the string comparison, please.

"ipa_deskprofile_rule_check_memberhost() failed [%d]: %s\n",
ret, sss_strerror(ret));
goto done;
if (hostcat != NULL && strcmp(hostcat, "all") == 0) {

This comment has been minimized.

@jhrozek

jhrozek Feb 4, 2018

Contributor

I have one nitpick here, the IPA HBAC code checks the category with strcasecmp, can we do the same here? Otherwise LGTM and the code was demonstrated to me (and a room full of people) to work

@fidencio

This comment has been minimized.

Contributor

fidencio commented Feb 5, 2018

Patch has been updated, thanks for the review.

@jhrozek jhrozek added the Accepted label Feb 5, 2018

@jhrozek

jhrozek approved these changes Feb 5, 2018

@jhrozek

This comment has been minimized.

Contributor

jhrozek commented Feb 5, 2018

I have no more requests, thank you, ack.

@jhrozek

This comment has been minimized.

Contributor

jhrozek commented Feb 5, 2018

Gah, no, sorry, I found another nitpick :-(

@jhrozek jhrozek removed the Accepted label Feb 5, 2018

ret, sss_strerror(ret));
goto done;
if (usercat != NULL && strcasecmp(usercat, "all") == 0) {
user_prio = talloc_strdup(tmp_ctx, rule_prio);

This comment has been minimized.

@jhrozek

jhrozek Feb 5, 2018

Contributor

The allocations you added are not checked against ENOMEM. Which reminds me to also run Coverity..becuase Coverity might complain that we check talloc retval N times, but not here.

DESKPROFILE: Add checks for user and host category
freeipa-deskprofile-plugin can have both user and host category set as
"all" and when it happens, no users and groups or hosts or hostgroups
are going to be set.

Let's treat this expected (but so far missed) situation on SSSD side.

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

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
@fidencio

This comment has been minimized.

Contributor

fidencio commented Feb 5, 2018

Here are the changes introduced in the last patch:

[ffidenci@pessoa x86_64]$ git diff
diff --git a/src/providers/ipa/ipa_deskprofile_rules_util.c b/src/providers/ipa/ipa_deskprofile_rules_util.c
index ffcb5c846..01b7d0527 100644
--- a/src/providers/ipa/ipa_deskprofile_rules_util.c
+++ b/src/providers/ipa/ipa_deskprofile_rules_util.c
@@ -779,7 +779,22 @@ ipa_deskprofile_rules_save_rule_to_disk(
 
     if (usercat != NULL && strcasecmp(usercat, "all") == 0) {
         user_prio = talloc_strdup(tmp_ctx, rule_prio);
+        if (user_prio == NULL) {
+            DEBUG(SSSDBG_CRIT_FAILURE,
+                  "Failed to allocate the user priority "
+                  "when user category is \"all\"\n");
+            ret = ENOMEM;
+            goto done;
+        }
+
         group_prio = talloc_strdup(tmp_ctx, rule_prio);
+        if (group_prio == NULL) {
+            DEBUG(SSSDBG_CRIT_FAILURE,
+                  "Failed to allocate the group priority "
+                  "when user category is \"all\"\n");
+            ret = ENOMEM;
+            goto done;
+        }
     } else {
         ret = ipa_deskprofile_rule_check_memberuser(tmp_ctx, domain, rule,
                                                     rule_name, rule_prio,
@@ -795,7 +810,22 @@ ipa_deskprofile_rules_save_rule_to_disk(
 
     if (hostcat != NULL && strcasecmp(hostcat, "all") == 0) {
         host_prio = talloc_strdup(tmp_ctx, rule_prio);
+        if (host_prio == NULL) {
+            DEBUG(SSSDBG_CRIT_FAILURE,
+                  "Failed to allocate the host priority "
+                  "when host category is \"all\"\n");
+            ret = ENOMEM;
+            goto done;
+        }
+
         hostgroup_prio = talloc_strdup(tmp_ctx, rule_prio);
+        if (hostgroup_prio == NULL) {
+            DEBUG(SSSDBG_CRIT_FAILURE,
+                  "Failed to allocate the hostgroup priority "
+                  "when host category is \"all\"\n");
+            ret = ENOMEM;
+            goto done;
+        }
     } else {
         ret = ipa_deskprofile_rule_check_memberhost(tmp_ctx, domain, rule,
                                                     rule_name, rule_prio,
@jhrozek

This comment has been minimized.

Contributor

jhrozek commented Feb 5, 2018

OK, Coverity came clean, thank you.

@jhrozek jhrozek added the Accepted label Feb 5, 2018

@jhrozek

jhrozek approved these changes Feb 5, 2018

@jhrozek

This comment has been minimized.

Contributor

jhrozek commented Feb 5, 2018

@jhrozek jhrozek closed this Feb 5, 2018

@jhrozek jhrozek added the Pushed label Feb 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment