[SSSD] [sssd PR#137][comment] Initial pkinit support
URL: https://github.com/SSSD/sssd/pull/137 Title: #137: Initial pkinit support jhrozek commented: """ master: 2d527aab0bab0c5323b7ea09c9a8c3820f4f8736 52f45837ded98564968da42229b37db6a36ad627 ead25e32c52c8c2f5fd9abd179e9e81de58f9ca3 82c5971fafe6063a90289ebba08035fc49ae8590 dd17a3aaddab6f122dff3bd15b7005464c07c0ea f70d946f8cde55b6bdc09345e22849842bca4387 d4757440418c7b73bbecec7e40baf6dfe8cc9460 254f3898cc9fb9d76e12d72a2955906c49748e6d 327a16652bbafbb77b5b90cc7abac3ded7c14364 f561c2bd3c72631ccb7ad6d0b5f6541b27b0922d """ See the full comment at https://github.com/SSSD/sssd/pull/137#issuecomment-281937886 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#137][comment] Initial pkinit support
URL: https://github.com/SSSD/sssd/pull/137 Title: #137: Initial pkinit support jhrozek commented: """ On Wed, Feb 22, 2017 at 09:29:02AM -0800, sumit-bose wrote: > Thank you for the rigid review, Since the review took so long, it better be good :-) > I've fixed the comments move some strucht members to a previous patch to not > break the individual compilation. All my comments were addressed and the patches still work fine. ACK. """ See the full comment at https://github.com/SSSD/sssd/pull/137#issuecomment-281937150 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#137][comment] Initial pkinit support
URL: https://github.com/SSSD/sssd/pull/137 Title: #137: Initial pkinit support sumit-bose commented: """ Thank you for the rigid review, I've fixed the comments move some strucht members to a previous patch to not break the individual compilation. """ See the full comment at https://github.com/SSSD/sssd/pull/137#issuecomment-281740575 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#137][comment] Initial pkinit support
URL: https://github.com/SSSD/sssd/pull/137 Title: #137: Initial pkinit support jhrozek commented: """ The patches work now, if the three small issues above and the individual compilation are addressed, I'll ack """ See the full comment at https://github.com/SSSD/sssd/pull/137#issuecomment-281685683 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#137][comment] Initial pkinit support
URL: https://github.com/SSSD/sssd/pull/137 Title: #137: Initial pkinit support sumit-bose commented: """ Thank you for review and testing. I fixed the issue with changing the expired password and reordered the patches so that sss_authtok_set_sc is defined before it is used. """ See the full comment at https://github.com/SSSD/sssd/pull/137#issuecomment-281303524 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#137][comment] Initial pkinit support
URL: https://github.com/SSSD/sssd/pull/137 Title: #137: Initial pkinit support jhrozek commented: """ OK, apart from the issue with the patch compilation, I found one more with manual testing -- it looks like changing the expired password of a newly created IPA user is not working correctly. I'm getting: ``` (Mon Feb 20 20:54:03 2017) [[sssd[krb5_child[1798 [sss_child_krb5_trace_cb] (0x4000): [1798] 1487624043.229515: Received error from KDC: -1765328361/Password has expired (Mon Feb 20 20:54:03 2017) [[sssd[krb5_child[1798 [get_and_save_tgt] (0x0020): 1526: [-1765328361][Password has expired] (Mon Feb 20 20:54:03 2017) [[sssd[krb5_child[1798 [map_krb5_error] (0x0020): [1432158285][No authentication methode available]. (Mon Feb 20 20:54:03 2017) [[sssd[krb5_child[1798 [k5c_send_data] (0x0200): Received error code 1432158285 (Mon Feb 20 20:54:03 2017) [[sssd[krb5_child[1798 [pack_response_packet] (0x2000): response packet size: [4] (Mon Feb 20 20:54:03 2017) [[sssd[krb5_child[1798 [k5c_send_data] (0x4000): Response sent. (Mon Feb 20 20:54:03 2017) [[sssd[krb5_child[1798 [main] (0x0400): krb5_child completed successfully (Mon Feb 20 20:54:03 2017) [sssd[be[ipa.test]]] [read_pipe_handler] (0x0400): EOF received, client finished (Mon Feb 20 20:54:03 2017) [sssd[be[ipa.test]]] [check_wait_queue] (0x1000): Wait queue for user [authte...@ipa.test] is empty. (Mon Feb 20 20:54:03 2017) [sssd[be[ipa.test]]] [krb5_auth_queue_done] (0x1000): krb5_auth_queue request [0x19ccf00] done. (Mon Feb 20 20:54:03 2017) [sssd[be[ipa.test]]] [dp_req_done] (0x0400): DP Request [PAM Authenticate #8]: Request handler finished [0]: Success (Mon Feb 20 20:54:03 2017) [sssd[be[ipa.test]]] [_dp_req_recv] (0x0400): DP Request [PAM Authenticate #8]: Receiving request data. (Mon Feb 20 20:54:03 2017) [sssd[be[ipa.test]]] [dp_req_destructor] (0x0400): DP Request [PAM Authenticate #8]: Request removed. (Mon Feb 20 20:54:03 2017) [sssd[be[ipa.test]]] [dp_req_destructor] (0x0400): Number of active DP request: 0 (Mon Feb 20 20:54:03 2017) [sssd[be[ipa.test]]] [dp_pam_reply] (0x1000): DP Request [PAM Authenticate #8]: Sending result [18][ipa.test] ``` This works fine with the current master. Apart from that, I ran downstream tests for AD, LDAP/LDAP and LDAP/KRB5. Manual testing included: * IPA auth with a password, online and offline * IPA auth with OTP, online and offline * AD auth, AD auth with a UPN * subdomain auth * IPA password change The code looks mostly good, I will make another pass on it tomorrow, but I suppose if I even ask for anything, it would be comments or so. """ See the full comment at https://github.com/SSSD/sssd/pull/137#issuecomment-281179465 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#137][comment] Initial pkinit support
URL: https://github.com/SSSD/sssd/pull/137 Title: #137: Initial pkinit support jhrozek commented: """ There is a small issue where `sss_authtok_set_sc` is used before it's defined, which would break bisect. btw I'm battling a bit with the downstream tests, it looks like saying these patches break renewals wasn't right, because there is some issue with tests that makes the renewal downstream tests fail even with the current master. So I'm re-running the tests more or less one by one and inspecting the debug messages to see if we have more failures with the patches than with master. I'm also running some totally manual tests.. I will post more issues (if I see any) here, but in the meantime, I spotted only the function-used-before-defined one. """ See the full comment at https://github.com/SSSD/sssd/pull/137#issuecomment-281045334 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#137][comment] Initial pkinit support
URL: https://github.com/SSSD/sssd/pull/137 Title: #137: Initial pkinit support sumit-bose commented: """ Hi Lukas, thank you for your comments. I move get_pkinit_identity() out if the ifdef block. For pam_check_user_done() I removed the assignment which should hopefully silence the Coverity warning. Since we use pam_check_user_done() without assigning the return code to a variable at other places as well I hope you agree with this. I think we can even make pam_check_user_done() to return void but this should be handled by a different patch. bye, Sumit """ See the full comment at https://github.com/SSSD/sssd/pull/137#issuecomment-280277210 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#137][comment] Initial pkinit support
URL: https://github.com/SSSD/sssd/pull/137 Title: #137: Initial pkinit support lslebodn commented: """ On (09/02/17 02:50), sumit-bose wrote: >Thank you for running the tests, the valgirind issue was the same as the last >coverity warning. > The function get_pkinit_identity is defined inside following ifdef `#ifdef HAVE_KRB5_GET_INIT_CREDS_OPT_SET_RESPONDER` but it is used without this `ifdef`. This feature is not available on el6 and therefore there is a compilation error: ``` src/providers/krb5/krb5_child.c: In function 'get_and_save_tgt': src/providers/krb5/krb5_child.c:1493: error: implicit declaration of function 'get_pkinit_identity' src/providers/krb5/krb5_child.c: In function 'changepw_child': ``` There is also a warning: ``` Error: UNUSED_VALUE (CWE-563): [#def6] sssd-1.15.1/src/responder/pam/pamsrv_cmd.c:742: returned_value: Assigning value from "pam_check_user_done(preq, ret)" to "ret" here, but that stored value is not used. # 740| preq->cert_auth_local = true; # 741| ret = check_cert(cctx, cctx->ev, pctx, preq, pd); # 742|-> ret = pam_check_user_done(preq, ret); # 743| return; # 744| } ``` We should at least log a message. LS """ See the full comment at https://github.com/SSSD/sssd/pull/137#issuecomment-278978626 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#137][comment] Initial pkinit support
URL: https://github.com/SSSD/sssd/pull/137 Title: #137: Initial pkinit support sumit-bose commented: """ Thank you for running the tests, the valgirind issue was the same as the last coverity warning. """ See the full comment at https://github.com/SSSD/sssd/pull/137#issuecomment-278608541 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#137][comment] Initial pkinit support
URL: https://github.com/SSSD/sssd/pull/137 Title: #137: Initial pkinit support jhrozek commented: """ pam_srv_test doesn't like being run under valgrind: http://sssd-ci.duckdns.org/logs/job/61/71/rhel7/ci-build-debug/ci-make-check-valgrind.log """ See the full comment at https://github.com/SSSD/sssd/pull/137#issuecomment-277782987 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#137][comment] Initial pkinit support
URL: https://github.com/SSSD/sssd/pull/137
Title: #137: Initial pkinit support
jhrozek commented:
"""
There are some Coverity warnings:
```
Error: UNINIT (CWE-457):
sssd-1.15.1/src/p11_child/p11_child_nss.c:112: var_decl: Declaring variable
"key_id_str" without initializer.
sssd-1.15.1/src/p11_child/p11_child_nss.c:482: uninit_use_in_call: Using
uninitialized value "key_id_str" when calling "PORT_Free".
# 480|
# 481| SECITEM_FreeItem(key_id, PR_TRUE);
# 482|-> PORT_Free(key_id_str);
# 483|
# 484| PORT_Free(signed_random_value.data);
Error: COMPILER_WARNING:
sssd-1.15.1/src/p11_child/p11_child_nss.c: scope_hint: In function 'do_work'
sssd-1.15.1/src/p11_child/p11_child_nss.c:482:5: warning: 'key_id_str' may be
used uninitialized in this function [-Wmaybe-uninitialized]
# PORT_Free(key_id_str);
# ^
# 480|
# 481| SECITEM_FreeItem(key_id, PR_TRUE);
# 482|-> PORT_Free(key_id_str);
# 483|
# 484| PORT_Free(signed_random_value.data);
Error: NEGATIVE_RETURNS (CWE-394):
sssd-1.15.1/src/providers/krb5/krb5_child.c:1836: negative_return_fn: Function
"get_and_save_tgt(kr, newpassword)" returns a negative number.
sssd-1.15.1/src/providers/krb5/krb5_child.c:1484:9: return_negative_constant:
Explicitly returning negative value "-1765328324".
sssd-1.15.1/src/providers/krb5/krb5_child.c:1836: var_assign: Assigning: signed
variable "kerr" = "get_and_save_tgt".
sssd-1.15.1/src/providers/krb5/krb5_child.c:1844: negative_returns: "kerr" is
passed to a parameter that cannot be negative.
sssd-1.15.1/src/providers/krb5/krb5_child.c:1603:9: neg_sink_parm_call: Passing
"kerr" to "sss_strerror", which cannot accept a negative number.
sssd-1.15.1/src/util/util_errors.c:117:5: neg_sink_parm_call: Passing "error"
to "strerror", which cannot accept a negative number.
# 1842| kerr = k5c_attach_ccname_msg(kr);
# 1843| }
# 1844|-> return map_krb5_error(kerr);
# 1845| }
# 1846|
Error: NEGATIVE_RETURNS (CWE-394):
sssd-1.15.1/src/providers/krb5/krb5_child.c:1878: negative_return_fn: Function
"get_and_save_tgt(kr, password)" returns a negative number.
sssd-1.15.1/src/providers/krb5/krb5_child.c:1484:9: return_negative_constant:
Explicitly returning negative value "-1765328324".
sssd-1.15.1/src/providers/krb5/krb5_child.c:1878: var_assign: Assigning: signed
variable "kerr" = "get_and_save_tgt".
sssd-1.15.1/src/providers/krb5/krb5_child.c:1913: negative_returns: "kerr" is
passed to a parameter that cannot be negative.
sssd-1.15.1/src/providers/krb5/krb5_child.c:1603:9: neg_sink_parm_call: Passing
"kerr" to "sss_strerror", which cannot accept a negative number.
sssd-1.15.1/src/util/util_errors.c:117:5: neg_sink_parm_call: Passing "error"
to "strerror", which cannot accept a negative number.
# 1911| }
# 1912| }
# 1913|-> ret = map_krb5_error(kerr);
# 1914| goto done;
# 1915| }
Error: UNUSED_VALUE (CWE-563):
sssd-1.15.1/src/responder/pam/pamsrv_cmd.c:1505: value_overwrite: Overwriting
previous write to "ret" with value from "pam_check_user_search(preq)".
sssd-1.15.1/src/responder/pam/pamsrv_cmd.c:1507: value_overwrite: Overwriting
previous write to "ret" with value "0".
sssd-1.15.1/src/responder/pam/pamsrv_cmd.c:1488: returned_value: Assigning
value from "sss_parse_name_for_domains(preq->pd, preq->cctx->rctx->domains,
preq->cctx->rctx->default_domain, cert_user, &preq->pd->domain,
&preq->pd->user)" to "ret" here, but that stored value is overwritten before it
can be used.
# 1486| cert_user);
# 1487|
# 1488|-> ret = sss_parse_name_for_domains(preq->pd,
# 1489|
preq->cctx->rctx->domains,
# 1490|
preq->cctx->rctx->default_domain,
Error: UNINIT (CWE-457):
sssd-1.15.1/src/util/authtok.c:597: var_decl: Declaring variable "key_id_len"
without initializer.
sssd-1.15.1/src/util/authtok.c:665: uninit_use: Using uninitialized value
"key_id_len".
# 663| }
# 664|
# 665|-> if (key_id_len != 0) {
# 666| *key_id = talloc_strndup(mem_ctx,
# 667| (const char *) blob + c +
pin_len
Error: COMPILER_WARNING:
sssd-1.15.1/src/util/authtok.c: scope_hint: In function
'sss_auth_unpack_sc_blob'
sssd-1.15.1/src/util/authtok.c:666:19: warning: 'key_id_len' may be used
uninitialized in this function [-Wmaybe-uninitialized]
# *key_id = talloc_strndup(mem_ctx,
# ^
# 664|
# 665| if (key_id_len != 0) {
# 666|-> *key_id = talloc_strndup(mem_ctx,
# 667| (const char *) blob + c +
pin_len
# 668| +
token_name_len
```
"""
See the full comment at
https://github.com/SSSD/sssd/pull/137#issuecomment-25771
___
sssd
