Hi,
this is the initial version of my patch which add Smartcard authentication to SSSD. I'm still working on a design page which will explain everything in more details so I will only add a short version here.
The main job will be done by a new child process called p11_child. Since the Smartcard support in GDM is based on NSS I used NSS for the first version of p11_child as well. But since all PKCS#11 (API to talk to Smartcards) related code is in this child process adding support for other PKCS#11 frameworks like p11-kit would be straight forward (in fact I already started on the p11-kit version). Using NSS here means you have to add the PKCS#11 module for your Smartcards reader to /etc/pki/nssdb (the NSS DB GDM uses as well) with modutil or pk11install from the coolkey package.
The PAM configuration so far must not be changed. pam_sss will do a pre-auth request similar to the OPT case for find a suitable authentication method for the user. The pam responder then checks is Smartcard authentication is enabled (pam_cert_auth = True in the [pam] section of sssd.conf), if the service is a local one and if there if a valid certificate can be found which is available in the users LDAP entry as well. If all this checks pass pam_sss will ask the user for a PIN and then SSSD tries to validate that PIN, public and private keys all relate to each other. If no Smartcard is found for the user the standard password prompt is displayed.
With some valuable input form Christian Heimes I think I found a way to test the Smartcard support even without real hardware but I still have to work out some of the details. I will add instructions to the design page and better and more unit tests.
Any comments and suggestions are welcome.
bye, Sumit
On Fri, Jul 10, 2015 at 06:40:30PM +0200, Sumit Bose wrote:
Hi,
this is the initial version of my patch which add Smartcard authentication to SSSD. I'm still working on a design page which will explain everything in more details so I will only add a short version here.
The main job will be done by a new child process called p11_child. Since the Smartcard support in GDM is based on NSS I used NSS for the first version of p11_child as well. But since all PKCS#11 (API to talk to Smartcards) related code is in this child process adding support for other PKCS#11 frameworks like p11-kit would be straight forward (in fact I already started on the p11-kit version). Using NSS here means you have to add the PKCS#11 module for your Smartcards reader to /etc/pki/nssdb (the NSS DB GDM uses as well) with modutil or pk11install from the coolkey package.
The PAM configuration so far must not be changed. pam_sss will do a pre-auth request similar to the OPT case for find a suitable authentication method for the user. The pam responder then checks is Smartcard authentication is enabled (pam_cert_auth = True in the [pam] section of sssd.conf), if the service is a local one and if there if a valid certificate can be found which is available in the users LDAP entry as well. If all this checks pass pam_sss will ask the user for a PIN and then SSSD tries to validate that PIN, public and private keys all relate to each other. If no Smartcard is found for the user the standard password prompt is displayed.
With some valuable input form Christian Heimes I think I found a way to test the Smartcard support even without real hardware but I still have to work out some of the details. I will add instructions to the design page and better and more unit tests.
Any comments and suggestions are welcome.
Please find attached an improved version of the patches. Especially there are improvements to the test, the return values from the p11_child are not mocked used the actual retrieved certificate data. The test data causes some increase in the patch size. I plan to replace this with data which is generate during the test run but for a start it is easier this way.
I also added a 7th patch which should resolve https://fedorahosted.org/sssd/ticket/2711 (SSH with certificates). Strictly it is not related to Smartcard authentication via PAM but it depends on the NSS version of the cert utilities (patch 0001) so I included it here as well.
On the design page I added the 'How to test' section https://fedorahosted.org/sssd/wiki/DesignDocs/SmartcardAuthenticationStep1#H... which hopefully gives sufficient details how to set up a test environment.
bye, Sumit
----- Original Message -----
From: "Sumit Bose" sbose@redhat.com To: sssd-devel@lists.fedorahosted.org Sent: Friday, 17 July, 2015 9:01:49 PM Subject: Re: [SSSD] [PATCHES] Add support for Smartcard authentication
On Fri, Jul 10, 2015 at 06:40:30PM +0200, Sumit Bose wrote:
Hi,
this is the initial version of my patch which add Smartcard authentication to SSSD. I'm still working on a design page which will explain everything in more details so I will only add a short version here.
The main job will be done by a new child process called p11_child. Since the Smartcard support in GDM is based on NSS I used NSS for the first version of p11_child as well. But since all PKCS#11 (API to talk to Smartcards) related code is in this child process adding support for other PKCS#11 frameworks like p11-kit would be straight forward (in fact I already started on the p11-kit version). Using NSS here means you have to add the PKCS#11 module for your Smartcards reader to /etc/pki/nssdb (the NSS DB GDM uses as well) with modutil or pk11install from the coolkey package.
The PAM configuration so far must not be changed. pam_sss will do a pre-auth request similar to the OPT case for find a suitable authentication method for the user. The pam responder then checks is Smartcard authentication is enabled (pam_cert_auth = True in the [pam] section of sssd.conf), if the service is a local one and if there if a valid certificate can be found which is available in the users LDAP entry as well. If all this checks pass pam_sss will ask the user for a PIN and then SSSD tries to validate that PIN, public and private keys all relate to each other. If no Smartcard is found for the user the standard password prompt is displayed.
With some valuable input form Christian Heimes I think I found a way to test the Smartcard support even without real hardware but I still have to work out some of the details. I will add instructions to the design page and better and more unit tests.
Any comments and suggestions are welcome.
Please find attached an improved version of the patches.
One quick nit in b/src/responder/pam/pamsrv.h: -- snip -- int public_domains_count; + + bool cert_auth; + int p11_child_debug_fd; -- snip -- Typically |int| is 32bit and |bool| is one byte, so we end-up with a 3byte pad here for no good reason. Better would be this order: -- snip -- -- snip -- int public_domains_count; + + int p11_child_debug_fd; + bool cert_auth; -- snip -- (same applies to |struct pam_check_cert_state| with 64bit pointers vs. |int|)
Question: In src/p11_child/p11_child_nss.c |PK11_SetPasswordFunc(password_passthrough)| uses memory from |PL_strdup()| - who is freeing that memory (|PL_free()|) ?
Especially there are improvements to the test, the return values from the p11_child are not mocked used the actual retrieved certificate data. The test data causes some increase in the patch size. I plan to replace this with data which is generate during the test run but for a start it is easier this way.
I also added a 7th patch which should resolve https://fedorahosted.org/sssd/ticket/2711 (SSH with certificates). Strictly it is not related to Smartcard authentication via PAM but it depends on the NSS version of the cert utilities (patch 0001) so I included it here as well.
On the design page I added the 'How to test' section https://fedorahosted.org/sssd/wiki/DesignDocs/SmartcardAuthenticationStep1#H... which hopefully gives sufficient details how to set up a test environment.
Is there any development branch I can use to checkout and build these changes in one step ? It'll make testing of future versions a tick easier...
----
Bye, Roland
----- Original Message -----
From: "Roland Mainz" rmainz@redhat.com To: "Development of the System Security Services Daemon" sssd-devel@lists.fedorahosted.org Cc: "Kai Engert" kengert@redhat.com Sent: Tuesday, 21 July, 2015 3:08:44 AM Subject: Re: [SSSD] [PATCHES] Add support for Smartcard authentication
----- Original Message -----
From: "Sumit Bose" sbose@redhat.com To: sssd-devel@lists.fedorahosted.org Sent: Friday, 17 July, 2015 9:01:49 PM Subject: Re: [SSSD] [PATCHES] Add support for Smartcard authentication
On Fri, Jul 10, 2015 at 06:40:30PM +0200, Sumit Bose wrote:
Hi,
this is the initial version of my patch which add Smartcard authentication to SSSD. I'm still working on a design page which will explain everything in more details so I will only add a short version here.
The main job will be done by a new child process called p11_child. Since the Smartcard support in GDM is based on NSS I used NSS for the first version of p11_child as well. But since all PKCS#11 (API to talk to Smartcards) related code is in this child process adding support for other PKCS#11 frameworks like p11-kit would be straight forward (in fact I already started on the p11-kit version). Using NSS here means you have to add the PKCS#11 module for your Smartcards reader to /etc/pki/nssdb (the NSS DB GDM uses as well) with modutil or pk11install from the coolkey package.
The PAM configuration so far must not be changed. pam_sss will do a pre-auth request similar to the OPT case for find a suitable authentication method for the user. The pam responder then checks is Smartcard authentication is enabled (pam_cert_auth = True in the [pam] section of sssd.conf), if the service is a local one and if there if a valid certificate can be found which is available in the users LDAP entry as well. If all this checks pass pam_sss will ask the user for a PIN and then SSSD tries to validate that PIN, public and private keys all relate to each other. If no Smartcard is found for the user the standard password prompt is displayed.
With some valuable input form Christian Heimes I think I found a way to test the Smartcard support even without real hardware but I still have to work out some of the details. I will add instructions to the design page and better and more unit tests.
Any comments and suggestions are welcome.
Please find attached an improved version of the patches.
One quick nit in b/src/responder/pam/pamsrv.h: -- snip -- int public_domains_count;
- bool cert_auth;
- int p11_child_debug_fd;
-- snip -- Typically |int| is 32bit and |bool| is one byte, so we end-up with a 3byte pad here for no good reason. Better would be this order: -- snip -- -- snip -- int public_domains_count;
- int p11_child_debug_fd;
- bool cert_auth;
-- snip -- (same applies to |struct pam_check_cert_state| with 64bit pointers vs. |int|)
Question: In src/p11_child/p11_child_nss.c |PK11_SetPasswordFunc(password_passthrough)| uses memory from |PL_strdup()| - who is freeing that memory (|PL_free()|) ?
Especially there are improvements to the test, the return values from the p11_child are not mocked used the actual retrieved certificate data. The test data causes some increase in the patch size. I plan to replace this with data which is generate during the test run but for a start it is easier this way.
I also added a 7th patch which should resolve https://fedorahosted.org/sssd/ticket/2711 (SSH with certificates). Strictly it is not related to Smartcard authentication via PAM but it depends on the NSS version of the cert utilities (patch 0001) so I included it here as well.
On the design page I added the 'How to test' section https://fedorahosted.org/sssd/wiki/DesignDocs/SmartcardAuthenticationStep1#H... which hopefully gives sufficient details how to set up a test environment.
Is there any development branch I can use to checkout and build these changes in one step ? It'll make testing of future versions a tick easier...
Development branch for checkout is available at https://fedorapeople.org/cgit/sbose/public_git/sssd.git/log/?h=smartcard ...
----
Bye, Roland
On Fri, Jul 17, 2015 at 09:01:49PM +0200, Sumit Bose wrote:
On Fri, Jul 10, 2015 at 06:40:30PM +0200, Sumit Bose wrote:
Hi,
this is the initial version of my patch which add Smartcard authentication to SSSD. I'm still working on a design page which will explain everything in more details so I will only add a short version here.
The main job will be done by a new child process called p11_child. Since the Smartcard support in GDM is based on NSS I used NSS for the first version of p11_child as well. But since all PKCS#11 (API to talk to Smartcards) related code is in this child process adding support for other PKCS#11 frameworks like p11-kit would be straight forward (in fact I already started on the p11-kit version). Using NSS here means you have to add the PKCS#11 module for your Smartcards reader to /etc/pki/nssdb (the NSS DB GDM uses as well) with modutil or pk11install from the coolkey package.
The PAM configuration so far must not be changed. pam_sss will do a pre-auth request similar to the OPT case for find a suitable authentication method for the user. The pam responder then checks is Smartcard authentication is enabled (pam_cert_auth = True in the [pam] section of sssd.conf), if the service is a local one and if there if a valid certificate can be found which is available in the users LDAP entry as well. If all this checks pass pam_sss will ask the user for a PIN and then SSSD tries to validate that PIN, public and private keys all relate to each other. If no Smartcard is found for the user the standard password prompt is displayed.
With some valuable input form Christian Heimes I think I found a way to test the Smartcard support even without real hardware but I still have to work out some of the details. I will add instructions to the design page and better and more unit tests.
Any comments and suggestions are welcome.
Please find attached an improved version of the patches. Especially there are improvements to the test, the return values from the p11_child are not mocked used the actual retrieved certificate data. The test data causes some increase in the patch size. I plan to replace this with data which is generate during the test run but for a start it is easier this way.
I also added a 7th patch which should resolve https://fedorahosted.org/sssd/ticket/2711 (SSH with certificates). Strictly it is not related to Smartcard authentication via PAM but it depends on the NSS version of the cert utilities (patch 0001) so I included it here as well.
On the design page I added the 'How to test' section https://fedorahosted.org/sssd/wiki/DesignDocs/SmartcardAuthenticationStep1#H... which hopefully gives sufficient details how to set up a test environment.
bye, Sumit
Hi,
I started the review, but because the patches are quite big, I will send my comments in batches. I hope that's fine.
CI failed in distcheck.
Coverity found some issues that I forwarded to Sumit.
From 97b6f25141c56f7d40446b7baf9780dacf8a822b Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Mon, 22 Jun 2015 16:36:36 +0200 Subject: [PATCH 1/7] utils: add NSS version of cert utils
With this patch and p11_child being written with NSS, should the downstreams switch to NSS only again and drop the openssl-devel BuildRequires, as you did in the upstream specfile?
Makefile.am | 29 ++++- configure.ac | 4 +- contrib/sssd.spec.in | 1 - src/tests/cmocka/test_cert_utils.c | 4 + src/util/cert/nss/cert.c | 211 +++++++++++++++++++++++++++++++++++++ 5 files changed, 243 insertions(+), 6 deletions(-) create mode 100644 src/util/cert/nss/cert.c
[...]
+errno_t sss_cert_der_to_pem(TALLOC_CTX *mem_ctx, const uint8_t *der_blob,
size_t der_size, char **pem, size_t *pem_size)+{
- CERTCertDBHandle *handle;
- CERTCertificate *cert = NULL;
- SECItem der_item;
- char *ascii_crlf = NULL;
- size_t ascii_crlf_len;
- char *ascii_lf = NULL;
- char *pem_cert_str = NULL;
- int ret;
- size_t c;
- size_t d;
- /* initialize NSS if needed */
- ret = nspr_nss_init();
- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "nspr_nss_init failed.\n");return ret;- }
- handle = CERT_GetDefaultCertDB();
- der_item.len = der_size;
- der_item.data = discard_const(der_blob);
- cert = CERT_NewTempCertificate(handle, &der_item, NULL, PR_FALSE , PR_TRUE);
~~~ If you end up changing this patch, please also remove the extra space.
- if (cert == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "CERT_NewTempCertificate failed.\n");return EINVAL;- }
- ascii_crlf = BTOA_DataToAscii(cert->derCert.data, cert->derCert.len);
- if (ascii_crlf == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "BTOA_DataToAscii failed.\n");ret = EIO;goto done;- }
- ascii_crlf_len = strlen(ascii_crlf) + 1;
- ascii_lf = malloc(ascii_crlf_len * sizeof(char));
Any reason to use malloc and not talloc here? Using talloc would protect against leaks better, I think, if we forget to free ascii_lf here, but mem_ctx goes away later.
- if (ascii_lf == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "malloc failed.\n");ret = ENOMEM;goto done;- }
- d = 0;
- for (c = 0; c < ascii_crlf_len; c++) {
if (ascii_crlf[c] != '\r') {ascii_lf[d++] = ascii_crlf[c];}- }
- pem_cert_str = talloc_asprintf(mem_ctx, "%s\n%s\n%s\n", NS_CERT_HEADER,
ascii_lf,NS_CERT_TRAILER);- if (pem_cert_str == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n");ret = ENOMEM;goto done;- }
- if (pem_size != NULL) {
*pem_size = strlen(pem_cert_str);- }
- if (pem != NULL) {
*pem = pem_cert_str;pem_cert_str = NULL;- }
- ret = EOK;
+done:
- talloc_free(pem_cert_str);
- free(ascii_crlf);
It would be better to use PORT_free() to stick with NSPR API because the buffer is already created with NSS/NSPR.
- free(ascii_lf);
- CERT_DestroyCertificate(cert);
- return ret;
+}
+errno_t sss_cert_pem_to_der(TALLOC_CTX *mem_ctx, const char *pem,
uint8_t **_der_blob, size_t *_der_size)+{
- const char *ps;
- const char *pe;
- size_t pem_len;
- uint8_t *der_blob = NULL;
- size_t der_size;
- CERTCertDBHandle *handle;
- CERTCertificate *cert = NULL;
- SECItem der_item;
- int ret;
- char *b64 = NULL;
- /* initialize NSS if needed */
- ret = nspr_nss_init();
- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "nspr_nss_init failed.\n");return ret;- }
- if (pem == NULL || *pem == '\0') {
return EINVAL;- }
- pem_len = strlen(pem);
- if (pem_len <= NS_CERT_HEADER_LEN + NS_CERT_TRAILER_LEN) {
DEBUG(SSSDBG_CRIT_FAILURE, "PEM data too short.\n");return EINVAL;- }
- if (strncmp(pem, NS_CERT_HEADER, NS_CERT_HEADER_LEN) != 0) {
DEBUG(SSSDBG_CRIT_FAILURE, "Wrong PEM header.\n");return EINVAL;- }
- if (pem[NS_CERT_HEADER_LEN] != '\n') {
DEBUG(SSSDBG_CRIT_FAILURE, "Missing newline in PEM data.\n");return EINVAL;- }
- pe = pem + pem_len - NS_CERT_TRAILER_LEN;
- if (pem[pem_len - 1] == '\n') {
pe--;- }
- if (strncmp(pe, NS_CERT_TRAILER, NS_CERT_TRAILER_LEN) != 0) {
DEBUG(SSSDBG_CRIT_FAILURE, "Wrong PEM trailer.\n");return EINVAL;- }
- ps = pem + NS_CERT_HEADER_LEN + 1;
- b64 = talloc_strndup(mem_ctx, ps, pe - ps);
- if(b64 == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_strndup failed.\n");ret = ENOMEM;goto done;- }
- der_blob = ATOB_AsciiToData(b64, &der_size);
- if (der_blob == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "ATOB_AsciiToData failed.\n");return EIO;- }
- handle = CERT_GetDefaultCertDB();
- der_item.len = der_size;
- der_item.data = der_blob;
- cert = CERT_NewTempCertificate(handle, &der_item, NULL, PR_FALSE , PR_TRUE);
~~ Extra space
- if (cert == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "CERT_NewTempCertificate failed.\n");ret = EINVAL;goto done;- }
- if (_der_blob != NULL) {
*_der_blob = talloc_memdup(mem_ctx, cert->derCert.data,cert->derCert.len);if (*_der_blob == NULL) {DEBUG(SSSDBG_OP_FAILURE, "talloc_memdup failed.\n");ret = ENOMEM;goto done;}- }
- if (_der_size != NULL) {
*_der_size = cert->derCert.len;- }
+done:
- free(der_blob);
PORT_free() would be nicer here as well.
- talloc_free(b64);
- CERT_DestroyCertificate(cert);
- return ret;
+}
2.1.0
From 14532dbe323aa18c48b02c7d2178b2cb85c19e72 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Fri, 10 Jul 2015 12:10:53 +0200 Subject: [PATCH 2/7] Add NSS version of p11_child
Makefile.am | 25 +- src/p11_child/p11_child_nss.c | 634 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 658 insertions(+), 1 deletion(-) create mode 100644 src/p11_child/p11_child_nss.c
[...]
endif +if HAVE_NSS
- -chgrp $(SSSD_USER) $(DESTDIR)$(sssdlibexecdir)/p11_child
- chmod 4750 $(DESTDIR)$(sssdlibexecdir)/p11_child
Because this patch adds a setuid executable, I would prefer if the specfile change that also specifies the ownership and permissions of the executable was part of this patch.
+endif endif
install-data-hook: diff --git a/src/p11_child/p11_child_nss.c b/src/p11_child/p11_child_nss.c
In general my NSS knowledge is lacking a bit here. It would be helpful if you could add some comments (see inline). Also, did you follow some particular NSS documentation? Or another code that is using NSS (this is mostly for my education)
The code looks mostly good to me, though, just a couple of small picks..
new file mode 100644 index 0000000000000000000000000000000000000000..63e6ded7f6dbb2c5f05c085eb40082cf7f992b98 --- /dev/null +++ b/src/p11_child/p11_child_nss.c @@ -0,0 +1,634 @@ +/*
- SSSD
- Helper child to commmunicate with SamrtCard via NSS
~~~~~~~~ typo
- Authors:
Sumit Bose <sbose@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
[...]
+static char *password_passthrough(PK11SlotInfo *slot, PRBool retry, void *arg) +{
- /* give up if 1) no password was supplied, or 2) the password has already
- been rejected once by this token. */
This comment is strangely formatted
- if (retry || (arg == NULL)) {
- return NULL;
- }
- return PL_strdup((char *)arg);
+}
+int do_work(TALLOC_CTX *mem_ctx, const char *nss_db, const char *slot_name_in,
enum op_mode mode, const char *pin, char **cert,char **token_name_out)+{
- int ret;
- SECStatus rv;
- NSSInitContext *nss_ctx;
- SECMODModuleList *mod_list;
- SECMODModuleList *mod_list_item;
- const char *slot_name;
- const char *token_name;
- uint32_t flags = NSS_INIT_READONLY
| NSS_INIT_FORCEOPEN| NSS_INIT_NOROOTINIT| NSS_INIT_OPTIMIZESPACE| NSS_INIT_PK11RELOAD;- NSSInitParameters parameters = { 0 };
- parameters.length = sizeof (parameters);
- PK11SlotInfo *slot = NULL;
- CK_SLOT_ID slot_id;
- SECMODModuleID module_id;
- CERTCertList *cert_list = NULL;
- CERTCertListNode *cert_list_node;
- const PK11DefaultArrayEntry friendly_attr = { "Publicly-readable certs",
SECMOD_FRIENDLY_FLAG,CKM_INVALID_MECHANISM };- CERTCertDBHandle *handle;
- unsigned char random_value[128];
- SECKEYPrivateKey *priv_key;
- SECOidTag algtag;
- SECItem signed_random_value = {0};
- SECKEYPublicKey *pub_key;
- CERTCertificate *found_cert = NULL;
- PK11SlotList *list = NULL;
- PK11SlotListElement *le;
- nss_ctx = NSS_InitContext(nss_db, "", "", SECMOD_DB, ¶meters, flags);
- if (nss_ctx == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "NSS_InitContext failed [%d].\n",PR_GetError());return EIO;- }
- PK11_SetPasswordFunc(password_passthrough);
- DEBUG(SSSDBG_TRACE_ALL, "Default Module List:\n");
- mod_list = SECMOD_GetDefaultModuleList();
- for (mod_list_item = mod_list; mod_list_item != NULL;
mod_list_item = mod_list_item->next) {DEBUG(SSSDBG_TRACE_ALL, "common name: [%s].\n",mod_list_item->module->commonName);DEBUG(SSSDBG_TRACE_ALL, "dll name: [%s].\n",mod_list_item->module->dllName);- }
- DEBUG(SSSDBG_TRACE_ALL, "Dead Module List:\n");
What does "dead module" mean?
- mod_list = SECMOD_GetDeadModuleList();
- for (mod_list_item = mod_list; mod_list_item != NULL;
mod_list_item = mod_list_item->next) {DEBUG(SSSDBG_TRACE_ALL, "common name: [%s].\n",mod_list_item->module->commonName);DEBUG(SSSDBG_TRACE_ALL, "dll name: [%s].\n",mod_list_item->module->dllName);- }
- DEBUG(SSSDBG_TRACE_ALL, "DB Module List:\n");
- mod_list = SECMOD_GetDeadModuleList();
Did you really mean to call SECMOD_GetDeadModuleList() twice?
- for (mod_list_item = mod_list; mod_list_item != NULL;
mod_list_item = mod_list_item->next) {DEBUG(SSSDBG_TRACE_ALL, "common name: [%s].\n",mod_list_item->module->commonName);DEBUG(SSSDBG_TRACE_ALL, "dll name: [%s].\n",mod_list_item->module->dllName);- }
- if (slot_name_in != NULL) {
slot = PK11_FindSlotByName(slot_name_in);if (slot == NULL) {DEBUG(SSSDBG_OP_FAILURE, "PK11_FindSlotByName failed for [%s]: [%d].\n",slot_name_in, PR_GetError());return EIO;}- } else {
list = PK11_GetAllTokens(CKM_INVALID_MECHANISM, PR_FALSE, PR_TRUE,NULL);if (list == NULL) {DEBUG(SSSDBG_OP_FAILURE, "PK11_GetAllTokens failed.\n");return EIO;}for (le = list->head; le; le = le->next) {CK_SLOT_INFO slInfo;slInfo.flags = 0;rv = PK11_GetSlotInfo(le->slot, &slInfo);DEBUG(SSSDBG_TRACE_ALL,"Description [%s] Manufacturer [%s] flags [%lu].\n",slInfo.slotDescription, slInfo.manufacturerID, slInfo.flags);if (rv == SECSuccess && (slInfo.flags & CKF_REMOVABLE_DEVICE)) {slot = PK11_ReferenceSlot(le->slot);break;}}PK11_FreeSlotList(list);if (slot == NULL) {DEBUG(SSSDBG_OP_FAILURE, "No removable slots found.\n");return EIO;}- }
- slot_id = PK11_GetSlotID(slot);
- module_id = PK11_GetModuleID(slot);
- slot_name = PK11_GetSlotName(slot);
- token_name = PK11_GetTokenName(slot);
- DEBUG(SSSDBG_TRACE_ALL, "Found [%s] in slot [%s][%d] of module [%d].\n",
token_name, slot_name, (int) slot_id, (int) module_id);- if (PK11_IsFriendly(slot)) {
DEBUG(SSSDBG_TRACE_ALL, "Token is friendly.\n");
What does it mean for a token to be friendly? man modutil says it's when certificates are publicly readable, I guess that's it?
- } else {
DEBUG(SSSDBG_TRACE_ALL,"Token is NOT friendly.\n");if (mode == OP_PREAUTH) {DEBUG(SSSDBG_TRACE_ALL, "Trying to switch to friendly to read certificate.\n");rv = PK11_UpdateSlotAttribute(slot, &friendly_attr, PR_TRUE);if (rv != SECSuccess) {DEBUG(SSSDBG_OP_FAILURE,"PK11_UpdateSlotAttribute failed, continue.\n");}}- }
- /* TODO: check PK11_ProtectedAuthenticationPath() and return the result */
- if (mode == OP_AUTH || PK11_NeedLogin(slot)) {
DEBUG(SSSDBG_TRACE_ALL, "Login required.\n");if (pin != NULL) {rv = PK11_Authenticate(slot, PR_FALSE, discard_const(pin));if (rv != SECSuccess) {DEBUG(SSSDBG_OP_FAILURE, "PK11_Authenticate failed: [%d].\n",PR_GetError());return EIO;}} else {DEBUG(SSSDBG_CRIT_FAILURE,"Login required but no pin available, continue.\n");}- } else {
DEBUG(SSSDBG_TRACE_ALL, "Login NOT required.\n");- }
- cert_list = PK11_ListCertsInSlot(slot);
- if (cert_list == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "PK11_ListCertsInSlot failed: [%d].\n",PR_GetError());return EIO;- }
- for (cert_list_node = CERT_LIST_HEAD(cert_list);
!CERT_LIST_END(cert_list_node, cert_list);cert_list_node = CERT_LIST_NEXT(cert_list_node)) {if (cert_list_node->cert) {DEBUG(SSSDBG_TRACE_ALL, "found cert[%s][%s]\n",cert_list_node->cert->nickname,cert_list_node->cert->subjectName);} else {DEBUG(SSSDBG_TRACE_ALL, "--- empty cert list node ---\n");}- }
- rv = CERT_FilterCertListByUsage(cert_list, certUsageSSLClient, PR_FALSE);
- if (rv != SECSuccess) {
DEBUG(SSSDBG_OP_FAILURE, "CERT_FilterCertListByUsage failed: [%d].\n",PR_GetError());return EIO;- }
- rv = CERT_FilterCertListForUserCerts(cert_list);
- if (rv != SECSuccess) {
DEBUG(SSSDBG_OP_FAILURE, "CERT_FilterCertListForUserCerts failed: [%d].\n",PR_GetError());return EIO;- }
- handle = CERT_GetDefaultCertDB();
- if (handle == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "CERT_GetDefaultCertDB failed: [%d].\n",PR_GetError());return EIO;- }
- found_cert = NULL;
- DEBUG(SSSDBG_TRACE_ALL, "Filtered certificates:\n");
- for (cert_list_node = CERT_LIST_HEAD(cert_list);
!CERT_LIST_END(cert_list_node, cert_list);cert_list_node = CERT_LIST_NEXT(cert_list_node)) {if (cert_list_node->cert) {DEBUG(SSSDBG_TRACE_ALL, "found cert[%s][%s]\n",cert_list_node->cert->nickname,cert_list_node->cert->subjectName);if (found_cert == NULL) {found_cert = cert_list_node->cert;} else {DEBUG(SSSDBG_TRACE_ALL, "More than one certificate found, " \"using just the first one.\n");}} else {DEBUG(SSSDBG_TRACE_ALL, "--- empty cert list node ---\n");}- }
- if (found_cert == NULL) {
DEBUG(SSSDBG_TRACE_ALL, "No certificate found.\n");*cert = NULL;*token_name_out = NULL;ret = EOK;goto done;- }
- rv = CERT_VerifyCertificateNow(handle, found_cert, PR_TRUE,
certificateUsageSSLClient, NULL, NULL);
Is the comment in cert_to_ssh_key() still valid since we run CERT_VerifyCertificateNow here?
- if (rv != SECSuccess) {
DEBUG(SSSDBG_OP_FAILURE,"CERT_VerifyCertificateNow failed [%d].\n",PR_GetError());ret = EIO;goto done;- }
[...]
+int main(int argc, const char *argv[]) +{
- int opt;
- poptContext pc;
- int debug_fd = -1;
- errno_t ret;
- TALLOC_CTX *main_ctx = NULL;
- char *cert;
- enum op_mode mode = OP_NONE;
- enum pin_mode pin_mode = PIN_NONE;
- char *pin = NULL;
- char *slot_name_in = NULL;
- char *token_name_out = NULL;
- char *nss_db = NULL;
- struct poptOption long_options[] = {
POPT_AUTOHELP{"debug-level", 'd', POPT_ARG_INT, &debug_level, 0,_("Debug level"), NULL},{"debug-timestamps", 0, POPT_ARG_INT, &debug_timestamps, 0,_("Add debug timestamps"), NULL},{"debug-microseconds", 0, POPT_ARG_INT, &debug_microseconds, 0,_("Show timestamps with microseconds"), NULL},{"debug-fd", 0, POPT_ARG_INT, &debug_fd, 0,_("An open file descriptor for the debug logs"), NULL},{"debug-to-stderr", 0, POPT_ARG_NONE | POPT_ARGFLAG_DOC_HIDDEN,&debug_to_stderr, 0,_("Send the debug output to stderr directly."), NULL },{"auth", 0, POPT_ARG_NONE, NULL, 'a', _("Run in auth mode"), NULL},{"pre", 0, POPT_ARG_NONE, NULL, 'p', _("Run in pre-auth mode"), NULL},{"pin", 0, POPT_ARG_NONE, NULL, 'i', _("Expect PIN on stdin"), NULL},{"keypad", 0, POPT_ARG_NONE, NULL, 'k', _("Expect PIN on keypad"),NULL},{"nssdb", 0, POPT_ARG_STRING, &nss_db, 0, _("NSS DB to use"),NULL},POPT_TABLEEND- };
- /* Set debug level to invalid value so we can decide if -d 0 was used. */
- debug_level = SSSDBG_INVALID;
- pc = poptGetContext(argv[0], argc, argv, long_options, 0);
- while((opt = poptGetNextOpt(pc)) != -1) {
~~ Please put a space here.
switch(opt) {case 'a':if (mode != OP_NONE) {fprintf(stderr,"\n--auth and --pre are mutually exclusive and " \"should be only used once.\n\n");poptPrintUsage(pc, stderr, 0);_exit(-1);}mode = OP_AUTH;break;
From f004acdd79dfafb3c89a57e1b11950f2f5c3345e Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Fri, 26 Jun 2015 14:45:21 +0200 Subject: [PATCH 3/7] pack_message_v3: allow empty name
ACK, we do the same for service already.
On Mon, Jul 27, 2015 at 12:35:45PM +0200, Jakub Hrozek wrote:
On Fri, Jul 17, 2015 at 09:01:49PM +0200, Sumit Bose wrote:
On Fri, Jul 10, 2015 at 06:40:30PM +0200, Sumit Bose wrote:
Hi,
this is the initial version of my patch which add Smartcard authentication to SSSD. I'm still working on a design page which will explain everything in more details so I will only add a short version here.
The main job will be done by a new child process called p11_child. Since the Smartcard support in GDM is based on NSS I used NSS for the first version of p11_child as well. But since all PKCS#11 (API to talk to Smartcards) related code is in this child process adding support for other PKCS#11 frameworks like p11-kit would be straight forward (in fact I already started on the p11-kit version). Using NSS here means you have to add the PKCS#11 module for your Smartcards reader to /etc/pki/nssdb (the NSS DB GDM uses as well) with modutil or pk11install from the coolkey package.
The PAM configuration so far must not be changed. pam_sss will do a pre-auth request similar to the OPT case for find a suitable authentication method for the user. The pam responder then checks is Smartcard authentication is enabled (pam_cert_auth = True in the [pam] section of sssd.conf), if the service is a local one and if there if a valid certificate can be found which is available in the users LDAP entry as well. If all this checks pass pam_sss will ask the user for a PIN and then SSSD tries to validate that PIN, public and private keys all relate to each other. If no Smartcard is found for the user the standard password prompt is displayed.
With some valuable input form Christian Heimes I think I found a way to test the Smartcard support even without real hardware but I still have to work out some of the details. I will add instructions to the design page and better and more unit tests.
Any comments and suggestions are welcome.
Please find attached an improved version of the patches. Especially there are improvements to the test, the return values from the p11_child are not mocked used the actual retrieved certificate data. The test data causes some increase in the patch size. I plan to replace this with data which is generate during the test run but for a start it is easier this way.
I also added a 7th patch which should resolve https://fedorahosted.org/sssd/ticket/2711 (SSH with certificates). Strictly it is not related to Smartcard authentication via PAM but it depends on the NSS version of the cert utilities (patch 0001) so I included it here as well.
On the design page I added the 'How to test' section https://fedorahosted.org/sssd/wiki/DesignDocs/SmartcardAuthenticationStep1#H... which hopefully gives sufficient details how to set up a test environment.
bye, Sumit
Hi,
I started the review, but because the patches are quite big, I will send my comments in batches. I hope that's fine.
Thank you for the review.
CI failed in distcheck.
The failure was due to distcheck's tendency to make the complete source tree read-only and NSS's tendency open all databases in read-write more. I added the needed read-onl options to the NSS code.
Coverity found some issues that I forwarded to Sumit.
Thank you, I fixed them and hopefully did not create new ones.
From 97b6f25141c56f7d40446b7baf9780dacf8a822b Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Mon, 22 Jun 2015 16:36:36 +0200 Subject: [PATCH 1/7] utils: add NSS version of cert utils
With this patch and p11_child being written with NSS, should the downstreams switch to NSS only again and drop the openssl-devel BuildRequires, as you did in the upstream specfile?
Makefile.am | 29 ++++- configure.ac | 4 +- contrib/sssd.spec.in | 1 - src/tests/cmocka/test_cert_utils.c | 4 + src/util/cert/nss/cert.c | 211 +++++++++++++++++++++++++++++++++++++ 5 files changed, 243 insertions(+), 6 deletions(-) create mode 100644 src/util/cert/nss/cert.c
[...]
+errno_t sss_cert_der_to_pem(TALLOC_CTX *mem_ctx, const uint8_t *der_blob,
size_t der_size, char **pem, size_t *pem_size)+{
- CERTCertDBHandle *handle;
- CERTCertificate *cert = NULL;
- SECItem der_item;
- char *ascii_crlf = NULL;
- size_t ascii_crlf_len;
- char *ascii_lf = NULL;
- char *pem_cert_str = NULL;
- int ret;
- size_t c;
- size_t d;
- /* initialize NSS if needed */
- ret = nspr_nss_init();
- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "nspr_nss_init failed.\n");return ret;- }
- handle = CERT_GetDefaultCertDB();
- der_item.len = der_size;
- der_item.data = discard_const(der_blob);
- cert = CERT_NewTempCertificate(handle, &der_item, NULL, PR_FALSE , PR_TRUE);
~~~If you end up changing this patch, please also remove the extra space.
done
- if (cert == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "CERT_NewTempCertificate failed.\n");return EINVAL;- }
- ascii_crlf = BTOA_DataToAscii(cert->derCert.data, cert->derCert.len);
- if (ascii_crlf == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "BTOA_DataToAscii failed.\n");ret = EIO;goto done;- }
- ascii_crlf_len = strlen(ascii_crlf) + 1;
- ascii_lf = malloc(ascii_crlf_len * sizeof(char));
Any reason to use malloc and not talloc here? Using talloc would protect against leaks better, I think, if we forget to free ascii_lf here, but mem_ctx goes away later.
no real reason, I guess I wasn't aware that there already is a talloc context around, I switched to talloc.
- if (ascii_lf == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "malloc failed.\n");ret = ENOMEM;goto done;- }
- d = 0;
- for (c = 0; c < ascii_crlf_len; c++) {
if (ascii_crlf[c] != '\r') {ascii_lf[d++] = ascii_crlf[c];}- }
- pem_cert_str = talloc_asprintf(mem_ctx, "%s\n%s\n%s\n", NS_CERT_HEADER,
ascii_lf,NS_CERT_TRAILER);- if (pem_cert_str == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n");ret = ENOMEM;goto done;- }
- if (pem_size != NULL) {
*pem_size = strlen(pem_cert_str);- }
- if (pem != NULL) {
*pem = pem_cert_str;pem_cert_str = NULL;- }
- ret = EOK;
+done:
- talloc_free(pem_cert_str);
- free(ascii_crlf);
It would be better to use PORT_free() to stick with NSPR API because the buffer is already created with NSS/NSPR.
done
- free(ascii_lf);
- CERT_DestroyCertificate(cert);
- return ret;
+}
+errno_t sss_cert_pem_to_der(TALLOC_CTX *mem_ctx, const char *pem,
uint8_t **_der_blob, size_t *_der_size)+{
- const char *ps;
- const char *pe;
- size_t pem_len;
- uint8_t *der_blob = NULL;
- size_t der_size;
- CERTCertDBHandle *handle;
- CERTCertificate *cert = NULL;
- SECItem der_item;
- int ret;
- char *b64 = NULL;
- /* initialize NSS if needed */
- ret = nspr_nss_init();
- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "nspr_nss_init failed.\n");return ret;- }
- if (pem == NULL || *pem == '\0') {
return EINVAL;- }
- pem_len = strlen(pem);
- if (pem_len <= NS_CERT_HEADER_LEN + NS_CERT_TRAILER_LEN) {
DEBUG(SSSDBG_CRIT_FAILURE, "PEM data too short.\n");return EINVAL;- }
- if (strncmp(pem, NS_CERT_HEADER, NS_CERT_HEADER_LEN) != 0) {
DEBUG(SSSDBG_CRIT_FAILURE, "Wrong PEM header.\n");return EINVAL;- }
- if (pem[NS_CERT_HEADER_LEN] != '\n') {
DEBUG(SSSDBG_CRIT_FAILURE, "Missing newline in PEM data.\n");return EINVAL;- }
- pe = pem + pem_len - NS_CERT_TRAILER_LEN;
- if (pem[pem_len - 1] == '\n') {
pe--;- }
- if (strncmp(pe, NS_CERT_TRAILER, NS_CERT_TRAILER_LEN) != 0) {
DEBUG(SSSDBG_CRIT_FAILURE, "Wrong PEM trailer.\n");return EINVAL;- }
- ps = pem + NS_CERT_HEADER_LEN + 1;
- b64 = talloc_strndup(mem_ctx, ps, pe - ps);
- if(b64 == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_strndup failed.\n");ret = ENOMEM;goto done;- }
- der_blob = ATOB_AsciiToData(b64, &der_size);
- if (der_blob == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "ATOB_AsciiToData failed.\n");return EIO;- }
- handle = CERT_GetDefaultCertDB();
- der_item.len = der_size;
- der_item.data = der_blob;
- cert = CERT_NewTempCertificate(handle, &der_item, NULL, PR_FALSE , PR_TRUE);
~~ Extra space
fixed
- if (cert == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "CERT_NewTempCertificate failed.\n");ret = EINVAL;goto done;- }
- if (_der_blob != NULL) {
*_der_blob = talloc_memdup(mem_ctx, cert->derCert.data,cert->derCert.len);if (*_der_blob == NULL) {DEBUG(SSSDBG_OP_FAILURE, "talloc_memdup failed.\n");ret = ENOMEM;goto done;}- }
- if (_der_size != NULL) {
*_der_size = cert->derCert.len;- }
+done:
- free(der_blob);
PORT_free() would be nicer here as well.
done
- talloc_free(b64);
- CERT_DestroyCertificate(cert);
- return ret;
+}
2.1.0
From 14532dbe323aa18c48b02c7d2178b2cb85c19e72 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Fri, 10 Jul 2015 12:10:53 +0200 Subject: [PATCH 2/7] Add NSS version of p11_child
Makefile.am | 25 +- src/p11_child/p11_child_nss.c | 634 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 658 insertions(+), 1 deletion(-) create mode 100644 src/p11_child/p11_child_nss.c
[...]
endif +if HAVE_NSS
- -chgrp $(SSSD_USER) $(DESTDIR)$(sssdlibexecdir)/p11_child
- chmod 4750 $(DESTDIR)$(sssdlibexecdir)/p11_child
Because this patch adds a setuid executable, I would prefer if the specfile change that also specifies the ownership and permissions of the executable was part of this patch.
fixed, not sure why it went into the other patch, added ownership as well.
+endif endif
install-data-hook: diff --git a/src/p11_child/p11_child_nss.c b/src/p11_child/p11_child_nss.c
In general my NSS knowledge is lacking a bit here. It would be helpful if you could add some comments (see inline). Also, did you follow some particular NSS documentation? Or another code that is using NSS (this is mostly for my education)
For this first step I followed what the NSS version of pam_pkcs11 is doing to be as compatible as possible.
The code looks mostly good to me, though, just a couple of small picks..
new file mode 100644 index 0000000000000000000000000000000000000000..63e6ded7f6dbb2c5f05c085eb40082cf7f992b98 --- /dev/null +++ b/src/p11_child/p11_child_nss.c @@ -0,0 +1,634 @@ +/*
- SSSD
- Helper child to commmunicate with SamrtCard via NSS
~~~~~~~~ typo
duh, fixed
- Authors:
Sumit Bose <sbose@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
[...]
+static char *password_passthrough(PK11SlotInfo *slot, PRBool retry, void *arg) +{
- /* give up if 1) no password was supplied, or 2) the password has already
- been rejected once by this token. */
This comment is strangely formatted
fixed
- if (retry || (arg == NULL)) {
- return NULL;
- }
- return PL_strdup((char *)arg);
+}
+int do_work(TALLOC_CTX *mem_ctx, const char *nss_db, const char *slot_name_in,
enum op_mode mode, const char *pin, char **cert,char **token_name_out)+{
- int ret;
- SECStatus rv;
- NSSInitContext *nss_ctx;
- SECMODModuleList *mod_list;
- SECMODModuleList *mod_list_item;
- const char *slot_name;
- const char *token_name;
- uint32_t flags = NSS_INIT_READONLY
| NSS_INIT_FORCEOPEN| NSS_INIT_NOROOTINIT| NSS_INIT_OPTIMIZESPACE| NSS_INIT_PK11RELOAD;- NSSInitParameters parameters = { 0 };
- parameters.length = sizeof (parameters);
- PK11SlotInfo *slot = NULL;
- CK_SLOT_ID slot_id;
- SECMODModuleID module_id;
- CERTCertList *cert_list = NULL;
- CERTCertListNode *cert_list_node;
- const PK11DefaultArrayEntry friendly_attr = { "Publicly-readable certs",
SECMOD_FRIENDLY_FLAG,CKM_INVALID_MECHANISM };- CERTCertDBHandle *handle;
- unsigned char random_value[128];
- SECKEYPrivateKey *priv_key;
- SECOidTag algtag;
- SECItem signed_random_value = {0};
- SECKEYPublicKey *pub_key;
- CERTCertificate *found_cert = NULL;
- PK11SlotList *list = NULL;
- PK11SlotListElement *le;
- nss_ctx = NSS_InitContext(nss_db, "", "", SECMOD_DB, ¶meters, flags);
- if (nss_ctx == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "NSS_InitContext failed [%d].\n",PR_GetError());return EIO;- }
- PK11_SetPasswordFunc(password_passthrough);
- DEBUG(SSSDBG_TRACE_ALL, "Default Module List:\n");
- mod_list = SECMOD_GetDefaultModuleList();
- for (mod_list_item = mod_list; mod_list_item != NULL;
mod_list_item = mod_list_item->next) {DEBUG(SSSDBG_TRACE_ALL, "common name: [%s].\n",mod_list_item->module->commonName);DEBUG(SSSDBG_TRACE_ALL, "dll name: [%s].\n",mod_list_item->module->dllName);- }
- DEBUG(SSSDBG_TRACE_ALL, "Dead Module List:\n");
What does "dead module" mean?
I guess modules which are configured but could not be initialized properly or have no devices attached.
- mod_list = SECMOD_GetDeadModuleList();
- for (mod_list_item = mod_list; mod_list_item != NULL;
mod_list_item = mod_list_item->next) {DEBUG(SSSDBG_TRACE_ALL, "common name: [%s].\n",mod_list_item->module->commonName);DEBUG(SSSDBG_TRACE_ALL, "dll name: [%s].\n",mod_list_item->module->dllName);- }
- DEBUG(SSSDBG_TRACE_ALL, "DB Module List:\n");
- mod_list = SECMOD_GetDeadModuleList();
Did you really mean to call SECMOD_GetDeadModuleList() twice?
no, fixed
- for (mod_list_item = mod_list; mod_list_item != NULL;
mod_list_item = mod_list_item->next) {DEBUG(SSSDBG_TRACE_ALL, "common name: [%s].\n",mod_list_item->module->commonName);DEBUG(SSSDBG_TRACE_ALL, "dll name: [%s].\n",mod_list_item->module->dllName);- }
- if (slot_name_in != NULL) {
slot = PK11_FindSlotByName(slot_name_in);if (slot == NULL) {DEBUG(SSSDBG_OP_FAILURE, "PK11_FindSlotByName failed for [%s]: [%d].\n",slot_name_in, PR_GetError());return EIO;}- } else {
list = PK11_GetAllTokens(CKM_INVALID_MECHANISM, PR_FALSE, PR_TRUE,NULL);if (list == NULL) {DEBUG(SSSDBG_OP_FAILURE, "PK11_GetAllTokens failed.\n");return EIO;}for (le = list->head; le; le = le->next) {CK_SLOT_INFO slInfo;slInfo.flags = 0;rv = PK11_GetSlotInfo(le->slot, &slInfo);DEBUG(SSSDBG_TRACE_ALL,"Description [%s] Manufacturer [%s] flags [%lu].\n",slInfo.slotDescription, slInfo.manufacturerID, slInfo.flags);if (rv == SECSuccess && (slInfo.flags & CKF_REMOVABLE_DEVICE)) {slot = PK11_ReferenceSlot(le->slot);break;}}PK11_FreeSlotList(list);if (slot == NULL) {DEBUG(SSSDBG_OP_FAILURE, "No removable slots found.\n");return EIO;}- }
- slot_id = PK11_GetSlotID(slot);
- module_id = PK11_GetModuleID(slot);
- slot_name = PK11_GetSlotName(slot);
- token_name = PK11_GetTokenName(slot);
- DEBUG(SSSDBG_TRACE_ALL, "Found [%s] in slot [%s][%d] of module [%d].\n",
token_name, slot_name, (int) slot_id, (int) module_id);- if (PK11_IsFriendly(slot)) {
DEBUG(SSSDBG_TRACE_ALL, "Token is friendly.\n");What does it mean for a token to be friendly? man modutil says it's when certificates are publicly readable, I guess that's it?
yes, this can be flagged on the card but is not required but it looks like NSS only proceeds without requiring a PIN if the friendly flag is set, so I just give it a try in the pre-auth run.
- } else {
DEBUG(SSSDBG_TRACE_ALL,"Token is NOT friendly.\n");if (mode == OP_PREAUTH) {DEBUG(SSSDBG_TRACE_ALL, "Trying to switch to friendly to read certificate.\n");rv = PK11_UpdateSlotAttribute(slot, &friendly_attr, PR_TRUE);if (rv != SECSuccess) {DEBUG(SSSDBG_OP_FAILURE,"PK11_UpdateSlotAttribute failed, continue.\n");}}- }
- /* TODO: check PK11_ProtectedAuthenticationPath() and return the result */
- if (mode == OP_AUTH || PK11_NeedLogin(slot)) {
DEBUG(SSSDBG_TRACE_ALL, "Login required.\n");if (pin != NULL) {rv = PK11_Authenticate(slot, PR_FALSE, discard_const(pin));if (rv != SECSuccess) {DEBUG(SSSDBG_OP_FAILURE, "PK11_Authenticate failed: [%d].\n",PR_GetError());return EIO;}} else {DEBUG(SSSDBG_CRIT_FAILURE,"Login required but no pin available, continue.\n");}- } else {
DEBUG(SSSDBG_TRACE_ALL, "Login NOT required.\n");- }
- cert_list = PK11_ListCertsInSlot(slot);
- if (cert_list == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "PK11_ListCertsInSlot failed: [%d].\n",PR_GetError());return EIO;- }
- for (cert_list_node = CERT_LIST_HEAD(cert_list);
!CERT_LIST_END(cert_list_node, cert_list);cert_list_node = CERT_LIST_NEXT(cert_list_node)) {if (cert_list_node->cert) {DEBUG(SSSDBG_TRACE_ALL, "found cert[%s][%s]\n",cert_list_node->cert->nickname,cert_list_node->cert->subjectName);} else {DEBUG(SSSDBG_TRACE_ALL, "--- empty cert list node ---\n");}- }
- rv = CERT_FilterCertListByUsage(cert_list, certUsageSSLClient, PR_FALSE);
- if (rv != SECSuccess) {
DEBUG(SSSDBG_OP_FAILURE, "CERT_FilterCertListByUsage failed: [%d].\n",PR_GetError());return EIO;- }
- rv = CERT_FilterCertListForUserCerts(cert_list);
- if (rv != SECSuccess) {
DEBUG(SSSDBG_OP_FAILURE, "CERT_FilterCertListForUserCerts failed: [%d].\n",PR_GetError());return EIO;- }
- handle = CERT_GetDefaultCertDB();
- if (handle == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "CERT_GetDefaultCertDB failed: [%d].\n",PR_GetError());return EIO;- }
- found_cert = NULL;
- DEBUG(SSSDBG_TRACE_ALL, "Filtered certificates:\n");
- for (cert_list_node = CERT_LIST_HEAD(cert_list);
!CERT_LIST_END(cert_list_node, cert_list);cert_list_node = CERT_LIST_NEXT(cert_list_node)) {if (cert_list_node->cert) {DEBUG(SSSDBG_TRACE_ALL, "found cert[%s][%s]\n",cert_list_node->cert->nickname,cert_list_node->cert->subjectName);if (found_cert == NULL) {found_cert = cert_list_node->cert;} else {DEBUG(SSSDBG_TRACE_ALL, "More than one certificate found, " \"using just the first one.\n");}} else {DEBUG(SSSDBG_TRACE_ALL, "--- empty cert list node ---\n");}- }
- if (found_cert == NULL) {
DEBUG(SSSDBG_TRACE_ALL, "No certificate found.\n");*cert = NULL;*token_name_out = NULL;ret = EOK;goto done;- }
- rv = CERT_VerifyCertificateNow(handle, found_cert, PR_TRUE,
certificateUsageSSLClient, NULL, NULL);Is the comment in cert_to_ssh_key() still valid since we run CERT_VerifyCertificateNow here?
yes, because they operate on different thing. Here the certificate on the Smartcard is validate. In the ssh case the certificate we got from the LDAP/IPA server is validated before the public key is converted into an ssh public key.
- if (rv != SECSuccess) {
DEBUG(SSSDBG_OP_FAILURE,"CERT_VerifyCertificateNow failed [%d].\n",PR_GetError());ret = EIO;goto done;- }
[...]
+int main(int argc, const char *argv[]) +{
- int opt;
- poptContext pc;
- int debug_fd = -1;
- errno_t ret;
- TALLOC_CTX *main_ctx = NULL;
- char *cert;
- enum op_mode mode = OP_NONE;
- enum pin_mode pin_mode = PIN_NONE;
- char *pin = NULL;
- char *slot_name_in = NULL;
- char *token_name_out = NULL;
- char *nss_db = NULL;
- struct poptOption long_options[] = {
POPT_AUTOHELP{"debug-level", 'd', POPT_ARG_INT, &debug_level, 0,_("Debug level"), NULL},{"debug-timestamps", 0, POPT_ARG_INT, &debug_timestamps, 0,_("Add debug timestamps"), NULL},{"debug-microseconds", 0, POPT_ARG_INT, &debug_microseconds, 0,_("Show timestamps with microseconds"), NULL},{"debug-fd", 0, POPT_ARG_INT, &debug_fd, 0,_("An open file descriptor for the debug logs"), NULL},{"debug-to-stderr", 0, POPT_ARG_NONE | POPT_ARGFLAG_DOC_HIDDEN,&debug_to_stderr, 0,_("Send the debug output to stderr directly."), NULL },{"auth", 0, POPT_ARG_NONE, NULL, 'a', _("Run in auth mode"), NULL},{"pre", 0, POPT_ARG_NONE, NULL, 'p', _("Run in pre-auth mode"), NULL},{"pin", 0, POPT_ARG_NONE, NULL, 'i', _("Expect PIN on stdin"), NULL},{"keypad", 0, POPT_ARG_NONE, NULL, 'k', _("Expect PIN on keypad"),NULL},{"nssdb", 0, POPT_ARG_STRING, &nss_db, 0, _("NSS DB to use"),NULL},POPT_TABLEEND- };
- /* Set debug level to invalid value so we can decide if -d 0 was used. */
- debug_level = SSSDBG_INVALID;
- pc = poptGetContext(argv[0], argc, argv, long_options, 0);
- while((opt = poptGetNextOpt(pc)) != -1) {
~~ Please put a space here.
fixed
switch(opt) {case 'a':if (mode != OP_NONE) {fprintf(stderr,"\n--auth and --pre are mutually exclusive and " \"should be only used once.\n\n");poptPrintUsage(pc, stderr, 0);_exit(-1);}mode = OP_AUTH;break;From f004acdd79dfafb3c89a57e1b11950f2f5c3345e Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Fri, 26 Jun 2015 14:45:21 +0200 Subject: [PATCH 3/7] pack_message_v3: allow empty name
ACK, we do the same for service already.
From b73cab487ffc4704239d4b5f5b63a75b43602e6b Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Fri, 26 Jun 2015 17:55:23 +0200 Subject: [PATCH 4/7] authok: add support for Smart Card related authtokens
src/sss_client/sss_cli.h | 7 ++++ src/tests/cmocka/test_authtok.c | 75 +++++++++++++++++++++++++++++++++++++++++ src/util/authtok.c | 64 +++++++++++++++++++++++++++++++++++ src/util/authtok.h | 41 ++++++++++++++++++++++
Only one typo in doxygen comments:
--- a/src/util/authtok.h +++ b/src/util/authtok.h @@ -223,4 +223,45 @@ errno_t sss_authtok_set_2fa(struct sss_auth_token *tok, errno_t sss_authtok_get_2fa(struct sss_auth_token *tok, const char **fa1, size_t *fa1_len, const char **fa2, size_t *fa2_len);
+/**
- @brief Set a Smart Card pin into a an auth token, replacing any previous data
- @param tok A pointer to a sss_auth_token structure to change, also
used as a memory context to allocate the internal data.
- @param pin A string
- @param len The length of the string or, if 0 is passed,
then strlen(password) will be used internally.
- @return EOK on success
ENOMEM on error- */
+errno_t sss_authtok_set_sc_pin(struct sss_auth_token *tok, const char *pin,
size_t len);+/**
- @brief Returns a Smart Card pin as const string if the auth token is of
type SSS_AUTHTOK_TYPE_SC_PIN, otherwise it returns an error
- @param tok A pointer to an sss_auth_token
- @param pwd A pointer to a const char *, that will point to a null
~~~ should be pin
fixed
terminated string
- @param len The length of the pin string
- @return EOK on success
ENOENT if the token is empty
EACCESS if the token is not a Smart Card pin token- */
+errno_t sss_authtok_get_sc_pin(struct sss_auth_token *tok, const char **pin,
size_t *len);+/**
- @brief Sets an auth token to type SSS_AUTHTOK_TYPE_SC_KEYPAD, replacing any
previous data
- @param tok A pointer to a sss_auth_token structure to change, also
used as a memory context to allocate the internal data.- */
+void sss_authtok_set_sc_keypad(struct sss_auth_token *tok);
#endif /* __AUTHTOK_H__ */
2.1.0
From 25897f8c5b421cb765bbfa6c47933e904e66bf06 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Fri, 10 Jul 2015 17:54:07 +0200 Subject: [PATCH 5/7] PAM: add certificate support to PAM (pre-)auth requests
Makefile.am | 5 + configure.ac | 3 + contrib/sssd.spec.in | 1 + src/confdb/confdb.h | 2 + src/responder/pam/pamsrv.c | 34 +++ src/responder/pam/pamsrv.h | 26 ++ src/responder/pam/pamsrv_cmd.c | 321 ++++++++++++++++++++--- src/responder/pam/pamsrv_p11.c | 508 ++++++++++++++++++++++++++++++++++++ src/sss_client/sss_cli.h | 1 + src/tests/cmocka/p11_nssdb/cert9.db | Bin 0 -> 13312 bytes src/tests/cmocka/p11_nssdb/key4.db | Bin 0 -> 14336 bytes src/tests/cmocka/test_pam_srv.c | 508 +++++++++++++++++++++++++++++++++++- src/util/util_errors.c | 1 + src/util/util_errors.h | 1 + 14 files changed, 1372 insertions(+), 39 deletions(-) create mode 100644 src/responder/pam/pamsrv_p11.c create mode 100644 src/tests/cmocka/p11_nssdb/cert9.db create mode 100644 src/tests/cmocka/p11_nssdb/key4.db
[...]
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index a68f1bdec917cb4af6b4fd31860e642c69ada4de..898d5c5b2d5668387586c1c42fc8d382cc6bf0c3 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -689,6 +689,7 @@ rm -rf $RPM_BUILD_ROOT %{_libexecdir}/%{servicename}/sssd_autofs %{_libexecdir}/%{servicename}/sssd_ssh %{_libexecdir}/%{servicename}/sssd_sudo +%{_libexecdir}/%{servicename}/p11_child
As said earlier, this hunk belongs to an earlier patch. Also, in Makefile.am, the permissions on p11_child are root.sssd 04750, don't we need to do the same here?
fixed
%dir %{_libdir}/%{name} %{_libdir}/%{name}/libsss_simple.so diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
[...]
diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c new file mode 100644 index 0000000000000000000000000000000000000000..bf404958340d4d155a0b60c4deda87fa26e1ad25 --- /dev/null +++ b/src/responder/pam/pamsrv_p11.c @@ -0,0 +1,508 @@ +/*
- SSSD
- PAM Responder - certificate realted requests
- Copyright (C) Sumit Bose sbose@redhat.com 2015
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <time.h>
+#include "util/util.h" +#include "providers/data_provider.h" +#include "util/child_common.h" +#include "util/strtonum.h" +#include "responder/pam/pamsrv.h"
+#ifndef SSSD_LIBEXEC_PATH +#error "SSSD_LIBEXEC_PATH not defined" +#endif /* SSSD_LIBEXEC_PATH */
+#define P11_CHILD_LOG_FILE "p11_child" +#define P11_CHILD_PATH SSSD_LIBEXEC_PATH"/p11_child"
+#define DEFAULT_NSS_DB "/etc/pki/nssdb"
We already have CONFDB_DEFAULT_SSH_CA_DB, do we need both default?
CONFDB_DEFAULT_SSH_CA_DB is an independent option which gives the DB with the CA certs to validate the certificate we get via LDAP from the IPA server before generating the ssh public keys. This can be the same as the DB which has the Smartcard configuration but is not required. But I put this default into DEFAULT_PAM_CERT_DB_PATH and removed it here.
+errno_t p11_child_init(struct pam_ctx *pctx) +{
- return child_debug_init(P11_CHILD_LOG_FILE, &pctx->p11_child_debug_fd);
+}
[...]
+errno_t pam_check_cert_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
uint8_t **buf, ssize_t *buf_len)+{
- struct pam_check_cert_state *state =
tevent_req_data(req, struct pam_check_cert_state);- TEVENT_REQ_RETURN_ON_ERROR(req);
- if (buf_len != NULL) {
*buf_len = state->len;- }
- if (buf != NULL) {
*buf = talloc_steal(mem_ctx, state->buf);- }
- return EOK;
+}
+errno_t parse_p11_child_response(TALLOC_CTX *mem_ctx, uint8_t *buf,
ssize_t buf_len, char **_cert,char **_token_name)Since this function only makes sense to parse output of pam_check_cert_recv what do you think of merging this function into pam_check_cert request and making the request return cert and token name directly?
good idea, done.
+{
- int ret;
- TALLOC_CTX *tmp_ctx = NULL;
- uint8_t *p;
- uint8_t *pn;
- char *endptr;
- size_t count = 0;
- char *cert = NULL;
- char *str;
- char *token_name = NULL;
- size_t c;
- if (buf_len < 0) {
DEBUG(SSSDBG_CRIT_FAILURE,"Error occured while reading data from p11_child.\n");return EIO;- }
- if (buf_len == 0) {
DEBUG(SSSDBG_TRACE_LIBS, "No certificate found.\n");ret = EOK;goto done;- }
- p = memchr(buf, '\n', buf_len);
- if (p == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "Missing new-line in p11_child response.\n");return EINVAL;- }
- if (p == buf) {
DEBUG(SSSDBG_OP_FAILURE, "Missing counter in p11_child response.\n");return EINVAL;- }
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");return ENOMEM;- }
- token_name = talloc_strndup(tmp_ctx, (char*) buf, (p - buf));
- if (token_name == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_strndup failed.\n");return ENOMEM;we should goto done here to get rid of tmp_ctx
done
- }
- p++;
- pn = memchr(p, '\n', buf_len - (p - buf));
- if (pn == NULL) {
DEBUG(SSSDBG_OP_FAILURE,"Missing new-line in p11_child response.\n");ret = EINVAL;goto done;- }
- if (pn == p) {
DEBUG(SSSDBG_OP_FAILURE, "Missing cert in p11_child response.\n");ret = EINVAL;goto done;- }
- cert = talloc_strndup(tmp_ctx, (char *) p, (pn - p));
- if(cert == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_strndup failed.\n");ret = ENOMEM;goto done;- }
- DEBUG(SSSDBG_TRACE_ALL, "Found cert [%s].\n", cert);
- ret = EOK;
+done:
- if (ret == EOK) {
*_token_name = talloc_steal(mem_ctx, token_name);*_cert = talloc_steal(mem_ctx, cert);- }
- talloc_free(tmp_ctx);
- return ret;
+}
From 7d505b88cd615db4e5318d01327794e3f81b7e4c Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Fri, 3 Jul 2015 14:05:11 +0200 Subject: [PATCH 6/7] pam_sss: add sc support
ACK
From f6d366ebd1307dfd36a907e55f32a66caf2b0f9d Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Wed, 15 Jul 2015 09:40:00 +0200 Subject: [PATCH 7/7] ssh: generate public keys from certificate
Resolves: https://fedorahosted.org/sssd/ticket/2711
src/confdb/confdb.h | 2 + src/config/SSSDConfig/__init__.py.in | 1 + src/config/etc/sssd.api.conf | 1 + src/man/sssd.conf.5.xml | 13 ++++ src/responder/ssh/sshsrv.c | 9 +++ src/responder/ssh/sshsrv_cmd.c | 54 ++++++++++++--- src/responder/ssh/sshsrv_private.h | 1 + src/tests/cmocka/test_cert_utils.c | 60 +++++++++++++++++ src/util/cert.h | 4 ++ src/util/cert/nss/cert.c | 125 +++++++++++++++++++++++++++++++++++ 10 files changed, 259 insertions(+), 11 deletions(-)
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index 254246a4c26c3cdb0b57ab987a53ebddea771218..2d7832e40fd0c9ad1ea94b86dd257fc87186d39a 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -135,6 +135,8 @@ #define CONFDB_DEFAULT_SSH_HASH_KNOWN_HOSTS true #define CONFDB_SSH_KNOWN_HOSTS_TIMEOUT "ssh_known_hosts_timeout" #define CONFDB_DEFAULT_SSH_KNOWN_HOSTS_TIMEOUT 180 +#define CONFDB_SSH_CA_DB "ca_db" +#define CONFDB_DEFAULT_SSH_CA_DB "/etc/pki/nssdb"
I wonder if we should use sysconfigdir here, because the default paths if you use configure will be /usr/etc/
But I know this is a bit academic on modern systems..
fixed
[...]
--- a/src/responder/ssh/sshsrv_cmd.c +++ b/src/responder/ssh/sshsrv_cmd.c @@ -27,6 +27,7 @@ #include "util/util.h" #include "util/crypto/sss_crypto.h" #include "util/sss_ssh.h" +#include "util/cert.h" #include "db/sysdb.h" #include "db/sysdb_ssh.h" #include "providers/data_provider.h" @@ -219,7 +220,8 @@ static errno_t ssh_user_pubkeys_search_next(struct ssh_cmd_ctx *cmd_ctx) { errno_t ret;
- const char *attrs[] = { SYSDB_NAME, SYSDB_SSH_PUBKEY, NULL };
const char *attrs[] = { SYSDB_NAME, SYSDB_SSH_PUBKEY, SYSDB_USER_CERT,
NULL };struct ldb_result *res;
DEBUG(SSSDBG_TRACE_FUNC,
@@ -794,6 +796,8 @@ ssh_cmd_parse_request(struct ssh_cmd_ctx *cmd_ctx)
static errno_t decode_and_add_base64_data(struct ssh_cmd_ctx *cmd_ctx, struct ldb_message_element *el,
bool cert_data,Do we anticipate any more formats? If yes, then let's add an enum here, otherwise the bool is fine.
no, but time will tell :-) So far I kept the bool.
struct ssh_ctx *ssh_ctx, size_t fqname_len, const char *fqname, size_t *c)
new versions attached.
bye, Sumit
On Tue, Jul 28, 2015 at 10:43:13PM +0200, Sumit Bose wrote:
Hi,
I started the review, but because the patches are quite big, I will send my comments in batches. I hope that's fine.
Thank you for the review.
CI failed in distcheck.
The failure was due to distcheck's tendency to make the complete source tree read-only and NSS's tendency open all databases in read-write more. I added the needed read-onl options to the NSS code.
Coverity found some issues that I forwarded to Sumit.
Thank you, I fixed them and hopefully did not create new ones.
Coverity is clean this time, but CI did not pass on Debian due to build failure of a test: http://sssd-ci.duckdns.org/logs/job/19/78/debian_testing/ci-build-debug/ci-m... Normally this is caused by a missing dependency, which the Fedora/RHEL linker can satisfy.
I will review the rest of the patches.
On Wed, Jul 29, 2015 at 03:34:32PM +0200, Jakub Hrozek wrote:
On Tue, Jul 28, 2015 at 10:43:13PM +0200, Sumit Bose wrote:
Hi,
I started the review, but because the patches are quite big, I will send my comments in batches. I hope that's fine.
Thank you for the review.
CI failed in distcheck.
The failure was due to distcheck's tendency to make the complete source tree read-only and NSS's tendency open all databases in read-write more. I added the needed read-onl options to the NSS code.
Coverity found some issues that I forwarded to Sumit.
Thank you, I fixed them and hopefully did not create new ones.
Coverity is clean this time, but CI did not pass on Debian due to build failure of a test: http://sssd-ci.duckdns.org/logs/job/19/78/debian_testing/ci-build-debug/ci-m... Normally this is caused by a missing dependency, which the Fedora/RHEL linker can satisfy.
I will review the rest of the patches.
I don't have any other comments. I tested authentication, password change and OTP authentication to exercise the PAM responder and pam_sss a bit and didn't find any issues.
So ACK except the test build issue. Please let me know if you'd like me to help with that one.
On Thu, Jul 30, 2015 at 11:10:33AM +0200, Jakub Hrozek wrote:
On Wed, Jul 29, 2015 at 03:34:32PM +0200, Jakub Hrozek wrote:
On Tue, Jul 28, 2015 at 10:43:13PM +0200, Sumit Bose wrote:
Hi,
I started the review, but because the patches are quite big, I will send my comments in batches. I hope that's fine.
Thank you for the review.
CI failed in distcheck.
The failure was due to distcheck's tendency to make the complete source tree read-only and NSS's tendency open all databases in read-write more. I added the needed read-onl options to the NSS code.
Coverity found some issues that I forwarded to Sumit.
Thank you, I fixed them and hopefully did not create new ones.
Coverity is clean this time, but CI did not pass on Debian due to build failure of a test: http://sssd-ci.duckdns.org/logs/job/19/78/debian_testing/ci-build-debug/ci-m... Normally this is caused by a missing dependency, which the Fedora/RHEL linker can satisfy.
Please find attached a new set which should fix the debian build (only the last patch changed). As you suspected it was a missing dependency in Makefile.am. But this make me realize that I didn't implement a OpenSSL/libcrypto version of cert_to_ssh_key() which would break the OpenSSL build. I added an initial version without certificate validation. If you don't like it it can simply replaced by 'return ENOSUPP'.
bye, Sumit
I will review the rest of the patches.
I don't have any other comments. I tested authentication, password change and OTP authentication to exercise the PAM responder and pam_sss a bit and didn't find any issues.
So ACK except the test build issue. Please let me know if you'd like me to help with that one. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, Jul 30, 2015 at 05:24:27PM +0200, Sumit Bose wrote:
On Thu, Jul 30, 2015 at 11:10:33AM +0200, Jakub Hrozek wrote:
On Wed, Jul 29, 2015 at 03:34:32PM +0200, Jakub Hrozek wrote:
On Tue, Jul 28, 2015 at 10:43:13PM +0200, Sumit Bose wrote:
Hi,
I started the review, but because the patches are quite big, I will send my comments in batches. I hope that's fine.
Thank you for the review.
CI failed in distcheck.
The failure was due to distcheck's tendency to make the complete source tree read-only and NSS's tendency open all databases in read-write more. I added the needed read-onl options to the NSS code.
Coverity found some issues that I forwarded to Sumit.
Thank you, I fixed them and hopefully did not create new ones.
Coverity is clean this time, but CI did not pass on Debian due to build failure of a test: http://sssd-ci.duckdns.org/logs/job/19/78/debian_testing/ci-build-debug/ci-m... Normally this is caused by a missing dependency, which the Fedora/RHEL linker can satisfy.
Please find attached a new set which should fix the debian build (only the last patch changed). As you suspected it was a missing dependency in Makefile.am. But this make me realize that I didn't implement a OpenSSL/libcrypto version of cert_to_ssh_key() which would break the OpenSSL build. I added an initial version without certificate validation. If you don't like it it can simply replaced by 'return ENOSUPP'.
I'm fine with the code as long as you keep the TODOs at least in some todo list of yours.
ACK code wise, also my regression tests went fine.
I'm only waiting for the automated tests to finish.
On Thu, Jul 30, 2015 at 09:27:31PM +0200, Jakub Hrozek wrote:
On Thu, Jul 30, 2015 at 05:24:27PM +0200, Sumit Bose wrote:
On Thu, Jul 30, 2015 at 11:10:33AM +0200, Jakub Hrozek wrote:
On Wed, Jul 29, 2015 at 03:34:32PM +0200, Jakub Hrozek wrote:
On Tue, Jul 28, 2015 at 10:43:13PM +0200, Sumit Bose wrote:
Hi,
I started the review, but because the patches are quite big, I will send my comments in batches. I hope that's fine.
Thank you for the review.
CI failed in distcheck.
The failure was due to distcheck's tendency to make the complete source tree read-only and NSS's tendency open all databases in read-write more. I added the needed read-onl options to the NSS code.
Coverity found some issues that I forwarded to Sumit.
Thank you, I fixed them and hopefully did not create new ones.
Coverity is clean this time, but CI did not pass on Debian due to build failure of a test: http://sssd-ci.duckdns.org/logs/job/19/78/debian_testing/ci-build-debug/ci-m... Normally this is caused by a missing dependency, which the Fedora/RHEL linker can satisfy.
Please find attached a new set which should fix the debian build (only the last patch changed). As you suspected it was a missing dependency in Makefile.am. But this make me realize that I didn't implement a OpenSSL/libcrypto version of cert_to_ssh_key() which would break the OpenSSL build. I added an initial version without certificate validation. If you don't like it it can simply replaced by 'return ENOSUPP'.
I'm fine with the code as long as you keep the TODOs at least in some todo list of yours.
ACK code wise, also my regression tests went fine.
I'm only waiting for the automated tests to finish.
CI passed, the rawhide failure is unrelated: http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/2015/
On Fri, Jul 31, 2015 at 09:28:13AM +0200, Jakub Hrozek wrote:
On Thu, Jul 30, 2015 at 09:27:31PM +0200, Jakub Hrozek wrote:
On Thu, Jul 30, 2015 at 05:24:27PM +0200, Sumit Bose wrote:
On Thu, Jul 30, 2015 at 11:10:33AM +0200, Jakub Hrozek wrote:
On Wed, Jul 29, 2015 at 03:34:32PM +0200, Jakub Hrozek wrote:
On Tue, Jul 28, 2015 at 10:43:13PM +0200, Sumit Bose wrote:
> > Hi, > > I started the review, but because the patches are quite big, I will send > my comments in batches. I hope that's fine.
Thank you for the review.
> > CI failed in distcheck.
The failure was due to distcheck's tendency to make the complete source tree read-only and NSS's tendency open all databases in read-write more. I added the needed read-onl options to the NSS code.
> > Coverity found some issues that I forwarded to Sumit.
Thank you, I fixed them and hopefully did not create new ones.
Coverity is clean this time, but CI did not pass on Debian due to build failure of a test: http://sssd-ci.duckdns.org/logs/job/19/78/debian_testing/ci-build-debug/ci-m... Normally this is caused by a missing dependency, which the Fedora/RHEL linker can satisfy.
Please find attached a new set which should fix the debian build (only the last patch changed). As you suspected it was a missing dependency in Makefile.am. But this make me realize that I didn't implement a OpenSSL/libcrypto version of cert_to_ssh_key() which would break the OpenSSL build. I added an initial version without certificate validation. If you don't like it it can simply replaced by 'return ENOSUPP'.
I'm fine with the code as long as you keep the TODOs at least in some todo list of yours.
ACK code wise, also my regression tests went fine.
I'm only waiting for the automated tests to finish.
CI passed, the rawhide failure is unrelated: http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/2015/
Pushed to master: * 4de84af23db74e13e867985c9093f394c9fa8d51 * 5242964d275d0b2e96c9b0d1f8a9958c85d566fc * a8d887323f83984679a7d9b827a70146656bb7b2 * 10703cd558016685ee778e333f1d4490238d46e7 * 35f3a213e0f0f2c60e9b5f095a05388e21092ae2 * 45726939a48e605b0166521f94300ae04981a3a7 * 0d5bb38364a6976e9c85d6349aa13a04d181a090
On Fri, Jul 17, 2015 at 09:01:49PM +0200, Sumit Bose wrote:
On Fri, Jul 10, 2015 at 06:40:30PM +0200, Sumit Bose wrote:
Hi,
this is the initial version of my patch which add Smartcard authentication to SSSD. I'm still working on a design page which will explain everything in more details so I will only add a short version here.
The main job will be done by a new child process called p11_child. Since the Smartcard support in GDM is based on NSS I used NSS for the first version of p11_child as well. But since all PKCS#11 (API to talk to Smartcards) related code is in this child process adding support for other PKCS#11 frameworks like p11-kit would be straight forward (in fact I already started on the p11-kit version). Using NSS here means you have to add the PKCS#11 module for your Smartcards reader to /etc/pki/nssdb (the NSS DB GDM uses as well) with modutil or pk11install from the coolkey package.
The PAM configuration so far must not be changed. pam_sss will do a pre-auth request similar to the OPT case for find a suitable authentication method for the user. The pam responder then checks is Smartcard authentication is enabled (pam_cert_auth = True in the [pam] section of sssd.conf), if the service is a local one and if there if a valid certificate can be found which is available in the users LDAP entry as well. If all this checks pass pam_sss will ask the user for a PIN and then SSSD tries to validate that PIN, public and private keys all relate to each other. If no Smartcard is found for the user the standard password prompt is displayed.
With some valuable input form Christian Heimes I think I found a way to test the Smartcard support even without real hardware but I still have to work out some of the details. I will add instructions to the design page and better and more unit tests.
Any comments and suggestions are welcome.
Please find attached an improved version of the patches. Especially there are improvements to the test, the return values from the p11_child are not mocked used the actual retrieved certificate data. The test data causes some increase in the patch size. I plan to replace this with data which is generate during the test run but for a start it is easier this way.
I also added a 7th patch which should resolve https://fedorahosted.org/sssd/ticket/2711 (SSH with certificates). Strictly it is not related to Smartcard authentication via PAM but it depends on the NSS version of the cert utilities (patch 0001) so I included it here as well.
On the design page I added the 'How to test' section https://fedorahosted.org/sssd/wiki/DesignDocs/SmartcardAuthenticationStep1#H... which hopefully gives sufficient details how to set up a test environment.
bye, Sumit
From b73cab487ffc4704239d4b5f5b63a75b43602e6b Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Fri, 26 Jun 2015 17:55:23 +0200 Subject: [PATCH 4/7] authok: add support for Smart Card related authtokens
src/sss_client/sss_cli.h | 7 ++++ src/tests/cmocka/test_authtok.c | 75 +++++++++++++++++++++++++++++++++++++++++ src/util/authtok.c | 64 +++++++++++++++++++++++++++++++++++ src/util/authtok.h | 41 ++++++++++++++++++++++
Only one typo in doxygen comments:
--- a/src/util/authtok.h +++ b/src/util/authtok.h @@ -223,4 +223,45 @@ errno_t sss_authtok_set_2fa(struct sss_auth_token *tok, errno_t sss_authtok_get_2fa(struct sss_auth_token *tok, const char **fa1, size_t *fa1_len, const char **fa2, size_t *fa2_len);
+/**
- @brief Set a Smart Card pin into a an auth token, replacing any previous data
- @param tok A pointer to a sss_auth_token structure to change, also
used as a memory context to allocate the internal data.
- @param pin A string
- @param len The length of the string or, if 0 is passed,
then strlen(password) will be used internally.
- @return EOK on success
ENOMEM on error- */
+errno_t sss_authtok_set_sc_pin(struct sss_auth_token *tok, const char *pin,
size_t len);+/**
- @brief Returns a Smart Card pin as const string if the auth token is of
type SSS_AUTHTOK_TYPE_SC_PIN, otherwise it returns an error
- @param tok A pointer to an sss_auth_token
- @param pwd A pointer to a const char *, that will point to a null
~~~ should be pin
terminated string
- @param len The length of the pin string
- @return EOK on success
ENOENT if the token is empty
EACCESS if the token is not a Smart Card pin token- */
+errno_t sss_authtok_get_sc_pin(struct sss_auth_token *tok, const char **pin,
size_t *len);+/**
- @brief Sets an auth token to type SSS_AUTHTOK_TYPE_SC_KEYPAD, replacing any
previous data
- @param tok A pointer to a sss_auth_token structure to change, also
used as a memory context to allocate the internal data.- */
+void sss_authtok_set_sc_keypad(struct sss_auth_token *tok);
#endif /* __AUTHTOK_H__ */
2.1.0
From 25897f8c5b421cb765bbfa6c47933e904e66bf06 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Fri, 10 Jul 2015 17:54:07 +0200 Subject: [PATCH 5/7] PAM: add certificate support to PAM (pre-)auth requests
Makefile.am | 5 + configure.ac | 3 + contrib/sssd.spec.in | 1 + src/confdb/confdb.h | 2 + src/responder/pam/pamsrv.c | 34 +++ src/responder/pam/pamsrv.h | 26 ++ src/responder/pam/pamsrv_cmd.c | 321 ++++++++++++++++++++--- src/responder/pam/pamsrv_p11.c | 508 ++++++++++++++++++++++++++++++++++++ src/sss_client/sss_cli.h | 1 + src/tests/cmocka/p11_nssdb/cert9.db | Bin 0 -> 13312 bytes src/tests/cmocka/p11_nssdb/key4.db | Bin 0 -> 14336 bytes src/tests/cmocka/test_pam_srv.c | 508 +++++++++++++++++++++++++++++++++++- src/util/util_errors.c | 1 + src/util/util_errors.h | 1 + 14 files changed, 1372 insertions(+), 39 deletions(-) create mode 100644 src/responder/pam/pamsrv_p11.c create mode 100644 src/tests/cmocka/p11_nssdb/cert9.db create mode 100644 src/tests/cmocka/p11_nssdb/key4.db
[...]
diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in index a68f1bdec917cb4af6b4fd31860e642c69ada4de..898d5c5b2d5668387586c1c42fc8d382cc6bf0c3 100644 --- a/contrib/sssd.spec.in +++ b/contrib/sssd.spec.in @@ -689,6 +689,7 @@ rm -rf $RPM_BUILD_ROOT %{_libexecdir}/%{servicename}/sssd_autofs %{_libexecdir}/%{servicename}/sssd_ssh %{_libexecdir}/%{servicename}/sssd_sudo +%{_libexecdir}/%{servicename}/p11_child
As said earlier, this hunk belongs to an earlier patch. Also, in Makefile.am, the permissions on p11_child are root.sssd 04750, don't we need to do the same here?
%dir %{_libdir}/%{name} %{_libdir}/%{name}/libsss_simple.so diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
[...]
diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c new file mode 100644 index 0000000000000000000000000000000000000000..bf404958340d4d155a0b60c4deda87fa26e1ad25 --- /dev/null +++ b/src/responder/pam/pamsrv_p11.c @@ -0,0 +1,508 @@ +/*
- SSSD
- PAM Responder - certificate realted requests
- Copyright (C) Sumit Bose sbose@redhat.com 2015
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <time.h>
+#include "util/util.h" +#include "providers/data_provider.h" +#include "util/child_common.h" +#include "util/strtonum.h" +#include "responder/pam/pamsrv.h"
+#ifndef SSSD_LIBEXEC_PATH +#error "SSSD_LIBEXEC_PATH not defined" +#endif /* SSSD_LIBEXEC_PATH */
+#define P11_CHILD_LOG_FILE "p11_child" +#define P11_CHILD_PATH SSSD_LIBEXEC_PATH"/p11_child"
+#define DEFAULT_NSS_DB "/etc/pki/nssdb"
We already have CONFDB_DEFAULT_SSH_CA_DB, do we need both default?
+errno_t p11_child_init(struct pam_ctx *pctx) +{
- return child_debug_init(P11_CHILD_LOG_FILE, &pctx->p11_child_debug_fd);
+}
[...]
+errno_t pam_check_cert_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
uint8_t **buf, ssize_t *buf_len)+{
- struct pam_check_cert_state *state =
tevent_req_data(req, struct pam_check_cert_state);- TEVENT_REQ_RETURN_ON_ERROR(req);
- if (buf_len != NULL) {
*buf_len = state->len;- }
- if (buf != NULL) {
*buf = talloc_steal(mem_ctx, state->buf);- }
- return EOK;
+}
+errno_t parse_p11_child_response(TALLOC_CTX *mem_ctx, uint8_t *buf,
ssize_t buf_len, char **_cert,char **_token_name)
Since this function only makes sense to parse output of pam_check_cert_recv what do you think of merging this function into pam_check_cert request and making the request return cert and token name directly?
+{
- int ret;
- TALLOC_CTX *tmp_ctx = NULL;
- uint8_t *p;
- uint8_t *pn;
- char *endptr;
- size_t count = 0;
- char *cert = NULL;
- char *str;
- char *token_name = NULL;
- size_t c;
- if (buf_len < 0) {
DEBUG(SSSDBG_CRIT_FAILURE,"Error occured while reading data from p11_child.\n");return EIO;- }
- if (buf_len == 0) {
DEBUG(SSSDBG_TRACE_LIBS, "No certificate found.\n");ret = EOK;goto done;- }
- p = memchr(buf, '\n', buf_len);
- if (p == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "Missing new-line in p11_child response.\n");return EINVAL;- }
- if (p == buf) {
DEBUG(SSSDBG_OP_FAILURE, "Missing counter in p11_child response.\n");return EINVAL;- }
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");return ENOMEM;- }
- token_name = talloc_strndup(tmp_ctx, (char*) buf, (p - buf));
- if (token_name == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_strndup failed.\n");return ENOMEM;
we should goto done here to get rid of tmp_ctx
- }
- p++;
- pn = memchr(p, '\n', buf_len - (p - buf));
- if (pn == NULL) {
DEBUG(SSSDBG_OP_FAILURE,"Missing new-line in p11_child response.\n");ret = EINVAL;goto done;- }
- if (pn == p) {
DEBUG(SSSDBG_OP_FAILURE, "Missing cert in p11_child response.\n");ret = EINVAL;goto done;- }
- cert = talloc_strndup(tmp_ctx, (char *) p, (pn - p));
- if(cert == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_strndup failed.\n");ret = ENOMEM;goto done;- }
- DEBUG(SSSDBG_TRACE_ALL, "Found cert [%s].\n", cert);
- ret = EOK;
+done:
- if (ret == EOK) {
*_token_name = talloc_steal(mem_ctx, token_name);*_cert = talloc_steal(mem_ctx, cert);- }
- talloc_free(tmp_ctx);
- return ret;
+}
From 7d505b88cd615db4e5318d01327794e3f81b7e4c Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Fri, 3 Jul 2015 14:05:11 +0200 Subject: [PATCH 6/7] pam_sss: add sc support
ACK
From f6d366ebd1307dfd36a907e55f32a66caf2b0f9d Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Wed, 15 Jul 2015 09:40:00 +0200 Subject: [PATCH 7/7] ssh: generate public keys from certificate
Resolves: https://fedorahosted.org/sssd/ticket/2711
src/confdb/confdb.h | 2 + src/config/SSSDConfig/__init__.py.in | 1 + src/config/etc/sssd.api.conf | 1 + src/man/sssd.conf.5.xml | 13 ++++ src/responder/ssh/sshsrv.c | 9 +++ src/responder/ssh/sshsrv_cmd.c | 54 ++++++++++++--- src/responder/ssh/sshsrv_private.h | 1 + src/tests/cmocka/test_cert_utils.c | 60 +++++++++++++++++ src/util/cert.h | 4 ++ src/util/cert/nss/cert.c | 125 +++++++++++++++++++++++++++++++++++ 10 files changed, 259 insertions(+), 11 deletions(-)
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index 254246a4c26c3cdb0b57ab987a53ebddea771218..2d7832e40fd0c9ad1ea94b86dd257fc87186d39a 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -135,6 +135,8 @@ #define CONFDB_DEFAULT_SSH_HASH_KNOWN_HOSTS true #define CONFDB_SSH_KNOWN_HOSTS_TIMEOUT "ssh_known_hosts_timeout" #define CONFDB_DEFAULT_SSH_KNOWN_HOSTS_TIMEOUT 180 +#define CONFDB_SSH_CA_DB "ca_db" +#define CONFDB_DEFAULT_SSH_CA_DB "/etc/pki/nssdb"
I wonder if we should use sysconfigdir here, because the default paths if you use configure will be /usr/etc/
But I know this is a bit academic on modern systems..
[...]
--- a/src/responder/ssh/sshsrv_cmd.c +++ b/src/responder/ssh/sshsrv_cmd.c @@ -27,6 +27,7 @@ #include "util/util.h" #include "util/crypto/sss_crypto.h" #include "util/sss_ssh.h" +#include "util/cert.h" #include "db/sysdb.h" #include "db/sysdb_ssh.h" #include "providers/data_provider.h" @@ -219,7 +220,8 @@ static errno_t ssh_user_pubkeys_search_next(struct ssh_cmd_ctx *cmd_ctx) { errno_t ret;
- const char *attrs[] = { SYSDB_NAME, SYSDB_SSH_PUBKEY, NULL };
const char *attrs[] = { SYSDB_NAME, SYSDB_SSH_PUBKEY, SYSDB_USER_CERT,
NULL };struct ldb_result *res;
DEBUG(SSSDBG_TRACE_FUNC,
@@ -794,6 +796,8 @@ ssh_cmd_parse_request(struct ssh_cmd_ctx *cmd_ctx)
static errno_t decode_and_add_base64_data(struct ssh_cmd_ctx *cmd_ctx, struct ldb_message_element *el,
bool cert_data,
Do we anticipate any more formats? If yes, then let's add an enum here, otherwise the bool is fine.
struct ssh_ctx *ssh_ctx, size_t fqname_len, const char *fqname, size_t *c)
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Roland Mainz -
Sumit Bose