[sssd PR#433][comment] PAM: Multiple certificates on a Smartcard
by sumit-bose
URL: https://github.com/SSSD/sssd/pull/433
Title: #433: PAM: Multiple certificates on a Smartcard
sumit-bose commented:
"""
@fidencio, thank you for the review. I used '0' instead of EOK here on purpose. Although in the SSSD source tree the sss_certmap_* calls are coming from an external library and the return code for success is '0' as documented in src/lib/certmap/sss_certmap.h.
EOK is used by SSSD's internal calls and is currently defined as '0'. But in theory it should be possible to set this to any int value and SSSD should keep working. I pretty sure this would currently break heavily but that why I would prefer to not use EOK for calls from external libraries.
"""
See the full comment at https://github.com/SSSD/sssd/pull/433#issuecomment-342808294
6 years, 5 months
[sssd PR#433][comment] PAM: Multiple certificates on a Smartcard
by fidencio
URL: https://github.com/SSSD/sssd/pull/433
Title: #433: PAM: Multiple certificates on a Smartcard
fidencio commented:
"""
So, code-wise I have just a few nitpicks related to the new patches. Feel free to pick up the changes or ignore them if you don't think they're valid:
```
[ffidenci@pessoa sssd]$ cat 0001-fixup-pam-filter-certificates-in-the-responder-not-i.patch
From b881194ae06e4e835f84d5bfbdf2924e5b2b7b15 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com>
Date: Wed, 8 Nov 2017 12:26:23 +0100
Subject: [PATCH] fixup! pam: filter certificates in the responder not in the
child
---
src/responder/pam/pamsrv_p11.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c
index 6386954ae..c2eccb1f3 100644
--- a/src/responder/pam/pamsrv_p11.c
+++ b/src/responder/pam/pamsrv_p11.c
@@ -157,7 +157,7 @@ errno_t p11_refresh_certmap_ctx(struct pam_ctx *pctx,
/* Try to add default matching rule */
ret = sss_certmap_add_rule(sss_certmap_ctx, SSS_CERTMAP_MIN_PRIO,
CERT_AUTH_DEFAULT_MATCHING_RULE, NULL, NULL);
- if (ret != 0) {
+ if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"Failed to add default matching rule.\n");
}
@@ -176,7 +176,7 @@ errno_t p11_refresh_certmap_ctx(struct pam_ctx *pctx,
certmap_list[c]->match_rule,
certmap_list[c]->map_rule,
certmap_list[c]->domains);
- if (ret != 0) {
+ if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
"sss_certmap_add_rule failed for rule [%s] "
"with error [%d][%s], skipping. "
@@ -463,7 +463,7 @@ static errno_t parse_p11_child_response(TALLOC_CTX *mem_ctx, uint8_t *buf,
}
ret = sss_certmap_match_cert(sss_certmap_ctx, der, der_size);
- if (ret == 0) {
+ if (ret == EOK) {
DLIST_ADD(cert_list, cert_auth_info);
} else {
DEBUG(SSSDBG_TRACE_LIBS,
--
2.13.6
```
Covscan came up without any issues and I'm just waiting for the results of the downstream tests to ACK the patches.
"""
See the full comment at https://github.com/SSSD/sssd/pull/433#issuecomment-342803822
6 years, 5 months