URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: opened
PR body: """ This patch series contains: - some refactoring done on access module (and, consequently, on HBAC) in order to reuse a some code in the session module (an, consequently, on FleetCommander code); - the new session module that provides the FleetCommander Integration; """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ I've updated the patch set accordingly to https://pagure.io/SSSD/sssd/issue/2995#comment-447913 """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-314018566
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ CI: http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/7173/
Failures do not seem to be related to this patch set. """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-314036112
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ Here [0] you can find some step-by-step on how to test the functionality of those patches. It's important to mention that @olivergs has been doing some functional tests on those patches, using the copr repo provided by [0].
[0]: https://copr.fedorainfracloud.org/coprs/fidencio/deskprofile/ """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-314434411
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ I've updated the patch set and the changes are: - as @jhrozek agreed on the approach proposed by FleetCommander guys, the 2 "fixup" patches got squashed into "DESKPROFILE: Introduce the new IPA session provider"; - updated the link to thefor design page; """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-314467404
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ There are some issues Coverity found: ``` Error: TOCTOU (CWE-367): sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:157: fs_check_call: Calling function "stat" to perform check on "domain_dir". sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:159: toctou: Calling function "mkdir" that uses "domain_dir" after a check function. This can cause a time-of-check, time-of-use race condition. # 157| ret = stat(domain_dir, &info); # 158| if (ret != EOK) { # 159|-> ret = mkdir(domain_dir, 0755); # 160| if (ret != EOK) { # 161| ret = errno;
Error: TOCTOU (CWE-367): sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:214: fs_check_call: Calling function "stat" to perform check on "user_dir". sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:216: toctou: Calling function "mkdir" that uses "user_dir" after a check function. This can cause a time-of-check, time-of-use race condition. # 214| ret = stat(user_dir, &info); # 215| if (ret != EOK) { # 216|-> ret = mkdir(user_dir, 0600); # 217| if (ret != EOK) { # 218| ret = errno;
Error: TOCTOU (CWE-367): sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:227: fs_check_call: Calling function "stat" to perform check on "user_dir". sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:247: toctou: Calling function "chown" that uses "user_dir" after a check function. This can cause a time-of-check, time-of-use race condition. # 245| } # 246| # 247|-> ret = chown(user_dir, uid, gid); # 248| if (ret != EOK) { # 249| ret = errno;
Error: TOCTOU (CWE-367): sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:687: fs_check_call: Calling function "stat" to perform check on "filepath". sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:702: toctou: Calling function "remove" that uses "filepath" after a check function. This can cause a time-of-check, time-of-use race condition. # 700| } # 701| # 702|-> ret = remove(filepath); # 703| if (ret != EOK) { # 704| ret = errno;
Error: RESOURCE_LEAK (CWE-772): sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:659: alloc_fn: Storage is returned from allocation function "opendir". sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:659: var_assign: Assigning: "dir" = storage returned from "opendir(user_dir)". sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:680: noescape: Resource "dir" is not freed or pointed-to in "readdir". sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:725: leaked_storage: Variable "dir" going out of scope leaks the storage it points to. # 723| done: # 724| talloc_free(tmp_ctx); # 725|-> return ret; # 726| } # 727|
Error: UNINIT (CWE-457): sssd-1.15.4/src/responder/common/cache_req/cache_req_sr_overlay.c:122: var_decl: Declaring variable "ret" without initializer. sssd-1.15.4/src/responder/common/cache_req/cache_req_sr_overlay.c:209: uninit_use: Using uninitialized value "ret". # 207| done: # 208| talloc_zfree(tmp_ctx); # 209|-> return ret; # 210| } # 211| ``` """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-319315814
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ In general, to get rid of the TOCTOU issues, the key is using the `*at` and `*fd` family of functions - you might open the top level directory to get a fd and then instead of `stat` call `fstat`. You can see how we deal with that in `src/tools/files.c`. """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-319316366
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ With an old server that has no support for the desktop profiles, I'm getting: ``` (Tue Aug 1 10:19:17 2017) [sssd[be[ipa.test]]] [ipa_deskprofile_get_config_done] (0x0040): Unexpected number of results, expected 1, got 0. (Tue Aug 1 10:19:17 2017) [sssd[be[ipa.test]]] [ipa_pam_session_handler_done] (0x0020): Unable to fetch Desktop Profile rules [22]: Invalid argument ``` """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-319330842
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ sorry, I hit comment too soon. I think we want to handle that more gracefully and not throw an error.
By the way, I'm still not finished with the review of the last patch (the previous ones LGTM), but because I'm running out of time today, I'm already adding the "Changes Requested" keyword. I will continue my review in the evening. """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-319331120
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ @jhrozek, about the issue when connecting to old server that has no support for the desktop profiles ...
In that case we'd always get a reply could as 0. Would be okay, for you, to just return ENOENT in those cases? ipa_pam_session_handler_done() handles well ENOENT :-)
AI: Still have to fix the TUCTOU issues. """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-319394677
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ I think the error condition is not really as much of a code issue as a user-friendliness issue. Admins of older IPA servers would suddendly see an odd message saying that something failed. I think returning ENOENT or a special code is both fine as long as there is just an informational message that the server doesn't support desktop profiles. """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-319449557
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ @jhrozek, a new version of the patchset has been pushed and hopefully all the comments have been addressed.
A few more patches were introduced in this new patchset. """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-320668549
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ There are still some Coverity warnings: ``` Error: NEGATIVE_RETURNS (CWE-394): sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:232: negative_return_fn: Function "sss_create_dir("/var/lib/sss/deskprofile", domain, 493U, getuid(), getgid())" returns a negative number. sssd-1.15.4/src/util/files.c:843:5: var_tested_neg: Variable "ret" is negative. sssd-1.15.4/src/util/files.c:878:5: return_negative_variable: Explicitly returning negative variable "ret". sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:232: var_assign: Assigning: signed variable "ret" = "sss_create_dir". sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:235: negative_returns: "ret" is passed to a parameter that cannot be negative. sssd-1.15.4/src/util/util_errors.c:128:5: neg_sink_parm_call: Passing "error" to "strerror", which cannot accept a negative number. # 233| getuid(), getgid()); # 234| if (ret != EOK) { # 235|-> DEBUG(SSSDBG_TRACE_FUNC, # 236| "Failed to create the directory "%s/%s" that would be used to " # 237| "store the Desktop Profile rules users' directory [%d]: %s\n",
Error: NEGATIVE_RETURNS (CWE-394): sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:250: negative_return_fn: Function "sss_create_dir(domain_dir, shortname, 384U, uid, gid)" returns a negative number. sssd-1.15.4/src/util/files.c:843:5: var_tested_neg: Variable "ret" is negative. sssd-1.15.4/src/util/files.c:878:5: return_negative_variable: Explicitly returning negative variable "ret". sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:250: var_assign: Assigning: signed variable "ret" = "sss_create_dir". sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:252: negative_returns: "ret" is passed to a parameter that cannot be negative. sssd-1.15.4/src/util/util_errors.c:128:5: neg_sink_parm_call: Passing "error" to "strerror", which cannot accept a negative number. # 250| ret = sss_create_dir(domain_dir, shortname, 0600, uid, gid); # 251| if (ret != EOK) { # 252|-> DEBUG(SSSDBG_TRACE_FUNC, # 253| "Failed to create the directory "%s/%s/%s" that would be used " # 254| "to store the Desktop Profile rules for the user "%s" [%d]: " ``` """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-322232061
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ I think to fix the warnings, you just need to `errno=ret` before printing the DEBUG message """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-322233270
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ retest this please """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-322233302
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ @jhrozek, about the coverity warnings, I guess the safest path to take is just use int instead of errno_t in that function where the warning happend (and, of course, change the whole chain ...)
It's still not fixed locally. """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-322265238
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ Hmm. That's actually a bad idea and I'm struggling to see a better solution for this. Suggestions are welcome.
"""
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-322272067
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ Well, I'm struggling to see how would `sss_create_dir` return -1. All control paths either return a directly assigned return value (ENOMEM) or errno. """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-322288785
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ Since most of the comments were minor issues with little impact on functionality, I went ahead and scheduled regression runs. """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-322440673
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ btw the downstream regression test passed (job ID 2005455) """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-322566071
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ I have updated the patch set and, hopefully, all your comments have been addressed.
For a few comments I ended up taking the path I believe is the right one. Let me know whether you need some changes and those will be done according to your answers for the questions I've raised as answers for your comments.
This version of the patches includes support for https://pagure.io/SSSD/sssd/issue/3449 and https://github.com/abbra/freeipa-desktop-profile/issues/3. """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-323014987
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ Thank you for the updates. I think only the `permuts` dereference should be changed to happen after the check.
In the meantime, I've started CI, Coverity and downstream HBAC tests. """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-323043057
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ On Thu, Aug 17, 2017 at 04:25:07AM -0700, Jakub Hrozek wrote:
Thank you for the updates. I think only the `permuts` dereference should be changed to happen after the check.
In the meantime, I've started CI, Coverity and downstream HBAC tests.
All passed by the way. So once the one-liner with the array index check is addressed, I'll be happy about the patches.
"""
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-323077931
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ Patch set updated.
This is the patch that I've squashed: ``` From 97f189d3a4fcbbfe453cc6ba8eab8710ca31c4ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Thu, 17 Aug 2017 13:29:24 +0200 Subject: [PATCH] fixup! DESKPROFILE: Introduce the new IPA session provider
--- src/providers/ipa/ipa_deskprofile_rules_util.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/providers/ipa/ipa_deskprofile_rules_util.c b/src/providers/ipa/ipa_deskprofile_rules_util.c index 12c6492ba..2ece5a1ee 100644 --- a/src/providers/ipa/ipa_deskprofile_rules_util.c +++ b/src/providers/ipa/ipa_deskprofile_rules_util.c @@ -150,7 +150,7 @@ ipa_deskprofile_get_filename_path(TALLOC_CTX *mem_ctx, extension, NULL, }; - const uint8_t *perms = permuts[config_priority - 1]; + const uint8_t *perms; char *result; errno_t ret;
@@ -167,6 +167,8 @@ ipa_deskprofile_get_filename_path(TALLOC_CTX *mem_ctx, goto done; }
+ perms = permuts[config_priority - 1]; + result = talloc_strdup(tmp_ctx, ""); if (result == NULL) { ret = ENOMEM;
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ Patch set updated.
This is the patch that I've squashed: ``` From 97f189d3a4fcbbfe453cc6ba8eab8710ca31c4ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Thu, 17 Aug 2017 13:29:24 +0200 Subject: [PATCH] fixup! DESKPROFILE: Introduce the new IPA session provider
--- src/providers/ipa/ipa_deskprofile_rules_util.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/providers/ipa/ipa_deskprofile_rules_util.c b/src/providers/ipa/ipa_deskprofile_rules_util.c index 12c6492ba..2ece5a1ee 100644 --- a/src/providers/ipa/ipa_deskprofile_rules_util.c +++ b/src/providers/ipa/ipa_deskprofile_rules_util.c @@ -150,7 +150,7 @@ ipa_deskprofile_get_filename_path(TALLOC_CTX *mem_ctx, extension, NULL, }; - const uint8_t *perms = permuts[config_priority - 1]; + const uint8_t *perms; char *result; errno_t ret;
@@ -167,6 +167,8 @@ ipa_deskprofile_get_filename_path(TALLOC_CTX *mem_ctx, goto done; }
+ perms = permuts[config_priority - 1]; + result = talloc_strdup(tmp_ctx, ""); if (result == NULL) { ret = ENOMEM;
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ Thank you, code-wise ACK. I'm not going to add the accepted label so that the patches don't get pushed by accident until we hear from the desktop group about results of their testing. Thank you for accomodating all my comments, looking forward to merging the patches! """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-323112506
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ @jhrozek,
I've updated the patch set including **DESKPROFILE: Add ipa_deskprofile_request_interval**, which fix for https://pagure.io/SSSD/sssd/issue/3482 """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-323209189
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ @jhrozek,
I've updated the patch set including **DESKPROFILE: Add ipa_deskprofile_request_interval**, which is a fix for https://pagure.io/SSSD/sssd/issue/3482 """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-323209189
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
pbrezina commented: """ I just briefly read the code, haven't tried it.
```c @@ -126,10 +126,9 @@ static void choose_target(struct data_provider *provider, name = "PAM Chpass 2nd"; break; case SSS_PAM_OPEN_SESSION: - target = DP_TARGET_SENTINEL; - method = DP_METHOD_SENTINEL; + target = DPT_SESSION; + method = DPM_SESSION_HANDLER; name = "PAM Open Session"; - pd->pam_status = PAM_SUCCESS; break; ... /* Check that target is configured. */ if (target != DP_TARGET_SENTINEL && !dp_target_enabled(provider, NULL, target)) { target = DP_TARGET_SENTINEL; method = DP_METHOD_SENTINEL; pd->pam_status = PAM_MODULE_UNKNOWN; } ```
What happens here if session provider is not set? With this patch we return `PAM_MODULE_UNKNOWN` but it was `PAM_SUCCESS` before. Doesn't this cause any kind of problem? """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-323305584
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ On Fri, Aug 18, 2017 at 09:26:14AM +0000, Pavel Březina wrote:
I just briefly read the code, haven't tried it.
@@ -126,10 +126,9 @@ static void choose_target(struct data_provider *provider, name = "PAM Chpass 2nd"; break; case SSS_PAM_OPEN_SESSION: - target = DP_TARGET_SENTINEL; - method = DP_METHOD_SENTINEL; + target = DPT_SESSION; + method = DPM_SESSION_HANDLER; name = "PAM Open Session"; - pd->pam_status = PAM_SUCCESS; break; ... /* Check that target is configured. */ if (target != DP_TARGET_SENTINEL && !dp_target_enabled(provider, NULL, target)) { target = DP_TARGET_SENTINEL; method = DP_METHOD_SENTINEL; pd->pam_status = PAM_MODULE_UNKNOWN; }
What happens here if session provider is not set? With this patch we return `PAM_MODULE_UNKNOWN` but it was `PAM_SUCCESS` before. Doesn't this cause any kind of problem?
I don't follow. The IPA session provider is enabled by default (all providers except access are derived from the id_provider).
Setting session_provider = none gives me: ``` (Fri Aug 18 10:21:17 2017) [sssd[be[ipa.test]]] [dp_req_reply_gen_error] (0x0080): DP Request [PAM Open Session #13]: Finished. Target is not supported with this configuration. pam_dp_send_req returned 0 (Fri Aug 18 10:21:17 2017) [sssd[pam]] [pam_dp_process_reply] (0x0010): Reply error. (Fri Aug 18 10:21:17 2017) [sssd[pam]] [filter_responses] (0x0100): [pam_response_filter] not available, not fatal. ```
Which, except the reply error is also expected, right?
"""
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-323317998
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ @jhrozek: @pbrezina told me (on #sssd) to test with `ipa_enable_deskprofile = false`.
Here's the result: ``` (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [dp_pam_handler] (0x0100): Got request with the following data (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): command: SSS_PAM_OPEN_SESSION (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): domain: ipa.example (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): user: admin@ipa.example (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): service: su-l (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): tty: pts/0 (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): ruser: root (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): rhost: (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): authtok type: 0 (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): newauthtok type: 0 (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): priv: 1 (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): cli_pid: 2324 (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): logon name: not set (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [dp_attach_req] (0x0400): DP Request [PAM Open Session #7]: New request. Flags [0000]. (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [dp_attach_req] (0x0400): Number of active DP request: 1 (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [sss_domain_get_state] (0x1000): Domain ipa.example is Active (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [dp_find_method] (0x0020): Bug: Invalid combination of target [session] and method [9] (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [_dp_req_recv] (0x0400): DP Request [PAM Open Session #7]: Receiving request data. (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [dp_req_destructor] (0x0400): DP Request [PAM Open Session #7]: Request removed. (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [dp_req_destructor] (0x0400): Number of active DP request: 0 (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [dp_req_reply_gen_error] (0x0020): DP Request [PAM Open Session #7]: Finished. Error [1432158209]: Internal Error (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [sbus_dispatch] (0x4000): dbus conn: 0x560901ec30b0 (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [sbus_dispatch] (0x4000): Dispatching. (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [sbus_message_handler] (0x2000): Received SBUS method org.freedesktop.sssd.dataprovider.pamHandler on path /org/freedesktop/sssd/dataprovider (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [sbus_get_sender_id_send] (0x2000): Not a sysbus message, quit (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [dp_pam_handler] (0x0100): Got request with the following data (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): command: SSS_PAM_CLOSE_SESSION (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): domain: ipa.example (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): user: admin@ipa.example (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): service: su-l (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): tty: pts/0 (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): ruser: root (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): rhost: (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): authtok type: 0 (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): newauthtok type: 0 (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): priv: 1 (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): cli_pid: 2324 (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [pam_print_data] (0x0100): logon name: not set (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [dp_pam_reply] (0x1000): DP Request [PAM Close Session]: Sending result [0][ipa.example] ```
Seems that we throw an internal error there. I'll fix it and update the patch to be squashed so @pbrezina can review. """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-323318997
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ @pbrezina: So, here's the patch that solves the issue: ``` From ae60eae181c7a3214d76b3ff00d9e431f060bbc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Fri, 18 Aug 2017 19:46:20 +0200 Subject: [PATCH] fixup! DESKPROFILE: Introduce the new IPA session provider
--- src/providers/ipa/ipa_init.c | 9 --------- src/providers/ipa/ipa_session.c | 10 ++++++++++ 2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/providers/ipa/ipa_init.c b/src/providers/ipa/ipa_init.c index b3daa4921..7cae43c06 100644 --- a/src/providers/ipa/ipa_init.c +++ b/src/providers/ipa/ipa_init.c @@ -950,20 +950,11 @@ errno_t sssm_ipa_session_init(TALLOC_CTX *mem_ctx, struct ipa_session_ctx *session_ctx; struct ipa_init_ctx *init_ctx; struct ipa_id_ctx *id_ctx; - bool enabled; errno_t ret;
init_ctx = talloc_get_type(module_data, struct ipa_init_ctx); id_ctx = init_ctx->id_ctx;
- enabled = dp_opt_get_bool(id_ctx->ipa_options->basic, - IPA_ENABLE_DESKPROFILE); - if (!enabled) { - DEBUG(SSSDBG_TRACE_FUNC, "ipa_enable_deskprofile is set to FALSE\n"); - - return EOK; - } - session_ctx = talloc_zero(mem_ctx, struct ipa_session_ctx); if (session_ctx == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero() failed.\n"); diff --git a/src/providers/ipa/ipa_session.c b/src/providers/ipa/ipa_session.c index 8559284c9..800ea665a 100644 --- a/src/providers/ipa/ipa_session.c +++ b/src/providers/ipa/ipa_session.c @@ -485,6 +485,7 @@ ipa_pam_session_handler_send(TALLOC_CTX *mem_ctx, struct tevent_req *req; struct tevent_req *subreq; struct ipa_pam_session_handler_state *state; + bool enabled; errno_t ret;
DEBUG(SSSDBG_TRACE_FUNC, "Retrieving Desktop Profile rules\n"); @@ -500,6 +501,15 @@ ipa_pam_session_handler_send(TALLOC_CTX *mem_ctx, state->be_ctx = params->be_ctx; state->session_ctx = session_ctx;
+ enabled = dp_opt_get_bool(session_ctx->ipa_options, + IPA_ENABLE_DESKPROFILE); + if (!enabled) { + ret = EOK; + DEBUG(SSSDBG_TRACE_FUNC, "ipa_enable_deskprofile is set to FALSE\n"); + state->pd->pam_status = PAM_SUCCESS; + goto done; + } + /* Get all the user info that will be needed in order the delete the * user's deskprofile directory from the disk, create the user's directory, * save the fetched rules to the disk and notify the deskprofile client
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
pbrezina commented: """ I think it is better to set the method only when needed. Try this diff instead of your patch:
```c --- a/src/providers/data_provider/dp_target_auth.c +++ b/src/providers/data_provider/dp_target_auth.c @@ -126,9 +126,16 @@ static void choose_target(struct data_provider *provider, name = "PAM Chpass 2nd"; break; case SSS_PAM_OPEN_SESSION: - target = DPT_SESSION; - method = DPM_SESSION_HANDLER; name = "PAM Open Session"; + if (dp_method_enabled(provider, DPT_SESSION, DPM_SESSION_HANDLER)) { + target = DPT_SESSION; + method = DPM_SESSION_HANDLER; + break; + } + + target = DP_TARGET_SENTINEL; + method = DP_METHOD_SENTINEL; + pd->pam_status = PAM_SUCCESS; break; case SSS_PAM_SETCRED: target = DP_TARGET_SENTINEL;
```
Also, as I read through the patches, I don't see `session_provider` documented anywhere and since `ipa_deskprofile_enable ` defaults to `true` it is basically an equivalent to `session_provider = ipa | not set`. Wouldn't it be better to remove this option and document `session_provider` instead as we do with other providers (e.g. hostid)? """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-323710521
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ On Mon, Aug 21, 2017 at 12:39 PM, Pavel Březina notifications@github.com wrote:
I think it is better to set the method only when needed. Try this diff instead of your patch:
--- a/src/providers/data_provider/dp_target_auth.c +++ b/src/providers/data_provider/dp_target_auth.c @@ -126,9 +126,16 @@ static void choose_target(struct data_provider *provider, name = "PAM Chpass 2nd"; break; case SSS_PAM_OPEN_SESSION:
target = DPT_SESSION;
method = DPM_SESSION_HANDLER; name = "PAM Open Session";
if (dp_method_enabled(provider, DPT_SESSION, DPM_SESSION_HANDLER)) {
target = DPT_SESSION;
method = DPM_SESSION_HANDLER;
break;
}
target = DP_TARGET_SENTINEL;
method = DP_METHOD_SENTINEL;
pd->pam_status = PAM_SUCCESS; break; case SSS_PAM_SETCRED: target = DP_TARGET_SENTINEL;
Also, as I read through the patches, I don't see session_provider documented anywhere and since ipa_deskprofile_enable defaults to true it is basically an equivalent to session_provider = ipa | not set. Wouldn't it be better to remove this option and document session_provider instead as we do with other providers (e.g. hostid)?
I like the idea to have only the session_provider option documented/exposed. @jhrozek, would that be okay for you? Then we can drop the ipa_enable_deskprofile option entirely.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/241#issuecomment-323710521, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG4etDXxe7hxYhC8DpwTR2TzZ2gOM-Oks5saV51gaJpZM4NDNM2 .
"""
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-323711365
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
pbrezina commented: """ Typo in "related": ```xml + <para> + Default: <quote>id_provider</quote> is used if it + is set and can perform session reltted tasks. + </para>
```
This works and it looks good in provider logs but in pam log is a system error which may be confusing. Please add the diff I provided so we can avoid it.
``` 275 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_dp_send_req] (0x0100): Sending request with the following data: 276 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_print_data] (0x0100): command: SSS_PAM_OPEN_SESSION 277 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_print_data] (0x0100): domain: LDAP.PB 278 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_print_data] (0x0100): user: user-1@ldap.pb 279 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_print_data] (0x0100): service: su 280 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_print_data] (0x0100): tty: pts/7 281 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_print_data] (0x0100): ruser: pbrezina 282 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_print_data] (0x0100): rhost: not set 283 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_print_data] (0x0100): authtok type: 0 284 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_print_data] (0x0100): newauthtok type: 0 285 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_print_data] (0x0100): priv: 0 286 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_print_data] (0x0100): cli_pid: 10586 287 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_print_data] (0x0100): logon name: user-1 288 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [sbus_add_timeout] (0x2000): 0xe9e330 289 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_dom_forwarder] (0x0100): pam_dp_send_req returned 0 290 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [sbus_remove_timeout] (0x2000): 0xe9e330 291 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_dp_process_reply] (0x0010): Reply error. 292 (Tue Aug 22 09:59:23 2017) [sssd[pam]] [pam_reply] (0x0200): pam_reply called with result [4]: System error. ``` """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-323951221
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
fidencio commented: """ @pbrezina: patchset has been updated. Thanks for your suggestions! """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-324003707
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
pbrezina commented: """ On 08/22/2017 01:54 PM, fidencio wrote:
@pbrezina https://github.com/pbrezina: patchset has been updated. Thanks for your suggestions!
Ack.
"""
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-324011406
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
olivergs commented: """ Tested the latest build for the patches from @fidencio COPR repo and it worked as expected.
On Thu, Aug 17, 2017 at 4:23 PM, fidencio notifications@github.com wrote:
Patch set updated.
This is the patch that I've squashed:
From 97f189d3a4fcbbfe453cc6ba8eab8710ca31c4ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= fidencio@redhat.com Date: Thu, 17 Aug 2017 13:29:24 +0200 Subject: [PATCH] fixup! DESKPROFILE: Introduce the new IPA session provider
src/providers/ipa/ipa_deskprofile_rules_util.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/providers/ipa/ipa_deskprofile_rules_util.c b/src/providers/ipa/ipa_deskprofile_rules_util.c index 12c6492ba..2ece5a1ee 100644 --- a/src/providers/ipa/ipa_deskprofile_rules_util.c +++ b/src/providers/ipa/ipa_deskprofile_rules_util.c @@ -150,7 +150,7 @@ ipa_deskprofile_get_filename_path(TALLOC_CTX *mem_ctx, extension, NULL, };
- const uint8_t *perms = permuts[config_priority - 1];
- const uint8_t *perms; char *result; errno_t ret;
@@ -167,6 +167,8 @@ ipa_deskprofile_get_filename_path(TALLOC_CTX *mem_ctx, goto done; }
- perms = permuts[config_priority - 1];
- result = talloc_strdup(tmp_ctx, ""); if (result == NULL) { ret = ENOMEM;
-- 2.13.4
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/241#issuecomment-323102642, or mute the thread https://github.com/notifications/unsubscribe-auth/ABX4oQhaAXHRCzP43H7qrQ3aCbpAVR9Zks5sZFhZgaJpZM4NDNM2 .
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
olivergs commented: """ Tested the latest build for the patches from @fidencio COPR repo and it worked as expected. """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-325337915
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ ACK, CI: http://vm-058-233.XXX/logs/job/74/33/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-325391191
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
jhrozek commented: """ * master: 4a311702045b065a97a0c0fc0ccc7a1fc84b38cf 85517b57685809ff96818bbd3e3b4678ac74b461 85a93ca67ae020607006cd035170c9360fb0a450 684a13e8de1526257ca2e40b6bf2e05585d4eaca 4b37ee7d370003514916c793046577ea4b6e736b dd6a4fb9ae4825caf4ccb835f8b8221c96bbb6f5 c9e104f17b6c4cf5741dea9fdbe864619125fab1 9d98e98ab37d86323034e7bc342f196b81fa07bc b054e7d8c43b024ee33e9343b4a15e124861f68c f982039c75ec064894deb676ae53ee57de868590 6f466e0a3d950d21bd750ef53cb93b75dc023f9e 5b93634c7f0e34f69b4cf8fb9b2e77b9179024a7 7c1d1393537dec95e09b83b607ce9d0e8f49584c 18d898d9cb30f298b3a35dc1c1bace95ef4e0b3b ee164913f9c12a557044eb469f4498b9be9a8f50 0f623456437c96f50330fe0ff21afd9638d14e57 d2a0b4a6a220bf9a58c7306c3f673891efc419eb e17e37cd0e2109e7f1bd4ae48edfc8cca85b3f93 21909d3b620d97e81dd946b959a47efe88d2b7d8 9a18f78f38e274f4906af6ef8e1a82d844fde4cc 8a26d32bc9b71e85a42b7832891100a7249f92aa
"""
See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-325442601
URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/241 Author: fidencio Title: #241: FleetCommander Integration Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/241/head:pr241 git checkout pr241
sssd-devel@lists.fedorahosted.org