From 6f7f15691b071cefd4e04a9fee44af580b6c502b Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose@redhat.com>
Date: Mon, 9 Mar 2020 13:39:47 +0100
Subject: [PATCH] ssh: fix matching rules default
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Before the ssh_use_certificate_matching_rules option was added the ssh
responder returned ssh keys derived from all valid certificates. Since
the default of the ssh_use_certificate_matching_rules option is
'all_rules' in a case where no matching rules are defined all
certificated will be filtered out and no ssh keys are returned.
The intention of the default was to allow the same same certificates
which are allowed in the PAM responder for authentication. The missing
default matching rule which is currently use by the PAM responder if no
other rules are available is added by this patch.
There might still be a small regression in case certificates without the
extended key usage (EKU) clientAuth were used for ssh. In this case
'ssh_use_certificate_matching_rules = no_rules' or a suitable matching
rule must be added to the configuration.
Related to https://pagure.io/SSSD/sssd/issue/4121
Reviewed-by: Tomáš Halman <thalman@redhat.com>
---
src/man/sssd.conf.5.xml | 9 ++++-
src/responder/pam/pam_helpers.h | 2 ++
src/responder/pam/pamsrv_p11.c | 3 +-
src/responder/ssh/ssh_cmd.c | 30 +++++++++++++----
src/tests/cmocka/test_ssh_srv.c | 58 +++++++++++++++++++++++++++++++++
5 files changed, 93 insertions(+), 9 deletions(-)
diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index 58383579c..a2567f5ac 100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -1766,6 +1766,13 @@ p11_uri = library-description=OpenSC%20smartcard%20framework;slot-id=2
will be filtered out and ssh keys will be generated
from all valid certificates.
</para>
+ <para>
+ If no rules are configured using 'all_rules' will
+ enable a default rule which enables all
+ certificates suitable for client authentication.
+ This is the same behavior as for the PAM responder
+ if certificate authentication is enabled.
+ </para>
<para>
A non-existing rule name is considered an error.
If as a result no rule is selected all certificates
@@ -1773,7 +1780,7 @@ p11_uri = library-description=OpenSC%20smartcard%20framework;slot-id=2
</para>
<para>
Default: not set, equivalent to 'all_rules,
- all found rules are used
+ all found rules or the default rule are used
</para>
</listitem>
</varlistentry>
diff --git a/src/responder/pam/pam_helpers.h b/src/responder/pam/pam_helpers.h
index 614389706..23fd308bb 100644
--- a/src/responder/pam/pam_helpers.h
+++ b/src/responder/pam/pam_helpers.h
@@ -25,6 +25,8 @@
#include "util/util.h"
+#define CERT_AUTH_DEFAULT_MATCHING_RULE "KRB5:<EKU>clientAuth"
+
errno_t pam_initgr_cache_set(struct tevent_context *ev,
hash_table_t *id_table,
char *name,
diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c
index 0dc53a826..8e276b200 100644
--- a/src/responder/pam/pamsrv_p11.c
+++ b/src/responder/pam/pamsrv_p11.c
@@ -26,13 +26,12 @@
#include "util/child_common.h"
#include "util/strtonum.h"
#include "responder/pam/pamsrv.h"
+#include "responder/pam/pam_helpers.h"
#include "lib/certmap/sss_certmap.h"
#include "util/crypto/sss_crypto.h"
#include "db/sysdb.h"
-#define CERT_AUTH_DEFAULT_MATCHING_RULE "KRB5:<EKU>clientAuth"
-
struct cert_auth_info {
char *cert;
char *token_name;
diff --git a/src/responder/ssh/ssh_cmd.c b/src/responder/ssh/ssh_cmd.c
index e42e29bfd..a593c904f 100644
--- a/src/responder/ssh/ssh_cmd.c
+++ b/src/responder/ssh/ssh_cmd.c
@@ -29,6 +29,7 @@
#include "responder/common/responder.h"
#include "responder/common/cache_req/cache_req.h"
#include "responder/ssh/ssh_private.h"
+#include "responder/pam/pam_helpers.h"
#include "lib/certmap/sss_certmap.h"
struct ssh_cmd_ctx {
@@ -159,6 +160,7 @@ static errno_t ssh_cmd_refresh_certmap_ctx(struct ssh_ctx *ssh_ctx,
bool rule_added;
bool all_rules = false;
bool no_rules = false;
+ bool rules_present = false;
ssh_ctx->cert_rules_error = false;
@@ -195,6 +197,7 @@ static errno_t ssh_cmd_refresh_certmap_ctx(struct ssh_ctx *ssh_ctx,
}
for (c = 0; certmap_list[c] != NULL; c++) {
+ rules_present = true;
if (!all_rules && !string_in_list(certmap_list[c]->name,
ssh_ctx->cert_rules, true)) {
@@ -227,12 +230,27 @@ static errno_t ssh_cmd_refresh_certmap_ctx(struct ssh_ctx *ssh_ctx,
}
if (!rule_added) {
- DEBUG(SSSDBG_CONF_SETTINGS,
- "No matching rule added, please check "
- "ssh_use_certificate_matching_rules option values for typos .\n");
-
- ret = EINVAL;
- goto done;
+ if (!rules_present) {
+ DEBUG(SSSDBG_TRACE_FUNC,
+ "No rules available, trying to add default matching rule.\n");
+ ret = sss_certmap_add_rule(sss_certmap_ctx, SSS_CERTMAP_MIN_PRIO,
+ CERT_AUTH_DEFAULT_MATCHING_RULE,
+ NULL, NULL);
+ if (ret != 0) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Failed to add default matching rule [%d][%s].\n",
+ ret, sss_strerror(ret));
+ goto done;
+ }
+ } else {
+ DEBUG(SSSDBG_CONF_SETTINGS,
+ "No matching rule added, please check "
+ "ssh_use_certificate_matching_rules option values for "
+ "typos.\n");
+
+ ret = EINVAL;
+ goto done;
+ }
}
ret = EOK;
diff --git a/src/tests/cmocka/test_ssh_srv.c b/src/tests/cmocka/test_ssh_srv.c
index fc43663a7..a48013416 100644
--- a/src/tests/cmocka/test_ssh_srv.c
+++ b/src/tests/cmocka/test_ssh_srv.c
@@ -769,6 +769,62 @@ void test_ssh_user_pubkey_cert_with_all_rules(void **state)
assert_int_equal(ret, EOK);
}
+void test_ssh_user_pubkey_cert_with_all_rules_but_no_rules_present(void **state)
+{
+ int ret;
+ struct sysdb_attrs *attrs;
+ /* Both rules are enabled, both certificates should be handled. */
+ const char *rule_list[] = { "all_rules", 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 = NULL;
+ 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;
@@ -966,6 +1022,8 @@ int main(int argc, const char *argv[])
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_all_rules_but_no_rules_present,
+ 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,
--
2.21.1