Initial pkinit support #137

sumit-bose · 2017-01-25T20:09:12Z

This series of patches add initial support for PKINIT
(https://fedorahosted.org/sssd/ticket/3270) by forwarding the information about
the selected certificate from the Smartcard to the backends. Currently only
the krb5 backend supports Smartcard authentication the other backends will
return an error code which will tell the PAM responder to fall back to local
Smartcard authentication as it is currently the case.

Testing requires a working PKINIT setup which e.g. can be done with AD by
setting up a CA and generating certificates as described in
https://fedorahosted.org/sssd/wiki/DesignDocs/SmartcardAuthenticationTestingWithAD.

But currently more important is regression testing, i.e. making sure all other
authentication methods are still working as expected.

jhrozek · 2017-02-06T18:51:35Z

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

jhrozek · 2017-02-06T19:17:32Z

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

sumit-bose · 2017-02-09T10:50:18Z

Thank you for running the tests, the valgirind issue was the same as the last coverity warning.

lslebodn · 2017-02-10T15:43:26Z

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

sumit-bose · 2017-02-16T09:20:46Z

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

jhrozek · 2017-02-20T10:41:22Z

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.

jhrozek · 2017-02-20T20:56:38Z

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 [authtest2@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.

sumit-bose · 2017-02-21T10:25:27Z

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.

jhrozek · 2017-02-22T14:32:36Z

src/util/authtok-utils.c

+
+    /* A missing pin is ok in the case of a reader with a keyboard */
+    if (pin == NULL) {
+        pin = "";


it would be nice to set the pin_len to strlen(pin) here to avoid issues if someone passed in NULL pin but non-zero pin_len. Same with other input strings.

jhrozek · 2017-02-22T14:33:09Z

src/sss_client/pam_sss.c

+                                    &needed_size);
+        if (ret != EOK) {
+            D(("sss_auth_pack_sc_blob failed."));
+            ret = PAM_BUF_ERR;


looks like pi->pam_authtok is not freed here

jhrozek · 2017-02-22T14:33:33Z

src/sss_client/pam_sss.c

+
+        if (pi->pam_authtok == NULL) {
+            ret = PAM_BUF_ERR;
+            goto done;


this block looks like it belongs before sss_auth_pack_sc_blob and after malloc

jhrozek · 2017-02-22T14:34:14Z

The patches work now, if the three small issues above and the individual compilation are addressed, I'll ack

Since there can be multiple rounds trips between the PAM client and SSSD it might be possible that the same data is send multiple times by SSSD. So before overriding the old data it should be freed. I've seen this with the domain name which is send both in the pre-auth and the auth responses. To be on the safe side I added free() for some other items as well.

ERR_SC_AUTH_NOT_SUPPORTED can be used by backends to indicate that Smartcard authentication is not supported. ERR_NO_AUTH_METHOD_AVAILABLE can be used by backends that no authentication method was found.

The blobs contains beside the PIN the name of the PKCS#11 module and the token name where the certificate of the user was found and the key id. Those data will be used e.g. by the pkinit module to make sure them right certificate is used.

sumit-bose · 2017-02-22T17:29:01Z

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.

jhrozek · 2017-02-23T09:14:13Z

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.

jhrozek · 2017-02-23T09:17:11Z

master:
2d527aa
52f4583
ead25e3
82c5971
dd17a3a
f70d946
d475744
254f389
327a166
f561c2b

jhrozek self-assigned this Feb 6, 2017

sumit-bose force-pushed the add_initial_pkinit_support branch from 48e5be1 to 4bc0525 Compare February 9, 2017 10:48

sumit-bose force-pushed the add_initial_pkinit_support branch from 4bc0525 to 2e1925b Compare February 16, 2017 09:16

jhrozek added the Changes requested label Feb 20, 2017

sumit-bose force-pushed the add_initial_pkinit_support branch from 2e1925b to 545dbaf Compare February 21, 2017 10:23

sumit-bose removed the Changes requested label Feb 21, 2017

jhrozek reviewed Feb 22, 2017

View reviewed changes

jhrozek suggested changes Feb 22, 2017

View reviewed changes

jhrozek added the Changes requested label Feb 22, 2017

sumit-bose added 10 commits February 22, 2017 16:46

PAM: store user object in the preq context

0c0b210

PAM: use sentinel error code in PAM tests

89022ab

utils: new error codes

0e70743

ERR_SC_AUTH_NOT_SUPPORTED can be used by backends to indicate that Smartcard authentication is not supported. ERR_NO_AUTH_METHOD_AVAILABLE can be used by backends that no authentication method was found.

LDAP/proxy: tell frontend that Smartcard auth is not supported

a21615e

PAM: forward Smartcard credentials to backends

f909dac

p11: return name of PKCS#11 module and key id to pam_sss

5973789

pam: enhance Smartcard authentication token

c3e3639

KRB5: allow pkinit pre-authentication

24e5ec2

sumit-bose force-pushed the add_initial_pkinit_support branch from 545dbaf to 24e5ec2 Compare February 22, 2017 17:27

sumit-bose removed the Changes requested label Feb 22, 2017

jhrozek added the Accepted label Feb 23, 2017

jhrozek closed this Feb 23, 2017

jhrozek added Pushed and removed Accepted labels Feb 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial pkinit support #137

Initial pkinit support #137

sumit-bose commented Jan 25, 2017

jhrozek commented Feb 6, 2017

jhrozek commented Feb 6, 2017

sumit-bose commented Feb 9, 2017

lslebodn commented Feb 10, 2017 via email

sumit-bose commented Feb 16, 2017

jhrozek commented Feb 20, 2017

jhrozek commented Feb 20, 2017

sumit-bose commented Feb 21, 2017

jhrozek Feb 22, 2017

jhrozek Feb 22, 2017

jhrozek Feb 22, 2017

jhrozek commented Feb 22, 2017

sumit-bose commented Feb 22, 2017

jhrozek commented Feb 23, 2017 via email

jhrozek commented Feb 23, 2017

Initial pkinit support #137

Initial pkinit support #137

Conversation

sumit-bose commented Jan 25, 2017

jhrozek commented Feb 6, 2017

jhrozek commented Feb 6, 2017

sumit-bose commented Feb 9, 2017

lslebodn commented Feb 10, 2017 via email

sumit-bose commented Feb 16, 2017

jhrozek commented Feb 20, 2017

jhrozek commented Feb 20, 2017

sumit-bose commented Feb 21, 2017

jhrozek Feb 22, 2017

Choose a reason for hiding this comment

jhrozek Feb 22, 2017

Choose a reason for hiding this comment

jhrozek Feb 22, 2017

Choose a reason for hiding this comment

jhrozek commented Feb 22, 2017

sumit-bose commented Feb 22, 2017

jhrozek commented Feb 23, 2017 via email

jhrozek commented Feb 23, 2017