From 849d495ea948e75ecb4ea469c9f8db4a740a2377 Mon Sep 17 00:00:00 2001

From: Sumit Bose <sbose@redhat.com>

Date: Fri, 7 Feb 2020 20:32:45 +0100

Subject: [PATCH 27/27] ssh: add 'no_rules' and 'all_rules' to

 ssh_use_certificate_matching_rules

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

To make ssh_use_certificate_matching_rules option more flexible and

predictable the keywords 'all_rules' and 'no_rules' are added.

'no_rules' can be used to allow all certificates.

If rules names are given but no matching rules can be found this is

considered an error and no ssh keys will be derived from the

certificates.

Related to https://pagure.io/SSSD/sssd/issue/4121

Reviewed-by: Tomáš Halman <thalman@redhat.com>

---

 src/man/sssd.conf.5.xml         |  16 +++--

 src/responder/ssh/ssh_cmd.c     |  33 ++++++---

 src/responder/ssh/ssh_private.h |   1 +

 src/responder/ssh/ssh_reply.c   |   8 +++

 src/tests/cmocka/test_ssh_srv.c | 122 +++++++++++++++++++++++++++++++-

 5 files changed, 165 insertions(+), 15 deletions(-)

diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml

index ef07c43d3..f71fbf4aa 100644

--- a/src/man/sssd.conf.5.xml

+++ b/src/man/sssd.conf.5.xml

@@ -1760,12 +1760,20 @@ p11_uri = library-description=OpenSC%20smartcard%20framework;slot-id=2

                             will be ignored.

                         </para>

                         <para>

-                            If a non-existing rule name is given all rules will

-                            be ignored and all available certificates will be

-                            used to derive ssh keys.

+                            There are two special key words 'all_rules' and

+                            'no_rules' which will enable all or no rules,

+                            respectively. The latter means that no certificates

+                            will be filtered out and ssh keys will be generated

+                            from all valid certificates.

                         </para>

                         <para>

-                            Default: not set, all found rules are used

+                            A non-existing rule name is considered an error.

+                            If as a result no rule is selected all certificates

+                            will be ignored.

+                        </para>

+                        <para>

+                            Default: not set, equivalent to 'all_rules,

+                            all found rules are used

                         </para>

                     </listitem>

                 </varlistentry>

diff --git a/src/responder/ssh/ssh_cmd.c b/src/responder/ssh/ssh_cmd.c

index 09f9b73b6..d1e7c667b 100644

--- a/src/responder/ssh/ssh_cmd.c

+++ b/src/responder/ssh/ssh_cmd.c

@@ -157,10 +157,26 @@ static errno_t ssh_cmd_refresh_certmap_ctx(struct ssh_ctx *ssh_ctx,

     size_t c;

     int ret;

     bool rule_added;

+    bool all_rules = false;

+    bool no_rules = false;

+

+    ssh_ctx->cert_rules_error = false;

+

+    if (ssh_ctx->cert_rules == NULL || ssh_ctx->cert_rules[0] == NULL) {

+        all_rules = true;

+    } else if (ssh_ctx->cert_rules[0] != NULL

+                    && ssh_ctx->cert_rules[1] == NULL) {

+        if (strcmp(ssh_ctx->cert_rules[0], "all_rules") == 0) {

+            all_rules = true;

+        } else if (strcmp(ssh_ctx->cert_rules[0], "no_rules") == 0) {

+            no_rules = true;

+        }

+    }

     if (!ssh_ctx->use_cert_keys

             || ssh_ctx->certmap_last_read

-                    >= ssh_ctx->rctx->get_domains_last_call.tv_sec) {

+                    >= ssh_ctx->rctx->get_domains_last_call.tv_sec

+            || no_rules) {

         DEBUG(SSSDBG_TRACE_ALL, "No certmap update needed.\n");

         return EOK;

     }

@@ -180,9 +196,8 @@ static errno_t ssh_cmd_refresh_certmap_ctx(struct ssh_ctx *ssh_ctx,

         for (c = 0; certmap_list[c] != NULL; c++) {

-            if (ssh_ctx->cert_rules != NULL

-                        && !string_in_list(certmap_list[c]->name,

-                                           ssh_ctx->cert_rules, true)) {

+            if (!all_rules && !string_in_list(certmap_list[c]->name,

+                                              ssh_ctx->cert_rules, true)) {

                 DEBUG(SSSDBG_TRACE_ALL, "Skipping matching rule [%s], it is "

                       "not listed in the ssh_use_certificate_matching_rules "

                       "option.\n", certmap_list[c]->name);

@@ -212,11 +227,12 @@ static errno_t ssh_cmd_refresh_certmap_ctx(struct ssh_ctx *ssh_ctx,

     }

     if (!rule_added) {

-        DEBUG(SSSDBG_TRACE_ALL,

-              "No matching rule added, all certificates will be used.\n");

+        DEBUG(SSSDBG_CONF_SETTINGS,

+              "No matching rule added, please check "

+              "ssh_use_certificate_matching_rules option values for typos .\n");

-        sss_certmap_free_ctx(sss_certmap_ctx);

-        sss_certmap_ctx = NULL;

+        ret = EINVAL;

+        goto done;

     }

     ret = EOK;

@@ -228,6 +244,7 @@ done:

         ssh_ctx->certmap_last_read = ssh_ctx->rctx->get_domains_last_call.tv_sec;

     } else {

         sss_certmap_free_ctx(sss_certmap_ctx);

+        ssh_ctx->cert_rules_error = true;

     }

     return ret;

diff --git a/src/responder/ssh/ssh_private.h b/src/responder/ssh/ssh_private.h

index 76a1aead3..028ccd616 100644

--- a/src/responder/ssh/ssh_private.h

+++ b/src/responder/ssh/ssh_private.h

@@ -40,6 +40,7 @@ struct ssh_ctx {

     time_t certmap_last_read;

     struct sss_certmap_ctx *sss_certmap_ctx;

     char **cert_rules;

+    bool cert_rules_error;

 };

 struct sss_cmd_table *get_ssh_cmds(void);

diff --git a/src/responder/ssh/ssh_reply.c b/src/responder/ssh/ssh_reply.c

index 1200a3a36..97914266d 100644

--- a/src/responder/ssh/ssh_reply.c

+++ b/src/responder/ssh/ssh_reply.c

@@ -196,6 +196,14 @@ struct tevent_req *ssh_get_output_keys_send(TALLOC_CTX *mem_ctx,

         goto done;

     }

+    if (state->ssh_ctx->cert_rules_error) {

+        DEBUG(SSSDBG_CONF_SETTINGS,

+              "Skipping keys from certificates because there was an error "

+              "while processing matching rules.\n");

+        ret = EOK;

+        goto done;

+    }

+

     ret = confdb_get_string(cli_ctx->rctx->cdb, state,

                             CONFDB_MONITOR_CONF_ENTRY,

                             CONFDB_MONITOR_CERT_VERIFICATION, NULL,

diff --git a/src/tests/cmocka/test_ssh_srv.c b/src/tests/cmocka/test_ssh_srv.c

index 45915f681..fc43663a7 100644

--- a/src/tests/cmocka/test_ssh_srv.c

+++ b/src/tests/cmocka/test_ssh_srv.c

@@ -712,6 +712,120 @@ void test_ssh_user_pubkey_cert_with_rule(void **state)

     assert_int_equal(ret, EOK);

 }

+void test_ssh_user_pubkey_cert_with_all_rules(void **state)

+{

+    int ret;

+    struct sysdb_attrs *attrs;

+    /* Both rules are enabled, both certificates should be handled. */

+    const char *rule_list[] = { "all_rules", NULL };

+    struct certmap_info *certmap_list[] = { &rule_1, &rule_2, NULL};

+

+    attrs = sysdb_new_attrs(ssh_test_ctx);

+    assert_non_null(attrs);

+    ret = sysdb_attrs_add_string(attrs, SYSDB_SSH_PUBKEY, TEST_SSH_PUBKEY);

+    assert_int_equal(ret, EOK);

+    ret = sysdb_attrs_add_base64_blob(attrs, SYSDB_USER_CERT,

+                                      SSSD_TEST_CERT_0001);

+    assert_int_equal(ret, EOK);

+    ret = sysdb_attrs_add_base64_blob(attrs, SYSDB_USER_CERT,

+                                      SSSD_TEST_CERT_0002);

+    assert_int_equal(ret, EOK);

+

+    ret = sysdb_set_user_attr(ssh_test_ctx->tctx->dom,

+                              ssh_test_ctx->ssh_user_fqdn,

+                              attrs,

+                              LDB_FLAG_MOD_ADD);

+    talloc_free(attrs);

+    assert_int_equal(ret, EOK);

+

+    mock_input_user(ssh_test_ctx, ssh_test_ctx->ssh_user_fqdn);

+    will_return(__wrap_sss_packet_get_cmd, SSS_SSH_GET_USER_PUBKEYS);

+    will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);

+    will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);

+    will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);

+    will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);

+

+    /* Enable certificate support */

+    ssh_test_ctx->ssh_ctx->use_cert_keys = true;

+    ssh_test_ctx->ssh_ctx->rctx->domains->certmaps = certmap_list;

+    ssh_test_ctx->ssh_ctx->certmap_last_read = 0;

+    ssh_test_ctx->ssh_ctx->rctx->get_domains_last_call.tv_sec = 1;

+    ssh_test_ctx->ssh_ctx->cert_rules = discard_const(rule_list);

+#ifdef HAVE_NSS

+    ssh_test_ctx->ssh_ctx->ca_db = discard_const("sql:" ABS_BUILD_DIR

+                                                "/src/tests/test_CA/p11_nssdb");

+#else

+    ssh_test_ctx->ssh_ctx->ca_db = discard_const(ABS_BUILD_DIR

+                                                "/src/tests/test_CA/SSSD_test_CA.pem");

+#endif

+

+    set_cmd_cb(test_ssh_user_pubkey_cert_check);

+    ret = sss_cmd_execute(ssh_test_ctx->cctx, SSS_SSH_GET_USER_PUBKEYS,

+                          ssh_test_ctx->ssh_cmds);

+    assert_int_equal(ret, EOK);

+

+    /* Wait until the test finishes with EOK */

+    ret = test_ev_loop(ssh_test_ctx->tctx);

+    assert_int_equal(ret, EOK);

+}

+

+void test_ssh_user_pubkey_cert_with_no_rules(void **state)

+{

+    int ret;

+    struct sysdb_attrs *attrs;

+    /* No rules should be used, both certificates should be handled. */

+    const char *rule_list[] = { "no_rules", NULL };

+    struct certmap_info *certmap_list[] = { &rule_1, &rule_2, NULL};

+

+    attrs = sysdb_new_attrs(ssh_test_ctx);

+    assert_non_null(attrs);

+    ret = sysdb_attrs_add_string(attrs, SYSDB_SSH_PUBKEY, TEST_SSH_PUBKEY);

+    assert_int_equal(ret, EOK);

+    ret = sysdb_attrs_add_base64_blob(attrs, SYSDB_USER_CERT,

+                                      SSSD_TEST_CERT_0001);

+    assert_int_equal(ret, EOK);

+    ret = sysdb_attrs_add_base64_blob(attrs, SYSDB_USER_CERT,

+                                      SSSD_TEST_CERT_0002);

+    assert_int_equal(ret, EOK);

+

+    ret = sysdb_set_user_attr(ssh_test_ctx->tctx->dom,

+                              ssh_test_ctx->ssh_user_fqdn,

+                              attrs,

+                              LDB_FLAG_MOD_ADD);

+    talloc_free(attrs);

+    assert_int_equal(ret, EOK);

+

+    mock_input_user(ssh_test_ctx, ssh_test_ctx->ssh_user_fqdn);

+    will_return(__wrap_sss_packet_get_cmd, SSS_SSH_GET_USER_PUBKEYS);

+    will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);

+    will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);

+    will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);

+    will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);

+

+    /* Enable certificate support */

+    ssh_test_ctx->ssh_ctx->use_cert_keys = true;

+    ssh_test_ctx->ssh_ctx->rctx->domains->certmaps = certmap_list;

+    ssh_test_ctx->ssh_ctx->certmap_last_read = 0;

+    ssh_test_ctx->ssh_ctx->rctx->get_domains_last_call.tv_sec = 1;

+    ssh_test_ctx->ssh_ctx->cert_rules = discard_const(rule_list);

+#ifdef HAVE_NSS

+    ssh_test_ctx->ssh_ctx->ca_db = discard_const("sql:" ABS_BUILD_DIR

+                                                "/src/tests/test_CA/p11_nssdb");

+#else

+    ssh_test_ctx->ssh_ctx->ca_db = discard_const(ABS_BUILD_DIR

+                                                "/src/tests/test_CA/SSSD_test_CA.pem");

+#endif

+

+    set_cmd_cb(test_ssh_user_pubkey_cert_check);

+    ret = sss_cmd_execute(ssh_test_ctx->cctx, SSS_SSH_GET_USER_PUBKEYS,

+                          ssh_test_ctx->ssh_cmds);

+    assert_int_equal(ret, EOK);

+

+    /* Wait until the test finishes with EOK */

+    ret = test_ev_loop(ssh_test_ctx->tctx);

+    assert_int_equal(ret, EOK);

+}

+

 void test_ssh_user_pubkey_cert_with_unknow_rule_name(void **state)

 {

     int ret;

@@ -743,8 +857,6 @@ void test_ssh_user_pubkey_cert_with_unknow_rule_name(void **state)

     will_return(__wrap_sss_packet_get_cmd, SSS_SSH_GET_USER_PUBKEYS);

     will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);

     will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);

-    will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);

-    will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL);

     /* Enable certificate support */

     ssh_test_ctx->ssh_ctx->use_cert_keys = true;

@@ -760,7 +872,7 @@ void test_ssh_user_pubkey_cert_with_unknow_rule_name(void **state)

                                                 "/src/tests/test_CA/SSSD_test_CA.pem");

 #endif

-    set_cmd_cb(test_ssh_user_pubkey_cert_check);

+    set_cmd_cb(test_ssh_user_one_pubkey_check);

     ret = sss_cmd_execute(ssh_test_ctx->cctx, SSS_SSH_GET_USER_PUBKEYS,

                           ssh_test_ctx->ssh_cmds);

     assert_int_equal(ret, EOK);

@@ -852,6 +964,10 @@ int main(int argc, const char *argv[])

                                         ssh_test_setup, ssh_test_teardown),

         cmocka_unit_test_setup_teardown(test_ssh_user_pubkey_cert_with_rule,

                                         ssh_test_setup, ssh_test_teardown),

+        cmocka_unit_test_setup_teardown(test_ssh_user_pubkey_cert_with_all_rules,

+                                        ssh_test_setup, ssh_test_teardown),

+        cmocka_unit_test_setup_teardown(test_ssh_user_pubkey_cert_with_no_rules,

+                                        ssh_test_setup, ssh_test_teardown),

         cmocka_unit_test_setup_teardown(test_ssh_user_pubkey_cert_with_unknow_rule_name,

                                         ssh_test_setup, ssh_test_teardown),

         cmocka_unit_test_setup_teardown(test_ssh_user_pubkey_cert_with_rule_1,

-- 

2.20.1