sumit-bose's pull request #17: "Improve support for gdm Smartcard support" was opened
PR body: """ Those two patches try to fix two issues related to the Smartcard handling feature of gdm. The first fixes https://fedorahosted.org/sssd/ticket/3165 and the second fixes an issue which was introduced by the sysdb refactoring. """ See the full pull-request at https://github.com/SSSD/sssd/pull/17 ... or pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/17/head:pr17 git checkout pr17
From 905e3ef9a3fcd4b75e87435a3d37303d100739f5 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Wed, 31 Aug 2016 14:32:31 +0200 Subject: [PATCH 1/2] p11: only set PKCS11_LOGIN_TOKEN_NAME if gdm-smartcard is used Resolves https://fedorahosted.org/sssd/ticket/3165 --- src/responder/pam/pamsrv_p11.c | 33 +++++++++------ src/tests/cmocka/test_pam_srv.c | 89 +++++++++++++++++++++++++++++++++++------ 2 files changed, 97 insertions(+), 25 deletions(-) diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c index a2514f6..22da330 100644 --- a/src/responder/pam/pamsrv_p11.c +++ b/src/responder/pam/pamsrv_p11.c @@ -505,7 +505,11 @@ errno_t pam_check_cert_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx, } /* The PKCS11_LOGIN_TOKEN_NAME environment variable is e.g. used by the Gnome - * Settings Daemon to determine the name of the token used for login */ + * Settings Daemon to determine the name of the token used for login but it + * should be only set if SSSD is called by gdm-smartcard. Otherwise desktop + * components might assume that gdm-smartcard PAM stack is configured + * correctly which might not be the case e.g. if Smartcard authentication was + * used when running gdm-password. */ #define PKCS11_LOGIN_TOKEN_ENV_NAME "PKCS11_LOGIN_TOKEN_NAME" errno_t add_pam_cert_response(struct pam_data *pd, const char *sysdb_username, @@ -553,19 +557,22 @@ errno_t add_pam_cert_response(struct pam_data *pd, const char *sysdb_username, return ret; } - env = talloc_asprintf(pd, "%s=%s", PKCS11_LOGIN_TOKEN_ENV_NAME, token_name); - if (env == NULL) { - DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n"); - return ENOMEM; - } + if (strcmp(pd->service, "gdm-smartcard") == 0) { + env = talloc_asprintf(pd, "%s=%s", PKCS11_LOGIN_TOKEN_ENV_NAME, + token_name); + if (env == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n"); + return ENOMEM; + } - ret = pam_add_response(pd, SSS_PAM_ENV_ITEM, strlen(env) + 1, - (uint8_t *)env); - talloc_free(env); - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, - "pam_add_response failed to add environment variable.\n"); - return ret; + ret = pam_add_response(pd, SSS_PAM_ENV_ITEM, strlen(env) + 1, + (uint8_t *)env); + talloc_free(env); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "pam_add_response failed to add environment variable.\n"); + return ret; + } } return ret; diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c index 5de092d..02199e6 100644 --- a/src/tests/cmocka/test_pam_srv.c +++ b/src/tests/cmocka/test_pam_srv.c @@ -554,7 +554,7 @@ static void mock_input_pam(TALLOC_CTX *mem_ctx, const char *name, } static void mock_input_pam_cert(TALLOC_CTX *mem_ctx, const char *name, - const char *pin) + const char *pin, const char *service) { size_t buf_size; uint8_t *m_buf; @@ -576,7 +576,7 @@ static void mock_input_pam_cert(TALLOC_CTX *mem_ctx, const char *name, pi.pam_authtok_type = SSS_AUTHTOK_TYPE_SC_PIN; } - pi.pam_service = "login"; + pi.pam_service = service == NULL ? "login" : service; pi.pam_service_size = strlen(pi.pam_service) + 1; pi.pam_tty = "/dev/tty"; pi.pam_tty_size = strlen(pi.pam_tty) + 1; @@ -626,7 +626,8 @@ static int test_pam_simple_check(uint32_t status, uint8_t *body, size_t blen) #define PKCS11_LOGIN_TOKEN_ENV_NAME "PKCS11_LOGIN_TOKEN_NAME" -static int test_pam_cert_check(uint32_t status, uint8_t *body, size_t blen) +static int test_pam_cert_check_gdm_smartcard(uint32_t status, uint8_t *body, + size_t blen) { size_t rp = 0; uint32_t val; @@ -675,6 +676,44 @@ static int test_pam_cert_check(uint32_t status, uint8_t *body, size_t blen) return EOK; } +static int test_pam_cert_check(uint32_t status, uint8_t *body, size_t blen) +{ + size_t rp = 0; + uint32_t val; + + assert_int_equal(status, 0); + + SAFEALIGN_COPY_UINT32(&val, body + rp, &rp); + assert_int_equal(val, pam_test_ctx->exp_pam_status); + + SAFEALIGN_COPY_UINT32(&val, body + rp, &rp); + assert_int_equal(val, 2); + + SAFEALIGN_COPY_UINT32(&val, body + rp, &rp); + assert_int_equal(val, SSS_PAM_DOMAIN_NAME); + + SAFEALIGN_COPY_UINT32(&val, body + rp, &rp); + assert_int_equal(val, 9); + + assert_int_equal(*(body + rp + val - 1), 0); + assert_string_equal(body + rp, TEST_DOM_NAME); + rp += val; + + SAFEALIGN_COPY_UINT32(&val, body + rp, &rp); + assert_int_equal(val, SSS_PAM_CERT_INFO); + + SAFEALIGN_COPY_UINT32(&val, body + rp, &rp); + assert_int_equal(val, (sizeof("pamuser") + sizeof(TEST_TOKEN_NAME))); + + assert_int_equal(*(body + rp + sizeof("pamuser") - 1), 0); + assert_string_equal(body + rp, "pamuser"); + rp += sizeof("pamuser"); + + assert_int_equal(*(body + rp + sizeof(TEST_TOKEN_NAME) - 1), 0); + assert_string_equal(body + rp, TEST_TOKEN_NAME); + + return EOK; +} static int test_pam_offline_chauthtok_check(uint32_t status, uint8_t *body, size_t blen) @@ -1438,7 +1477,7 @@ void test_pam_preauth_no_logon_name(void **state) { int ret; - mock_input_pam_cert(pam_test_ctx, NULL, NULL); + mock_input_pam_cert(pam_test_ctx, NULL, NULL, NULL); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -1465,7 +1504,7 @@ void test_pam_preauth_cert_nocert(void **state) set_cert_auth_param(pam_test_ctx->pctx, "/no/path"); - mock_input_pam_cert(pam_test_ctx, "pamuser", NULL); + mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, NULL); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -1544,7 +1583,7 @@ void test_pam_preauth_cert_nomatch(void **state) set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); - mock_input_pam_cert(pam_test_ctx, "pamuser", NULL); + mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, NULL); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -1566,7 +1605,7 @@ void test_pam_preauth_cert_match(void **state) set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); - mock_input_pam_cert(pam_test_ctx, "pamuser", NULL); + mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, NULL); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -1583,13 +1622,37 @@ void test_pam_preauth_cert_match(void **state) assert_int_equal(ret, EOK); } +/* Test if PKCS11_LOGIN_TOKEN_NAME is added for the gdm-smartcard service */ +void test_pam_preauth_cert_match_gdm_smartcard(void **state) +{ + int ret; + + set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); + + mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, "gdm-smartcard"); + + will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); + will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); + mock_account_recv(0, 0, NULL, test_lookup_by_cert_cb, + discard_const(TEST_TOKEN_CERT)); + + set_cmd_cb(test_pam_cert_check_gdm_smartcard); + ret = sss_cmd_execute(pam_test_ctx->cctx, SSS_PAM_PREAUTH, + pam_test_ctx->pam_cmds); + assert_int_equal(ret, EOK); + + /* Wait until the test finishes with EOK */ + ret = test_ev_loop(pam_test_ctx->tctx); + assert_int_equal(ret, EOK); +} + void test_pam_preauth_cert_match_wrong_user(void **state) { int ret; set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); - mock_input_pam_cert(pam_test_ctx, "pamuser", NULL); + mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, NULL); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -1613,7 +1676,7 @@ void test_pam_preauth_cert_no_logon_name(void **state) set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); - mock_input_pam_cert(pam_test_ctx, NULL, NULL); + mock_input_pam_cert(pam_test_ctx, NULL, NULL, NULL); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -1636,7 +1699,7 @@ void test_pam_preauth_no_cert_no_logon_name(void **state) set_cert_auth_param(pam_test_ctx->pctx, "/no/path"); - mock_input_pam_cert(pam_test_ctx, NULL, NULL); + mock_input_pam_cert(pam_test_ctx, NULL, NULL, NULL); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -1657,7 +1720,7 @@ void test_pam_preauth_cert_no_logon_name_no_match(void **state) set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); - mock_input_pam_cert(pam_test_ctx, NULL, NULL); + mock_input_pam_cert(pam_test_ctx, NULL, NULL, NULL); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -1679,7 +1742,7 @@ void test_pam_cert_auth(void **state) set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); - mock_input_pam_cert(pam_test_ctx, "pamuser", "123456"); + mock_input_pam_cert(pam_test_ctx, "pamuser", "123456", NULL); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_AUTHENTICATE); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -1790,6 +1853,8 @@ int main(int argc, const char *argv[]) pam_test_setup, pam_test_teardown), cmocka_unit_test_setup_teardown(test_pam_preauth_cert_match, pam_test_setup, pam_test_teardown), + cmocka_unit_test_setup_teardown(test_pam_preauth_cert_match_gdm_smartcard, + pam_test_setup, pam_test_teardown), cmocka_unit_test_setup_teardown(test_pam_preauth_cert_match_wrong_user, pam_test_setup, pam_test_teardown), cmocka_unit_test_setup_teardown(test_pam_preauth_cert_no_logon_name, From 0f1801b56e97ada0f949c4fe29afcc8bcb2713ad Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Thu, 1 Sep 2016 12:40:27 +0200 Subject: [PATCH 2/2] p11: return a fully-qualified name Related to https://fedorahosted.org/sssd/ticket/3165 --- src/responder/pam/pamsrv_p11.c | 20 +++++++++----------- src/tests/cmocka/test_pam_srv.c | 16 ++++++++-------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c index 22da330..570bfe0 100644 --- a/src/responder/pam/pamsrv_p11.c +++ b/src/responder/pam/pamsrv_p11.c @@ -521,33 +521,31 @@ errno_t add_pam_cert_response(struct pam_data *pd, const char *sysdb_username, size_t msg_len; size_t slot_len; int ret; - char *username; if (sysdb_username == NULL || token_name == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "Missing mandatory user or slot name.\n"); return EINVAL; } - ret = sss_parse_internal_fqname(pd, sysdb_username, &username, NULL); - if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, "Cannot parse [%s]\n", sysdb_username); - return ret; - } - - user_len = strlen(username) + 1; + user_len = strlen(sysdb_username) + 1; slot_len = strlen(token_name) + 1; msg_len = user_len + slot_len; msg = talloc_zero_size(pd, msg_len); if (msg == NULL) { DEBUG(SSSDBG_OP_FAILURE, "talloc_zero_size failed.\n"); - talloc_free(username); return ENOMEM; } - memcpy(msg, username, user_len); + /* sysdb_username is a fully-qualified name which is used by pam_sss when + * prompting the user for the PIN and as login name if it wasn't set by + * the PAM caller but has to be determined based on the inserted + * Smartcard. If this type of name is irritating at the PIN prompt or the + * re_expression config option was set in a way that user@domain cannot be + * handled anymore some more logic has to be added here. But for the time + * being I think using sysdb_username is fine. */ + memcpy(msg, sysdb_username, user_len); memcpy(msg + user_len, token_name, slot_len); - talloc_free(username); ret = pam_add_response(pd, SSS_PAM_CERT_INFO, msg_len, msg); talloc_free(msg); diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c index 02199e6..4b2dea4 100644 --- a/src/tests/cmocka/test_pam_srv.c +++ b/src/tests/cmocka/test_pam_srv.c @@ -664,11 +664,11 @@ static int test_pam_cert_check_gdm_smartcard(uint32_t status, uint8_t *body, assert_int_equal(val, SSS_PAM_CERT_INFO); SAFEALIGN_COPY_UINT32(&val, body + rp, &rp); - assert_int_equal(val, (sizeof("pamuser") + sizeof(TEST_TOKEN_NAME))); + assert_int_equal(val, (sizeof("pamuser@"TEST_DOM_NAME) + sizeof(TEST_TOKEN_NAME))); - assert_int_equal(*(body + rp + sizeof("pamuser") - 1), 0); - assert_string_equal(body + rp, "pamuser"); - rp += sizeof("pamuser"); + assert_int_equal(*(body + rp + sizeof("pamuser@"TEST_DOM_NAME) - 1), 0); + assert_string_equal(body + rp, "pamuser@"TEST_DOM_NAME); + rp += sizeof("pamuser@"TEST_DOM_NAME); assert_int_equal(*(body + rp + sizeof(TEST_TOKEN_NAME) - 1), 0); assert_string_equal(body + rp, TEST_TOKEN_NAME); @@ -703,11 +703,11 @@ static int test_pam_cert_check(uint32_t status, uint8_t *body, size_t blen) assert_int_equal(val, SSS_PAM_CERT_INFO); SAFEALIGN_COPY_UINT32(&val, body + rp, &rp); - assert_int_equal(val, (sizeof("pamuser") + sizeof(TEST_TOKEN_NAME))); + assert_int_equal(val, (sizeof("pamuser@"TEST_DOM_NAME) + sizeof(TEST_TOKEN_NAME))); - assert_int_equal(*(body + rp + sizeof("pamuser") - 1), 0); - assert_string_equal(body + rp, "pamuser"); - rp += sizeof("pamuser"); + assert_int_equal(*(body + rp + sizeof("pamuser@"TEST_DOM_NAME) - 1), 0); + assert_string_equal(body + rp, "pamuser@"TEST_DOM_NAME); + rp += sizeof("pamuser@"TEST_DOM_NAME); assert_int_equal(*(body + rp + sizeof(TEST_TOKEN_NAME) - 1), 0); assert_string_equal(body + rp, TEST_TOKEN_NAME);
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
