Hi,
this is a set of patches implementing SSH support in SSSD.
To test it, install a SSH-patched (patches are on freeipa-devel) IPA server, create a test user with SSH public keys ("ipa user-add someuser --sshpubkey=<blob>"), and then use the sss_ssh_authorizedkeys command to get the user's SSH public keys ("sss_ssh_authorizedkeys someuser" should output user's public keys in OpenSSH's authorized_keys format).
This is not a complete set of patches for SSH support, some work is still missing and will be posted soon (SSH host keys support in IPA provider, known_hosts client and documentation).
[PATCH 1/9] UTIL: Provide base64 encoding and decoding functions
[PATCH 2/9] BUILD: Introduce a --with-ssh config option
[PATCH 3/9] LDAP: Add support for SSH user public keys
[PATCH 4/9] DP: Add auxiliary info handler This is used to handle non-NSS accounts, such as hosts.
[PATCH 5/9] IPA: Add auxiliary info handler
[PATCH 6/9] DP: Add support for hosts in sss_dp_get_account Host requests are directed to the auxiliary info handler.
[PATCH 7/9] SSH: Responder
[PATCH 8/9] SSH: Common client code
[PATCH 9/9] SSH: AuthorizedKeysCommand client
Honza
On Fri, Feb 03, 2012 at 11:29:52PM +0100, Jan Cholasta wrote:
Hi,
this is a set of patches implementing SSH support in SSSD.
To test it, install a SSH-patched (patches are on freeipa-devel) IPA server, create a test user with SSH public keys ("ipa user-add someuser --sshpubkey=<blob>"), and then use the sss_ssh_authorizedkeys command to get the user's SSH public keys ("sss_ssh_authorizedkeys someuser" should output user's public keys in OpenSSH's authorized_keys format).
Is there a way to test these using an actual SSH client yet?
This is not a complete set of patches for SSH support, some work is still missing and will be posted soon (SSH host keys support in IPA provider, known_hosts client and documentation).
Also please remember to add the new options to the ConfigAPI.
[PATCH 1/9] UTIL: Provide base64 encoding and decoding functions
Can you add a DEBUG() message saying that sss_base64_{encode,decode} are not implemented with libcrypto back end?
I think the NSS back end sss_base64_{encode,decode} should call nspr_nss_init() otherwise they are just relying on the NSS crypto library being initialized.
[PATCH 2/9] BUILD: Introduce a --with-ssh config option
ACK to the code, but this patch will likely need a rebase on top of the similar sudo/autofs patches.
[PATCH 3/9] LDAP: Add support for SSH user public keys
This patch should be rebased on top of Stephen's cache timeout patch that is already acked on the list.
I don'really like the "continue" special case in sdap_save_user(). Why not simply move SDAP_AT_USER_SSH_PUBLIC_KEY before SDAP_FIRST_EXTRA_USER_AT?
[PATCH 4/9] DP: Add auxiliary info handler This is used to handle non-NSS accounts, such as hosts.
[PATCH 5/9] IPA: Add auxiliary info handler
I'm not a big fan of the auxilary handler, to be honest. While it saves a lot of code duplication, how do you define what falls into the auxiliary category and what needs its own provider type?
The rest is TBR, I'll continue later.
[PATCH 6/9] DP: Add support for hosts in sss_dp_get_account Host requests are directed to the auxiliary info handler.
[PATCH 7/9] SSH: Responder
[PATCH 8/9] SSH: Common client code
[PATCH 9/9] SSH: AuthorizedKeysCommand client
Honza
-- Jan Cholasta
From fea5e73be4ee89fbb403d11bd8b6c1652faed63d Mon Sep 17 00:00:00 2001 From: Jan Cholasta jcholast@redhat.com Date: Fri, 3 Feb 2012 22:10:17 +0100 Subject: [PATCH 1/9] UTIL: Provide base64 encoding and decoding functions
Makefile.am | 6 ++- src/util/crypto/libcrypto/crypto_base64.c | 33 +++++++++++++ src/util/crypto/nss/nss_base64.c | 73 +++++++++++++++++++++++++++++ src/util/crypto/nss/nss_obfuscate.c | 39 +-------------- src/util/crypto/sss_crypto.h | 8 +++ 5 files changed, 121 insertions(+), 38 deletions(-) create mode 100644 src/util/crypto/libcrypto/crypto_base64.c create mode 100644 src/util/crypto/nss/nss_base64.c
diff --git a/Makefile.am b/Makefile.am index 7a1b911..8b6a65d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -136,13 +136,15 @@ noinst_LTLIBRARIES = \ libsss_crypt.la
if HAVE_NSS
- SSS_CRYPT_SOURCES = src/util/crypto/nss/nss_sha512crypt.c \
- SSS_CRYPT_SOURCES = src/util/crypto/nss/nss_base64.c \
src/util/crypto/nss/nss_sha512crypt.c \ src/util/crypto/nss/nss_obfuscate.c \ src/util/crypto/nss/nss_util.celse
- SSS_CRYPT_SOURCES = src/util/crypto/libcrypto/crypto_sha512crypt.c \
- SSS_CRYPT_SOURCES = src/util/crypto/libcrypto/crypto_base64.c \
src/util/crypto/libcrypto/crypto_sha512crypt.c \ src/util/crypto/libcrypto/crypto_obfuscate.cdiff --git a/src/util/crypto/libcrypto/crypto_base64.c b/src/util/crypto/libcrypto/crypto_base64.c new file mode 100644 index 0000000..01a54fa --- /dev/null +++ b/src/util/crypto/libcrypto/crypto_base64.c @@ -0,0 +1,33 @@ +/*
- Authors:
Jan Cholasta <jcholast@redhat.com>- Copyright (C) 2012 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/.
+*/
+char *sss_base64_encode(TALLOC_CTX *mem_ctx,
const unsigned char *in,size_t insize)+{
- return NULL;
+}
+unsigned char *sss_base64_decode(TALLOC_CTX *mem_ctx,
const char *in,size_t *outsize)+{
- return NULL;
+} diff --git a/src/util/crypto/nss/nss_base64.c b/src/util/crypto/nss/nss_base64.c new file mode 100644 index 0000000..0232510 --- /dev/null +++ b/src/util/crypto/nss/nss_base64.c @@ -0,0 +1,73 @@ +/*
- Authors:
Jakub Hrozek <jhrozek@redhat.com>- Copyright (C) 2012 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/.
+*/
+#include <base64.h> +#include <talloc.h>
+/* NSS wraps b64 encoded buffers with CRLF automatically after 64 chars. This
- function strips the CRLF double-chars. The buffer can be decoded with plain
- NSS calls */
+char *sss_base64_encode(TALLOC_CTX *mem_ctx,
const unsigned char *inbuf,size_t inbufsize)+{
- char *b64encoded = NULL;
- int i, j, b64size;
- char *outbuf;
- b64encoded = BTOA_DataToAscii(inbuf, inbufsize);
- if (!b64encoded) return NULL;
- b64size = strlen(b64encoded) + 1;
- outbuf = talloc_array(mem_ctx, char, b64size);
- if (outbuf == NULL) {
PORT_Free(b64encoded);return NULL;- }
- for (i=0, j=0; i < b64size; i++) {
if (b64encoded[i] == '\n' || b64encoded[i] == '\r') {continue;}outbuf[j++] = b64encoded[i]; /* will also copy the trailing \0 char */- }
- PORT_Free(b64encoded);
- return outbuf;
+}
+unsigned char *sss_base64_decode(TALLOC_CTX *mem_ctx,
const char *inbuf,size_t *outbufsize)+{
- unsigned char *b64decoded = NULL;
- unsigned int size;
- unsigned char *outbuf;
- b64decoded = ATOB_AsciiToData(inbuf, &size);
- if (!b64decoded) return NULL;
- outbuf = talloc_memdup(mem_ctx, b64decoded, size);
- PORT_Free(b64decoded);
- if (!outbuf) return NULL;
- *outbufsize = size;
- return outbuf;
+} diff --git a/src/util/crypto/nss/nss_obfuscate.c b/src/util/crypto/nss/nss_obfuscate.c index db10be6..c0882d4 100644 --- a/src/util/crypto/nss/nss_obfuscate.c +++ b/src/util/crypto/nss/nss_obfuscate.c @@ -224,38 +224,6 @@ done: return ret; }
-/* NSS wraps b64 encoded buffers with CRLF automatically after 64 chars. This
- function strips the CRLF double-chars. The buffer can be decoded with plain
- NSS calls */
-static char *b64_encode(TALLOC_CTX *mem_ctx,
unsigned char *inbuf,size_t inbufsize)-{
- char *b64encoded = NULL;
- int i, j, b64size;
- char *outbuf;
- b64encoded = BTOA_DataToAscii(inbuf, inbufsize);
- if (!b64encoded) return NULL;
- b64size = strlen(b64encoded) + 1;
- outbuf = talloc_array(mem_ctx, char, b64size);
- if (outbuf == NULL) {
PORT_Free(b64encoded);return NULL;- }
- for (i=0, j=0; i < b64size; i++) {
if (b64encoded[i] == '\n' || b64encoded[i] == '\r') {continue;}outbuf[j++] = b64encoded[i]; /* will also copy the trailing \0 char */- }
- PORT_Free(b64encoded);
- return outbuf;
-}
int sss_password_encrypt(TALLOC_CTX *mem_ctx, const char *password, int plen, enum obfmethod meth, char **obfpwd) { @@ -383,7 +351,7 @@ int sss_password_encrypt(TALLOC_CTX *mem_ctx, const char *password, int plen, OBF_BUFFER_SENTINEL_SIZE, &p);
/* Base64 encode the resulting buffer */
- *obfpwd = b64_encode(mem_ctx, obfbuf, obufsize);
- *obfpwd = sss_base64_encode(mem_ctx, obfbuf, obufsize); if (*obfpwd == NULL) { ret = ENOMEM; goto done;
@@ -408,7 +376,7 @@ int sss_password_decrypt(TALLOC_CTX *mem_ctx, char *b64encoded, int plainlen; unsigned int digestlen; unsigned char *obfbuf = NULL;
- unsigned int obflen;
size_t obflen; char *pwdbuf;
/* for unmarshaling data */
@@ -433,7 +401,7 @@ int sss_password_decrypt(TALLOC_CTX *mem_ctx, char *b64encoded, }
/* Base64 decode the incoming buffer */
- obfbuf = ATOB_AsciiToData(b64encoded, &obflen);
- obfbuf = sss_base64_decode(tmp_ctx, b64encoded, &obflen); if (!obfbuf) { ret = ENOMEM; goto done;
@@ -531,7 +499,6 @@ int sss_password_decrypt(TALLOC_CTX *mem_ctx, char *b64encoded, *password = talloc_move(mem_ctx, &pwdbuf); ret = EOK; done:
- PORT_Free(obfbuf); talloc_free(tmp_ctx); nspr_nss_cleanup(); return ret;
diff --git a/src/util/crypto/sss_crypto.h b/src/util/crypto/sss_crypto.h index 66394ae..7b3e5e0 100644 --- a/src/util/crypto/sss_crypto.h +++ b/src/util/crypto/sss_crypto.h @@ -11,6 +11,14 @@ enum obfmethod {
int test2(void);
+char *sss_base64_encode(TALLOC_CTX *mem_ctx,
const unsigned char *in,size_t insize);+unsigned char *sss_base64_decode(TALLOC_CTX *mem_ctx,
const char *in,size_t *outsize);int sss_password_encrypt(TALLOC_CTX *mem_ctx, const char *password, int plen, enum obfmethod meth, char **obfpwd);
-- 1.7.6.5
From eee1577fde2a617f50a4104c9c79048dbda48468 Mon Sep 17 00:00:00 2001 From: Jan Cholasta jcholast@redhat.com Date: Fri, 3 Feb 2012 22:18:42 +0100 Subject: [PATCH 2/9] BUILD: Introduce a --with-ssh config option
configure.ac | 1 + src/conf_macros.m4 | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/configure.ac b/configure.ac index baea45c..3f4d794 100644 --- a/configure.ac +++ b/configure.ac @@ -94,6 +94,7 @@ WITH_SEMANAGE WITH_LIBNL WITH_NOLOGIN_SHELL WITH_APP_LIBS +WITH_SSH
m4_include([src/external/pkg.m4]) m4_include([src/external/libpopt.m4]) diff --git a/src/conf_macros.m4 b/src/conf_macros.m4 index 537b985..a46c63b 100644 --- a/src/conf_macros.m4 +++ b/src/conf_macros.m4 @@ -401,3 +401,26 @@ AC_DEFUN([WITH_APP_LIBS], AC_SUBST(appmodpath) AC_DEFINE_UNQUOTED(APP_MODULES_PATH, "$config_appmodpath", [Path to the 3rd party modules]) ])
+AC_DEFUN([WITH_SSH],
- [ AC_ARG_WITH([ssh],
[AC_HELP_STRING([--with-ssh],[Whether to build with SSH support [no]])],[with_ssh=$withval],[])- dnl Remove when SSH goes out of experimental
- if test x"$enable_all_experimental_features" = xyes; then
if test x"$with_ssh" != xno; thenwith_ssh=yesfi- fi
- if test x"$with_ssh" = xyes; then
AC_DEFINE(BUILD_SSH, 1, [whether to build with SSH support])- fi
- AM_CONDITIONAL([BUILD_SSH], [test x"$with_ssh" = xyes])
- ])
-- 1.7.6.5
From 86a9c9b155b45d2c3d09356b923cf30ca3e00b2d Mon Sep 17 00:00:00 2001 From: Jan Cholasta jcholast@redhat.com Date: Fri, 3 Feb 2012 22:29:47 +0100 Subject: [PATCH 3/9] LDAP: Add support for SSH user public keys
src/db/sysdb.h | 2 ++ src/providers/ipa/ipa_common.c | 3 ++- src/providers/ldap/ldap_common.c | 6 ++++-- src/providers/ldap/sdap.h | 1 + src/providers/ldap/sdap_async_users.c | 28 ++++++++++++++++++++++++++++ 5 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 31b988e..fd6d534 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -109,6 +109,8 @@ #define SYSDB_USN "entryUSN" #define SYSDB_HIGH_USN "highestUSN"
+#define SYSDB_SSH_PUBKEY "sshPublicKey"
#define SYSDB_NEXTID_FILTER "("SYSDB_NEXTID"=*)"
#define SYSDB_UC "objectclass="SYSDB_USER_CLASS diff --git a/src/providers/ipa/ipa_common.c b/src/providers/ipa/ipa_common.c index ba22830..790adb7 100644 --- a/src/providers/ipa/ipa_common.c +++ b/src/providers/ipa/ipa_common.c @@ -149,7 +149,8 @@ struct sdap_attr_map ipa_user_map[] = { { "ldap_user_authorized_host", "host", SYSDB_AUTHORIZED_HOST, NULL }, { "ldap_user_nds_login_disabled", "loginDisabled", SYSDB_NDS_LOGIN_DISABLED, NULL }, { "ldap_user_nds_login_expiration_time", "loginExpirationTime", SYSDB_NDS_LOGIN_EXPIRATION_TIME, NULL },
- { "ldap_user_nds_login_allowed_time_map", "loginAllowedTimeMap", SYSDB_NDS_LOGIN_ALLOWED_TIME_MAP, NULL }
- { "ldap_user_nds_login_allowed_time_map", "loginAllowedTimeMap", SYSDB_NDS_LOGIN_ALLOWED_TIME_MAP, NULL },
- { "ldap_user_ssh_public_key", "ipaSshPubKey", SYSDB_SSH_PUBKEY, NULL }
};
struct sdap_attr_map ipa_group_map[] = { diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c index 786e06b..061528d 100644 --- a/src/providers/ldap/ldap_common.c +++ b/src/providers/ldap/ldap_common.c @@ -150,7 +150,8 @@ struct sdap_attr_map rfc2307_user_map[] = { { "ldap_user_authorized_host", "host", SYSDB_AUTHORIZED_HOST, NULL }, { "ldap_user_nds_login_disabled", "loginDisabled", SYSDB_NDS_LOGIN_DISABLED, NULL }, { "ldap_user_nds_login_expiration_time", "loginExpirationTime", SYSDB_NDS_LOGIN_EXPIRATION_TIME, NULL },
- { "ldap_user_nds_login_allowed_time_map", "loginAllowedTimeMap", SYSDB_NDS_LOGIN_ALLOWED_TIME_MAP, NULL }
- { "ldap_user_nds_login_allowed_time_map", "loginAllowedTimeMap", SYSDB_NDS_LOGIN_ALLOWED_TIME_MAP, NULL },
- { "ldap_user_ssh_public_key", NULL, SYSDB_SSH_PUBKEY, NULL }
};
struct sdap_attr_map rfc2307_group_map[] = { @@ -197,7 +198,8 @@ struct sdap_attr_map rfc2307bis_user_map[] = { { "ldap_user_authorized_host", "host", SYSDB_AUTHORIZED_HOST, NULL }, { "ldap_user_nds_login_disabled", "loginDisabled", SYSDB_NDS_LOGIN_DISABLED, NULL }, { "ldap_user_nds_login_expiration_time", "loginExpirationTime", SYSDB_NDS_LOGIN_EXPIRATION_TIME, NULL },
- { "ldap_user_nds_login_allowed_time_map", "loginAllowedTimeMap", SYSDB_NDS_LOGIN_ALLOWED_TIME_MAP, NULL }
- { "ldap_user_nds_login_allowed_time_map", "loginAllowedTimeMap", SYSDB_NDS_LOGIN_ALLOWED_TIME_MAP, NULL },
- { "ldap_user_ssh_public_key", NULL, SYSDB_SSH_PUBKEY, NULL }
};
struct sdap_attr_map rfc2307bis_group_map[] = { diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h index 7bf1805..8a827ff 100644 --- a/src/providers/ldap/sdap.h +++ b/src/providers/ldap/sdap.h @@ -255,6 +255,7 @@ enum sdap_user_attrs { SDAP_AT_NDS_LOGIN_DISABLED, SDAP_AT_NDS_LOGIN_EXPIRATION_TIME, SDAP_AT_NDS_LOGIN_ALLOWED_TIME_MAP,
SDAP_AT_USER_SSH_PUBLIC_KEY,
SDAP_OPTS_USER /* attrs counter */
}; diff --git a/src/providers/ldap/sdap_async_users.c b/src/providers/ldap/sdap_async_users.c index ac856a6..c4fa968 100644 --- a/src/providers/ldap/sdap_async_users.c +++ b/src/providers/ldap/sdap_async_users.c @@ -22,6 +22,7 @@ */
#include "util/util.h" +#include "util/crypto/sss_crypto.h" #include "db/sysdb.h" #include "providers/ldap/sdap_async_private.h" #include "providers/ldap/ldap_common.h" @@ -55,6 +56,7 @@ int sdap_save_user(TALLOC_CTX *memctx, char *usn_value = NULL; char **missing = NULL; TALLOC_CTX *tmpctx = NULL;
char *sshpubkey;
DEBUG(9, ("Save user\n"));
@@ -228,6 +230,8 @@ int sdap_save_user(TALLOC_CTX *memctx, }
for (i = SDAP_FIRST_EXTRA_USER_AT; i < SDAP_OPTS_USER; i++) {
if (i == SDAP_AT_USER_SSH_PUBLIC_KEY) continue;ret = sdap_attrs_add_list(attrs, opts->user_map[i].sys_name, NULL, name, user_attrs); if (ret) {@@ -235,6 +239,30 @@ int sdap_save_user(TALLOC_CTX *memctx, } }
/* base64 encode sshPublicKey and add it to user_attrs */
ret = sysdb_attrs_get_el(attrs,
opts->user_map[SDAP_AT_USER_SSH_PUBLIC_KEY].sys_name, &el);if (ret) {
goto fail;}
for (i = 0; i < el->num_values; i++) {
sshpubkey = sss_base64_encode(user_attrs,el->values[i].data,el->values[i].length);if (!sshpubkey) {ret = ENOMEM;goto fail;}ret = sysdb_attrs_add_string(user_attrs,opts->user_map[SDAP_AT_USER_SSH_PUBLIC_KEY].sys_name,sshpubkey);if (ret) {goto fail;}}
cache_timeout = dp_opt_get_int(opts->basic, SDAP_ENTRY_CACHE_TIMEOUT);
if (is_initgr) {
-- 1.7.6.5
From 8efdb2d33b2581a2a53979ad7d3aa0e25be23d37 Mon Sep 17 00:00:00 2001 From: Jan Cholasta jcholast@redhat.com Date: Fri, 3 Feb 2012 22:35:14 +0100 Subject: [PATCH 4/9] DP: Add auxiliary info handler
This is used to handle non-NSS accounts, such as hosts.
src/confdb/confdb.h | 1 + src/providers/data_provider.h | 2 ++ src/providers/data_provider_be.c | 35 +++++++++++++++++++++++++++++++++-- src/providers/dp_backend.h | 1 + 4 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index 7cfc73d..56b06b9 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -101,6 +101,7 @@ #define CONFDB_DOMAIN_PATH_TMPL "config/domain/%s" #define CONFDB_DOMAIN_BASEDN "cn=domain,cn=config" #define CONFDB_DOMAIN_ID_PROVIDER "id_provider" +#define CONFDB_DOMAIN_AUXID_PROVIDER "auxid_provider" #define CONFDB_DOMAIN_AUTH_PROVIDER "auth_provider" #define CONFDB_DOMAIN_ACCESS_PROVIDER "access_provider" #define CONFDB_DOMAIN_CHPASS_PROVIDER "chpass_provider" diff --git a/src/providers/data_provider.h b/src/providers/data_provider.h index 627085b..e8395dc 100644 --- a/src/providers/data_provider.h +++ b/src/providers/data_provider.h @@ -50,6 +50,7 @@
#define DP_METHOD_REGISTER "RegisterService" #define DP_METHOD_GETACCTINFO "getAccountInfo" +#define DP_METHOD_GETAUXINFO "getAuxiliaryInfo" #define DP_METHOD_SUDOHANDLER "sudoHandler" /**
- @defgroup pamHandler PAM DBUS request
@@ -140,6 +141,7 @@ #define BE_REQ_SUDO_ALL 0x0006 #define BE_REQ_SUDO_DEFAULTS 0x0007 #define BE_REQ_SUDO_USER 0x0008 +#define BE_REQ_HOST 0x0009 #define BE_REQ_FAST 0x1000
/* AUTH related common data and functions */ diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c index 9965e52..35d705a 100644 --- a/src/providers/data_provider_be.c +++ b/src/providers/data_provider_be.c @@ -80,6 +80,7 @@ struct sbus_interface monitor_be_interface = {
static int client_registration(DBusMessage *message, struct sbus_connection *conn); static int be_get_account_info(DBusMessage *message, struct sbus_connection *conn); +static int be_get_auxiliary_info(DBusMessage *message, struct sbus_connection *conn); static int be_pam_handler(DBusMessage *message, struct sbus_connection *conn); static int be_sudo_handler(DBusMessage *message, struct sbus_connection *conn);
@@ -88,6 +89,7 @@ struct sbus_method be_methods[] = { { DP_METHOD_GETACCTINFO, be_get_account_info }, { DP_METHOD_PAMHANDLER, be_pam_handler }, { DP_METHOD_SUDOHANDLER, be_sudo_handler },
- { DP_METHOD_GETAUXINFO, be_get_auxiliary_info }, { NULL, NULL }
};
@@ -106,6 +108,7 @@ static struct bet_data bet_data[] = { {BET_ACCESS, CONFDB_DOMAIN_ACCESS_PROVIDER, "sssm_%s_access_init"}, {BET_CHPASS, CONFDB_DOMAIN_CHPASS_PROVIDER, "sssm_%s_chpass_init"}, {BET_SUDO, CONFDB_DOMAIN_SUDO_PROVIDER, "sssm_%s_sudo_init"},
- {BET_AUXID, CONFDB_DOMAIN_AUXID_PROVIDER, "sssm_%s_auxid_init"}, {BET_MAX, NULL, NULL}
};
@@ -295,7 +298,9 @@ split_service_name_filter(TALLOC_CTX *mem_ctx, return EOK; }
-static int be_get_account_info(DBusMessage *message, struct sbus_connection *conn) +static int be_get_account_info_generic(DBusMessage *message,
struct sbus_connection *conn,enum bet_type bet_type){ struct be_acct_req *req; struct be_req *be_req; @@ -440,7 +445,7 @@ static int be_get_account_info(DBusMessage *message, struct sbus_connection *con /* process request */
ret = be_file_request(becli->bectx,
becli->bectx->bet_info[BET_ID].bet_ops->handler,
becli->bectx->bet_info[bet_type].bet_ops->handler, be_req);@@ -475,6 +480,17 @@ done: return EOK; }
+static int be_get_account_info(DBusMessage *message, struct sbus_connection *conn) +{
- return be_get_account_info_generic(message, conn, BET_ID);
+}
+static int be_get_auxiliary_info(DBusMessage *message,
struct sbus_connection *conn)+{
- return be_get_account_info_generic(message, conn, BET_AUXID);
+}
static void be_pam_handler_callback(struct be_req *req, int dp_err_type, int errnum, @@ -1419,6 +1435,21 @@ int be_process_init(TALLOC_CTX *mem_ctx, "from provider [%s].\n", ctx->bet_info[BET_SUDO].mod_name)); }
- ret = load_backend_module(ctx, BET_AUXID,
&ctx->bet_info[BET_AUXID],ctx->bet_info[BET_ID].mod_name);- if (ret != EOK) {
if (ret != ENOENT) {DEBUG(0, ("fatal error initializing data providers\n"));return ret;}DEBUG(1, ("No auxiliary ID module provided for [%s] !!\n",be_domain));- } else {
DEBUG(9, ("AUXID backend target successfully loaded ""from provider [%s].\n", ctx->bet_info[BET_AUXID].mod_name));- }
- /* Handle SIGUSR1 to force offline behavior */ BlockSignals(false, SIGUSR1); tes = tevent_add_signal(ctx->ev, ctx, SIGUSR1, 0,
diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h index 0964110..1a36778 100644 --- a/src/providers/dp_backend.h +++ b/src/providers/dp_backend.h @@ -50,6 +50,7 @@ enum bet_type { BET_ACCESS, BET_CHPASS, BET_SUDO,
- BET_AUXID, BET_MAX
};
-- 1.7.6.5
From f80acc202e4e338c26a04ba48acb669a46e94fc7 Mon Sep 17 00:00:00 2001 From: Jan Cholasta jcholast@redhat.com Date: Fri, 3 Feb 2012 22:41:30 +0100 Subject: [PATCH 5/9] IPA: Add auxiliary info handler
Makefile.am | 4 +++ src/providers/ipa/ipa_auxid.c | 59 ++++++++++++++++++++++++++++++++++++++++ src/providers/ipa/ipa_auxid.h | 33 ++++++++++++++++++++++ src/providers/ipa/ipa_common.c | 1 + src/providers/ipa/ipa_common.h | 1 + src/providers/ipa/ipa_init.c | 45 ++++++++++++++++++++++++++++++ 6 files changed, 143 insertions(+), 0 deletions(-) create mode 100644 src/providers/ipa/ipa_auxid.c create mode 100644 src/providers/ipa/ipa_auxid.h
diff --git a/Makefile.am b/Makefile.am index 8b6a65d..f2569df 100644 --- a/Makefile.am +++ b/Makefile.am @@ -355,6 +355,7 @@ dist_noinst_HEADERS = \ src/providers/ipa/ipa_auth.h \ src/providers/ipa/ipa_dyndns.h \ src/providers/ipa/ipa_id.h \
- src/providers/ipa/ipa_auxid.h \ src/providers/proxy/proxy.h \ src/tools/tools_util.h \ src/tools/sss_sync_ops.h \
@@ -1069,6 +1070,9 @@ libsss_ipa_la_SOURCES += src/providers/ldap/sdap_sudo_cache.c \ src/providers/ldap/sdap_sudo_timer.c \ src/providers/ldap/sdap_sudo.c endif +if BUILD_SSH +libsss_ipa_la_SOURCES += src/providers/ipa/ipa_auxid.c +endif
krb5_child_SOURCES = \ diff --git a/src/providers/ipa/ipa_auxid.c b/src/providers/ipa/ipa_auxid.c new file mode 100644 index 0000000..eb38fbe --- /dev/null +++ b/src/providers/ipa/ipa_auxid.c @@ -0,0 +1,59 @@ +/*
- Authors:
Jan Cholasta <jcholast@redhat.com>- Copyright (C) 2012 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/.
+*/
+#include "util/util.h" +#include "util/crypto/sss_crypto.h" +#include "providers/ldap/ldap_common.h" +#include "providers/ipa/ipa_common.h" +#include "providers/ipa/ipa_auxid.h" +#include "providers/ipa/ipa_hosts.h" +#include "providers/ipa/ipa_hbac_private.h"
+void +ipa_auxiliary_info_handler(struct be_req *breq) +{
- struct ipa_auxid_ctx *auxid_ctx;
- struct sdap_id_ctx *ctx;
- struct be_acct_req *ar;
- struct tevent_req *req;
- const char *err = "Unknown Error";
- errno_t ret = EOK;
- auxid_ctx = talloc_get_type(breq->be_ctx->bet_info[BET_AUXID].pvt_bet_data, struct ipa_auxid_ctx);
- ctx = auxid_ctx->sdap_id_ctx;
- if (be_is_offline(ctx->be)) {
return sdap_handler_done(breq, DP_ERR_OFFLINE, EAGAIN, "Offline");- }
- ar = talloc_get_type(breq->req_data, struct be_acct_req);
- switch (ar->entry_type & 0xFFF) {
- case BE_REQ_HOST:
ret = EINVAL;err = "Not implemented yet";- default:
ret = EINVAL;err = "Invalid request type";- }
- if (ret != EOK) return sdap_handler_done(breq, DP_ERR_FATAL, ret, err);
+} diff --git a/src/providers/ipa/ipa_auxid.h b/src/providers/ipa/ipa_auxid.h new file mode 100644 index 0000000..b09ac24 --- /dev/null +++ b/src/providers/ipa/ipa_auxid.h @@ -0,0 +1,33 @@ +/*
- Authors:
Jan Cholasta <jcholast@redhat.com>- Copyright (C) 2012 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/.
+*/
+#ifndef _IPA_AUXID_H_ +#define _IPA_AUXID_H_
+struct ipa_auxid_ctx {
- struct sdap_id_ctx *sdap_id_ctx;
- struct dp_option *ipa_options;
- struct sdap_search_base **host_search_bases;
+};
+void ipa_auxiliary_info_handler(struct be_req *be_req);
+#endif /* _IPA_AUXID_H_ */ diff --git a/src/providers/ipa/ipa_common.c b/src/providers/ipa/ipa_common.c index 790adb7..0121ca5 100644 --- a/src/providers/ipa/ipa_common.c +++ b/src/providers/ipa/ipa_common.c @@ -180,6 +180,7 @@ struct sdap_attr_map ipa_host_map[] = { { "ipa_host_object_class", "ipaHost", SYSDB_HOST_CLASS, NULL }, { "ipa_host_fqdn", "fqdn", SYSDB_NAME, NULL }, { "ipa_host_member_of", "memberOf", SYSDB_MEMBEROF, NULL },
- { "ipa_host_ssh_public_key", "ipaSshPubKey", SYSDB_SSH_PUBKEY, NULL }
};
struct dp_option ipa_def_krb5_opts[] = { diff --git a/src/providers/ipa/ipa_common.h b/src/providers/ipa/ipa_common.h index 9cbd993..5883fce 100644 --- a/src/providers/ipa/ipa_common.h +++ b/src/providers/ipa/ipa_common.h @@ -78,6 +78,7 @@ enum ipa_host_attrs { IPA_OC_HOST = 0, IPA_AT_HOST_FQDN, IPA_AT_HOST_MEMBER_OF,
IPA_AT_HOST_SSH_PUBLIC_KEY,
IPA_OPTS_HOST /* attrs counter */
}; diff --git a/src/providers/ipa/ipa_init.c b/src/providers/ipa/ipa_init.c index 78a102a..39a5b27 100644 --- a/src/providers/ipa/ipa_init.c +++ b/src/providers/ipa/ipa_init.c @@ -33,6 +33,7 @@ #include "providers/ipa/ipa_id.h" #include "providers/ipa/ipa_auth.h" #include "providers/ipa/ipa_access.h" +#include "providers/ipa/ipa_auxid.h" #include "providers/ipa/ipa_dyndns.h"
struct ipa_options *ipa_options = NULL; @@ -59,6 +60,11 @@ struct bet_ops ipa_access_ops = { .finalize = NULL };
+struct bet_ops ipa_auxid_ops = {
- .handler = ipa_auxiliary_info_handler,
- .finalize = NULL
+};
int common_ipa_init(struct be_ctx *bectx) { const char *ipa_servers; @@ -392,3 +398,42 @@ done: } return ret; }
+int sssm_ipa_auxid_init(struct be_ctx *bectx,
struct bet_ops **ops,void **pvt_data)+{
- int ret;
- struct ipa_auxid_ctx *auxid_ctx;
- struct ipa_id_ctx *id_ctx;
- auxid_ctx = talloc_zero(bectx, struct ipa_auxid_ctx);
- if (auxid_ctx == NULL) {
DEBUG(1, ("talloc_zero failed.\n"));return ENOMEM;- }
- ret = sssm_ipa_id_init(bectx, ops, (void **) &id_ctx);
- if (ret != EOK) {
DEBUG(1, ("sssm_ipa_id_init failed.\n"));goto done;- }
- auxid_ctx->sdap_id_ctx = id_ctx->sdap_id_ctx;
- auxid_ctx->host_search_bases = id_ctx->ipa_options->host_search_bases;
- ret = dp_copy_options(auxid_ctx, ipa_options->basic,
IPA_OPTS_BASIC, &auxid_ctx->ipa_options);- if (ret != EOK) {
DEBUG(1, ("dp_copy_options failed.\n"));goto done;- }
- *ops = &ipa_auxid_ops;
- *pvt_data = auxid_ctx;
+done:
- if (ret != EOK) {
talloc_free(auxid_ctx);- }
- return ret;
+}
1.7.6.5
From d25b63912f1ecff4c8de1f2b938ca6ad92cef84e Mon Sep 17 00:00:00 2001 From: Jan Cholasta jcholast@redhat.com Date: Fri, 3 Feb 2012 22:43:18 +0100 Subject: [PATCH 6/9] DP: Add support for hosts in sss_dp_get_account
Host requests are directed to the auxiliary info handler.
src/responder/common/responder.h | 3 ++- src/responder/common/responder_dp.c | 12 +++++++++++- 2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h index 1c85b9d..d3eface 100644 --- a/src/responder/common/responder.h +++ b/src/responder/common/responder.h @@ -231,7 +231,8 @@ enum sss_dp_acct_type { SSS_DP_GROUP, SSS_DP_INITGROUPS, SSS_DP_NETGR,
- SSS_DP_SERVICES
- SSS_DP_SERVICES,
- SSS_DP_HOST
};
struct tevent_req * diff --git a/src/responder/common/responder_dp.c b/src/responder/common/responder_dp.c index aa439a6..fd8b0bf 100644 --- a/src/responder/common/responder_dp.c +++ b/src/responder/common/responder_dp.c @@ -487,24 +487,34 @@ sss_dp_get_account_msg(void *pvt) uint32_t be_type; uint32_t attrs = BE_ATTR_CORE; char *filter;
const char *dp_method;
info = talloc_get_type(pvt, struct sss_dp_account_info);
switch (info->type) { case SSS_DP_USER: be_type = BE_REQ_USER;
dp_method = DP_METHOD_GETACCTINFO; break; case SSS_DP_GROUP: be_type = BE_REQ_GROUP;dp_method = DP_METHOD_GETACCTINFO; break; case SSS_DP_INITGROUPS: be_type = BE_REQ_INITGROUPS;dp_method = DP_METHOD_GETACCTINFO; break; case SSS_DP_NETGR: be_type = BE_REQ_NETGROUP;dp_method = DP_METHOD_GETACCTINFO; break; case SSS_DP_SERVICES: be_type = BE_REQ_SERVICES;dp_method = DP_METHOD_GETACCTINFO;break;case SSS_DP_HOST:be_type = BE_REQ_HOST;dp_method = DP_METHOD_GETAUXINFO; break;}
@@ -537,7 +547,7 @@ sss_dp_get_account_msg(void *pvt) msg = dbus_message_new_method_call(NULL, DP_PATH, DP_INTERFACE,
DP_METHOD_GETACCTINFO);
dp_method);-- 1.7.6.5
From 5c9e073df33f3e9eadb3233b050806f8932634db Mon Sep 17 00:00:00 2001 From: Jan Cholasta jcholast@redhat.com Date: Fri, 3 Feb 2012 22:48:28 +0100 Subject: [PATCH 7/9] SSH: Responder
Makefile.am | 15 + src/confdb/confdb.h | 3 + src/monitor/monitor.c | 2 +- src/providers/data_provider_be.c | 5 + src/providers/dp_backend.h | 1 + src/responder/ssh/sshsrv.c | 193 +++++++++++ src/responder/ssh/sshsrv_cmd.c | 669 ++++++++++++++++++++++++++++++++++++ src/responder/ssh/sshsrv_private.h | 49 +++ 8 files changed, 936 insertions(+), 1 deletions(-) create mode 100644 src/responder/ssh/sshsrv.c create mode 100644 src/responder/ssh/sshsrv_cmd.c create mode 100644 src/responder/ssh/sshsrv_private.h
diff --git a/Makefile.am b/Makefile.am index f2569df..796a93b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -81,6 +81,9 @@ sssdlibexec_PROGRAMS = \ if BUILD_SUDO sssdlibexec_PROGRAMS += sssd_sudo endif +if BUILD_SSH +sssdlibexec_PROGRAMS += sssd_ssh +endif
if HAVE_CHECK non_interactive_check_based_tests = \ @@ -217,6 +220,7 @@ AM_CPPFLAGS = \ -DSSS_PAM_SOCKET_NAME="$(pipepath)/pam" \ -DSSS_PAM_PRIV_SOCKET_NAME="$(pipepath)/private/pam" \ -DSSS_SUDO_SOCKET_NAME="$(pipepath)/sudo" \
- -DSSS_SSH_SOCKET_NAME="$(pipepath)/ssh" \ -DLOCALEDIR="$(localedir)"
EXTRA_DIST = build/config.rpath @@ -321,6 +325,7 @@ dist_noinst_HEADERS = \ src/responder/nss/nsssrv_services.h \ src/responder/common/negcache.h \ src/responder/sudo/sudosrv_private.h \
- src/responder/ssh/sshsrv_private.h \ src/sbus/sbus_client.h \ src/sbus/sssd_dbus.h \ src/sbus/sssd_dbus_private.h \
@@ -479,6 +484,16 @@ sssd_sudo_LDADD = \ libsss_util.la endif
+if BUILD_SSH +sssd_ssh_SOURCES = \
- src/responder/ssh/sshsrv.c \
- src/responder/ssh/sshsrv_cmd.c \
- $(SSSD_RESPONDER_OBJ)
+sssd_ssh_LDADD = \
- $(SSSD_LIBS) \
- libsss_util.la
+endif
sssd_be_SOURCES = \ src/providers/data_provider_be.c \ src/providers/data_provider_fo.c \ diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index 56b06b9..808f034 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -94,6 +94,9 @@ /* SUDO */ #define CONFDB_SUDO_CONF_ENTRY "config/sudo"
+/* SSH */ +#define CONFDB_SSH_CONF_ENTRY "config/ssh"
/* Data Provider */ #define CONFDB_DP_CONF_ENTRY "config/dp"
diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c index d2717fa..4690f9f 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c @@ -754,7 +754,7 @@ static int check_local_domain_unique(struct sss_domain_info *domains)
static char *check_services(char **services) {
- const char *known_services[] = { "nss", "pam", "sudo", NULL };
- const char *known_services[] = { "nss", "pam", "sudo", "ssh", NULL }; int i; int ii;
diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c index 35d705a..405b993 100644 --- a/src/providers/data_provider_be.c +++ b/src/providers/data_provider_be.c @@ -853,6 +853,9 @@ static int be_client_destructor(void *ctx) } else if (becli->bectx->sudo_cli == becli) { DEBUG(4, ("Removed SUDO client\n")); becli->bectx->sudo_cli = NULL;
} else if (becli->bectx->ssh_cli == becli) {DEBUG(4, ("Removed SSH client\n"));becli->bectx->ssh_cli = NULL; } else { DEBUG(2, ("Unknown client removed ...\n")); }@@ -903,6 +906,8 @@ static int client_registration(DBusMessage *message, becli->bectx->pam_cli = becli; } else if (strcasecmp(cli_name, "SUDO") == 0) { becli->bectx->sudo_cli = becli;
- } else if (strcasecmp(cli_name, "SSH") == 0) {
becli->bectx->ssh_cli = becli;diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h index 1a36778..023d3d9 100644 --- a/src/providers/dp_backend.h +++ b/src/providers/dp_backend.h @@ -113,6 +113,7 @@ struct be_ctx { struct be_client *nss_cli; struct be_client *pam_cli; struct be_client *sudo_cli;
struct be_client *ssh_cli;
struct loaded_be loaded_be[BET_MAX]; struct bet_info bet_info[BET_MAX];
diff --git a/src/responder/ssh/sshsrv.c b/src/responder/ssh/sshsrv.c new file mode 100644 index 0000000..e5bb5b0 --- /dev/null +++ b/src/responder/ssh/sshsrv.c @@ -0,0 +1,193 @@ +/*
- Authors:
Jan Cholasta <jcholast@redhat.com>- Copyright (C) 2012 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/.
+*/
+#include <popt.h>
+#include "util/util.h" +#include "confdb/confdb.h" +#include "monitor/monitor_interfaces.h" +#include "responder/common/responder.h" +#include "responder/ssh/sshsrv_private.h" +#include "providers/data_provider.h"
+struct sbus_method monitor_ssh_methods[] = {
- { MON_CLI_METHOD_PING, monitor_common_pong },
- { MON_CLI_METHOD_RES_INIT, monitor_common_res_init },
- { MON_CLI_METHOD_ROTATE, responder_logrotate },
- { NULL, NULL }
+};
+struct sbus_interface monitor_ssh_interface = {
- MONITOR_INTERFACE,
- MONITOR_PATH,
- SBUS_DEFAULT_VTABLE,
- monitor_ssh_methods,
- NULL
+};
+static struct sbus_method ssh_dp_methods[] = {
- { NULL, NULL }
+};
+struct sbus_interface ssh_dp_interface = {
- DP_INTERFACE,
- DP_PATH,
- SBUS_DEFAULT_VTABLE,
- ssh_dp_methods,
- NULL
+};
+static void ssh_dp_reconnect_init(struct sbus_connection *conn,
int status, void *pvt)+{
- struct be_conn *be_conn = talloc_get_type(pvt, struct be_conn);
- int ret;
- /* Did we reconnect successfully? */
- if (status == SBUS_RECONNECT_SUCCESS) {
DEBUG(SSSDBG_TRACE_FUNC, ("Reconnected to the Data Provider.\n"));/* Identify ourselves to the data provider */ret = dp_common_send_id(be_conn->conn,DATA_PROVIDER_VERSION,"SSH");/* all fine */if (ret == EOK) {handle_requests_after_reconnect();return;}- }
- /* Failed to reconnect */
- DEBUG(SSSDBG_FATAL_FAILURE, ("Could not reconnect to %s provider.\n",
be_conn->domain->name));+}
+int ssh_process_init(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,struct confdb_ctx *cdb)+{
- struct sss_cmd_table *ssh_cmds;
- struct ssh_ctx *ssh_ctx;
- struct be_conn *iter;
- int ret;
- int max_retries;
- ssh_ctx = talloc_zero(mem_ctx, struct ssh_ctx);
- if (!ssh_ctx) {
DEBUG(SSSDBG_FATAL_FAILURE, ("fatal error initializing ssh_ctx\n"));return ENOMEM;- }
- ssh_cmds = get_ssh_cmds();
- ret = sss_process_init(ssh_ctx, ev, cdb,
ssh_cmds,SSS_SSH_SOCKET_NAME, NULL,CONFDB_SSH_CONF_ENTRY,SSS_SSH_SBUS_SERVICE_NAME,SSS_SSH_SBUS_SERVICE_VERSION,&monitor_ssh_interface,"SSH",&ssh_dp_interface,&ssh_ctx->rctx);- if (ret != EOK) {
return ret;- }
- ssh_ctx->rctx->pvt_ctx = ssh_ctx;
- /* Enable automatic reconnection to the Data Provider */
- ret = confdb_get_int(ssh_ctx->rctx->cdb, ssh_ctx->rctx,
CONFDB_SSH_CONF_ENTRY,CONFDB_SERVICE_RECON_RETRIES,3, &max_retries);- if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE,("Failed to set up automatic reconnection\n"));return ret;- }
- for (iter = ssh_ctx->rctx->be_conns; iter; iter = iter->next) {
sbus_reconnect_init(iter->conn, max_retries,ssh_dp_reconnect_init, iter);- }
- DEBUG(SSSDBG_TRACE_FUNC, ("SSH Initialization complete\n"));
- return EOK;
+}
+int main(int argc, const char *argv[]) +{
- int opt;
- poptContext pc;
- struct main_context *main_ctx;
- int ret;
- struct poptOption long_options[] = {
POPT_AUTOHELPSSSD_MAIN_OPTSPOPT_TABLEEND- };
- /* Set debug level to invalid value so we can deside if -d 0 was used. */
- debug_level = SSSDBG_INVALID;
- pc = poptGetContext(argv[0], argc, argv, long_options, 0);
- while((opt = poptGetNextOpt(pc)) != -1) {
switch(opt) {default:fprintf(stderr, "\nInvalid option %s: %s\n\n",poptBadOption(pc, 0), poptStrerror(opt));poptPrintUsage(pc, stderr, 0);return 1;}- }
- poptFreeContext(pc);
- CONVERT_AND_SET_DEBUG_LEVEL(debug_level);
- /* set up things like debug, signals, daemonization, etc... */
- debug_log_file = "sssd_ssh";
- ret = server_setup("sssd[ssh]", 0, CONFDB_SSH_CONF_ENTRY, &main_ctx);
- if (ret != EOK) {
return 2;- }
- ret = die_if_parent_died();
- if (ret != EOK) {
/* This is not fatal, don't return */DEBUG(SSSDBG_OP_FAILURE, ("Could not set up to exit ""when parent process does\n"));- }
- ret = ssh_process_init(main_ctx,
main_ctx->event_ctx,main_ctx->confdb_ctx);- if (ret != EOK) {
return 3;- }
- /* loop on main */
- server_loop(main_ctx);
- return 0;
+} diff --git a/src/responder/ssh/sshsrv_cmd.c b/src/responder/ssh/sshsrv_cmd.c new file mode 100644 index 0000000..386b1ae --- /dev/null +++ b/src/responder/ssh/sshsrv_cmd.c @@ -0,0 +1,669 @@ +/*
- Authors:
Jan Cholasta <jcholast@redhat.com>- Copyright (C) 2012 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/.
+*/
+#include <talloc.h>
+#include "util/util.h" +#include "util/crypto/sss_crypto.h" +#include "db/sysdb.h" +#include "providers/ipa/ipa_hbac_private.h" +#include "responder/common/responder.h" +#include "responder/common/responder_packet.h" +#include "responder/ssh/sshsrv_private.h"
+static errno_t +ssh_cmd_parse_request(struct ssh_cmd_ctx *cmd_ctx);
+static errno_t +ssh_cmd_done(struct ssh_cmd_ctx *cmd_ctx,
errno_t ret);+static errno_t +ssh_user_pubkeys_search(struct ssh_cmd_ctx *cmd_ctx);
+int +sss_ssh_cmd_get_user_pubkeys(struct cli_ctx *cctx) +{
- errno_t ret;
- struct ssh_cmd_ctx *cmd_ctx;
- cmd_ctx = talloc_zero(cctx, struct ssh_cmd_ctx);
- if (!cmd_ctx) {
return ENOMEM;- }
- cmd_ctx->cctx = cctx;
- cmd_ctx->type = SSS_DP_USER;
- ret = ssh_cmd_parse_request(cmd_ctx);
- if (ret != EOK) {
goto done;- }
- DEBUG(SSSDBG_TRACE_FUNC,
("Requesting SSH user public keys for [%s] from [%s]\n",cmd_ctx->name, cmd_ctx->domname ? cmd_ctx->domname : "<ALL>"));- if (cmd_ctx->domname) {
cmd_ctx->domain = responder_get_domain(cctx->rctx->domains,cmd_ctx->domname);if (!cmd_ctx->domain) {ret = ENOENT;goto done;}- } else {
cmd_ctx->domain = cctx->rctx->domains;cmd_ctx->check_next = true;- }
- cmd_ctx->check_provider = NEED_CHECK_PROVIDER(cmd_ctx->domain->provider);
- ret = ssh_user_pubkeys_search(cmd_ctx);
+done:
- return ssh_cmd_done(cmd_ctx, ret);
+}
+static errno_t +ssh_host_pubkeys_search(struct ssh_cmd_ctx *cmd_ctx);
+static int +sss_ssh_cmd_get_host_pubkeys(struct cli_ctx *cctx) +{
- struct ssh_cmd_ctx *cmd_ctx;
- errno_t ret;
- cmd_ctx = talloc_zero(cctx, struct ssh_cmd_ctx);
- if (!cmd_ctx) {
return ENOMEM;- }
- cmd_ctx->cctx = cctx;
- cmd_ctx->type = SSS_DP_HOST;
- ret = ssh_cmd_parse_request(cmd_ctx);
- if (ret != EOK) {
goto done;- }
- DEBUG(SSSDBG_TRACE_FUNC,
("Requesting SSH host public keys for [%s] from [%s]\n",cmd_ctx->name, cmd_ctx->domname ? cmd_ctx->domname : "<ALL>"));- if (cmd_ctx->domname) {
cmd_ctx->domain = responder_get_domain(cctx->rctx->domains,cmd_ctx->domname);if (!cmd_ctx->domain) {ret = ENOENT;goto done;}- } else {
cmd_ctx->domain = cctx->rctx->domains;cmd_ctx->check_next = true;- }
- cmd_ctx->check_provider = NEED_CHECK_PROVIDER(cmd_ctx->domain->provider);
- ret = ssh_host_pubkeys_search(cmd_ctx);
+done:
- return ssh_cmd_done(cmd_ctx, ret);
+}
+static void +ssh_dp_send_req_done(struct tevent_req *req) +{
- struct dp_callback_ctx *cb_ctx =
tevent_req_callback_data(req, struct dp_callback_ctx);- errno_t ret;
- dbus_uint16_t err_maj;
- dbus_uint32_t err_min;
- char *err_msg;
- ret = sss_dp_get_account_recv(cb_ctx->mem_ctx, req,
&err_maj, &err_min,&err_msg);- talloc_zfree(req);
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,("Fatal error, killing connection!\n"));talloc_free(cb_ctx->cctx);return;- }
- cb_ctx->callback(err_maj, err_min, err_msg, cb_ctx->ptr);
+}
+static void +ssh_user_pubkeys_search_dp_callback(uint16_t err_maj,
uint32_t err_min,const char *err_msg,void *ptr);+static errno_t +ssh_user_pubkeys_search(struct ssh_cmd_ctx *cmd_ctx) +{
- errno_t ret;
- struct sss_domain_info *dom = cmd_ctx->domain;
- struct cli_ctx *cctx = cmd_ctx->cctx;
- struct sysdb_ctx *sysdb;
- struct tevent_req *req;
- struct dp_callback_ctx *cb_ctx;
- const char *attrs[] = { SYSDB_NAME, SYSDB_SSH_PUBKEY, NULL };
- struct ldb_result *res;
- while (dom) {
/* if it is a domainless search, skip domains that require fully* qualified names instead */while (dom && cmd_ctx->check_next && dom->fqnames) {dom = dom->next;}if (!dom) break;if (dom != cmd_ctx->domain) {/* make sure we reset the check_provider flag when we check* a new domain */cmd_ctx->check_provider = NEED_CHECK_PROVIDER(dom->provider);}/* make sure to update the dom_ctx if we changed domain */cmd_ctx->domain = dom;/* refresh the user's cache entry */if (cmd_ctx->check_provider) {cmd_ctx->check_provider = false;req = sss_dp_get_account_send(cmd_ctx, cmd_ctx->cctx->rctx,dom, false, SSS_DP_USER,cmd_ctx->name, 0, NULL);if (!req) {DEBUG(SSSDBG_CRIT_FAILURE,("Out of memory sending data provider request\n"));return ENOMEM;}cb_ctx = talloc_zero(cmd_ctx, struct dp_callback_ctx);if (!cb_ctx) {talloc_zfree(req);return ENOMEM;}cb_ctx->callback = ssh_user_pubkeys_search_dp_callback;cb_ctx->ptr = cmd_ctx;cb_ctx->cctx = cmd_ctx->cctx;cb_ctx->mem_ctx = cmd_ctx;tevent_req_set_callback(req, ssh_dp_send_req_done, cb_ctx);/* tell caller we are in an async call */return EAGAIN;}DEBUG(SSSDBG_TRACE_FUNC,("Requesting SSH user public keys for [%s@%s]\n",cmd_ctx->name, dom->name));ret = sysdb_get_ctx_from_list(cctx->rctx->db_list, dom, &sysdb);if (ret != EOK) {DEBUG(SSSDBG_FATAL_FAILURE,("Fatal: Sysdb CTX not found for this domain!\n"));return EFAULT;}ret = sysdb_get_user_attr(cmd_ctx, sysdb,cmd_ctx->name, attrs, &res);if (ret != EOK) {DEBUG(SSSDBG_CRIT_FAILURE,("Failed to make request to our cache!\n"));return EIO;}cmd_ctx->results = res->msgs;cmd_ctx->results_len = res->count;if (cmd_ctx->results_len > 1) {DEBUG(SSSDBG_FATAL_FAILURE,("User search by name (%s) returned > 1 results!\n",cmd_ctx->name));return ENOENT;}if (cmd_ctx->results_len == 0) {/* if a multidomain search, try with next */if (cmd_ctx->check_next) {dom = dom->next;continue;}DEBUG(SSSDBG_OP_FAILURE,("No attributes for user [%s] found.\n", cmd_ctx->name));return ENOENT;}/* one result found */return EOK;- }
- DEBUG(SSSDBG_OP_FAILURE,
("No matching domain found for [%s], fail!\n", cmd_ctx->name));- return ENOENT;
+}
+static void +ssh_user_pubkeys_search_dp_callback(uint16_t err_maj,
uint32_t err_min,const char *err_msg,void *ptr)+{
- struct ssh_cmd_ctx *cmd_ctx = talloc_get_type(ptr, struct ssh_cmd_ctx);
- errno_t ret;
- if (err_maj) {
DEBUG(SSSDBG_OP_FAILURE,("Unable to get information from Data Provider\n""Error: %u, %u, %s\n",(unsigned int)err_maj, (unsigned int)err_min, err_msg));if (!cmd_ctx->check_next) {ret = ENOENT;goto done;}cmd_ctx->domain = cmd_ctx->domain->next;if (cmd_ctx->domain) {cmd_ctx->check_provider = NEED_CHECK_PROVIDER(cmd_ctx->domain->provider);}- }
- ret = ssh_user_pubkeys_search(cmd_ctx);
+done:
- ssh_cmd_done(cmd_ctx, ret);
+}
+static void +ssh_host_pubkeys_search_dp_callback(uint16_t err_maj,
uint32_t err_min,const char *err_msg,void *ptr);+static errno_t +ssh_host_pubkeys_search(struct ssh_cmd_ctx *cmd_ctx) +{
- errno_t ret;
- struct sss_domain_info *dom = cmd_ctx->domain;
- struct cli_ctx *cctx = cmd_ctx->cctx;
- struct sysdb_ctx *sysdb;
- struct tevent_req *req;
- struct dp_callback_ctx *cb_ctx;
- const char *attrs[] = { SYSDB_NAME, SYSDB_SSH_PUBKEY, NULL };
- while (dom) {
/* if it is a domainless search, skip domains that require fully* qualified names instead */while (dom && cmd_ctx->check_next && dom->fqnames) {dom = dom->next;}if (!dom) break;if (dom != cmd_ctx->domain) {/* make sure we reset the check_provider flag when we check* a new domain */cmd_ctx->check_provider = NEED_CHECK_PROVIDER(dom->provider);}/* make sure to update the dom_ctx if we changed domain */cmd_ctx->domain = dom;/* refresh the host's cache entry */if (cmd_ctx->check_provider) {cmd_ctx->check_provider = false;req = sss_dp_get_account_send(cmd_ctx, cmd_ctx->cctx->rctx,dom, false, SSS_DP_HOST,cmd_ctx->name, 0, NULL);if (!req) {DEBUG(SSSDBG_CRIT_FAILURE,("Out of memory sending data provider request\n"));return ENOMEM;}cb_ctx = talloc_zero(cmd_ctx, struct dp_callback_ctx);if (!cb_ctx) {talloc_zfree(req);return ENOMEM;}cb_ctx->callback = ssh_host_pubkeys_search_dp_callback;cb_ctx->ptr = cmd_ctx;cb_ctx->cctx = cmd_ctx->cctx;cb_ctx->mem_ctx = cmd_ctx;tevent_req_set_callback(req, ssh_dp_send_req_done, cb_ctx);/* tell caller we are in an async call */return EAGAIN;}DEBUG(SSSDBG_TRACE_FUNC,("Requesting SSH host public keys for [%s@%s]\n",cmd_ctx->name, dom->name));ret = sysdb_get_ctx_from_list(cctx->rctx->db_list, dom, &sysdb);if (ret != EOK) {DEBUG(SSSDBG_FATAL_FAILURE,("Fatal: Sysdb CTX not found for this domain!\n"));return EFAULT;}ret = sysdb_search_custom_by_name(cmd_ctx, sysdb, cmd_ctx->name,HBAC_HOSTS_SUBDIR, attrs,&cmd_ctx->results_len,&cmd_ctx->results);if (ret != EOK) {DEBUG(SSSDBG_CRIT_FAILURE,("Failed to make request to our cache!\n"));return EIO;}if (cmd_ctx->results_len > 1) {DEBUG(SSSDBG_FATAL_FAILURE,("Host search by name (%s) returned > 1 results!\n",cmd_ctx->name));return ENOENT;}if (cmd_ctx->results_len == 0) {/* if a multidomain search, try with next */if (cmd_ctx->check_next) {dom = dom->next;continue;}DEBUG(SSSDBG_OP_FAILURE,("No attributes for host [%s] found.\n", cmd_ctx->name));return ENOENT;}/* one result found */return EOK;- }
- DEBUG(SSSDBG_OP_FAILURE,
("No matching domain found for [%s], fail!\n", cmd_ctx->name));- return ENOENT;
+}
+static void +ssh_host_pubkeys_search_dp_callback(uint16_t err_maj,
uint32_t err_min,const char *err_msg,void *ptr)+{
- struct ssh_cmd_ctx *cmd_ctx = talloc_get_type(ptr, struct ssh_cmd_ctx);
- errno_t ret;
- if (err_maj) {
DEBUG(SSSDBG_OP_FAILURE,("Unable to get information from Data Provider\n""Error: %u, %u, %s\n",(unsigned int)err_maj, (unsigned int)err_min, err_msg));if (!cmd_ctx->check_next) {ret = ENOENT;goto done;}cmd_ctx->domain = cmd_ctx->domain->next;if (cmd_ctx->domain) {cmd_ctx->check_provider = NEED_CHECK_PROVIDER(cmd_ctx->domain->provider);}- }
- ret = ssh_host_pubkeys_search(cmd_ctx);
+done:
- ssh_cmd_done(cmd_ctx, ret);
+}
+static errno_t +ssh_cmd_parse_request(struct ssh_cmd_ctx *cmd_ctx) +{
- struct cli_ctx *cctx = cmd_ctx->cctx;
- errno_t ret;
- uint8_t *body;
- size_t body_len;
- size_t c = 0;
- uint32_t reserved;
- uint32_t name_len;
- char *name;
- sss_packet_get_body(cctx->creq->in, &body, &body_len);
- SAFEALIGN_COPY_UINT32_CHECK(&reserved, body+c, body_len, &c);
- if (reserved != 0) {
return EINVAL;- }
- SAFEALIGN_COPY_UINT32_CHECK(&name_len, body+c, body_len, &c);
- if (name_len == 0) {
return EINVAL;- }
- name = (char *)(body+c);
- if (strnlen(name, name_len) != name_len-1) {
return EINVAL;- }
- c += name_len;
- ret = sss_parse_name(cmd_ctx, cctx->rctx->names, name,
&cmd_ctx->domname, &cmd_ctx->name);- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, ("Invalid name received [%s]\n", name));return ENOENT;- }
- return EOK;
+}
+static errno_t +ssh_cmd_build_reply(struct ssh_cmd_ctx *cmd_ctx) +{
- struct cli_ctx *cctx = cmd_ctx->cctx;
- errno_t ret;
- uint8_t *body;
- size_t body_len;
- size_t c = 0;
- size_t i;
- unsigned int j;
- struct ldb_message_element *el;
- uint32_t count = 0;
- const char *name;
- char *fqname;
- uint32_t fqname_len;
- uint8_t *key;
- size_t key_len;
- /* count number of results */
- for (i = 0; i < cmd_ctx->results_len; i++) {
el = ldb_msg_find_element(cmd_ctx->results[i], SYSDB_SSH_PUBKEY);if (!el) {continue;}count += el->num_values;- }
- ret = sss_packet_grow(cctx->creq->out, 2*sizeof(uint32_t));
- if (ret != EOK) {
return ret;- }
- sss_packet_get_body(cctx->creq->out, &body, &body_len);
- SAFEALIGN_SET_UINT32(body+c, count, &c);
- SAFEALIGN_SET_UINT32(body+c, 0, &c);
- for (i = 0; i < cmd_ctx->results_len; i++) {
name = ldb_msg_find_attr_as_string(cmd_ctx->results[i],SYSDB_NAME, NULL);if (!name) {DEBUG(SSSDBG_OP_FAILURE,("Got unnamed result for [%s@%s]\n",cmd_ctx->name, cmd_ctx->domain->name));return ENOENT;}fqname = talloc_asprintf(cmd_ctx, "%s@%s",name, cmd_ctx->domain->name);if (!fqname) {return ENOMEM;}fqname_len = strlen(fqname)+1;el = ldb_msg_find_element(cmd_ctx->results[i], SYSDB_SSH_PUBKEY);if (!el) {/* this object has no SSH public keys */continue;}for (j = 0; j < el->num_values; j++) {key = sss_base64_decode(cmd_ctx,(const char *)el->values[j].data,&key_len);if (!key) {return ENOMEM;}ret = sss_packet_grow(cctx->creq->out,3*sizeof(uint32_t) + key_len + fqname_len);if (ret != EOK) {talloc_free(key);return ret;}sss_packet_get_body(cctx->creq->out, &body, &body_len);SAFEALIGN_SET_UINT32(body+c, 0, &c);SAFEALIGN_SET_UINT32(body+c, fqname_len, &c);safealign_memcpy(body+c, fqname, fqname_len, &c);SAFEALIGN_SET_UINT32(body+c, key_len, &c);safealign_memcpy(body+c, key, key_len, &c);talloc_free(key);count++;}- }
- return EOK;
+}
+static errno_t +ssh_cmd_send_error(struct ssh_cmd_ctx *cmd_ctx,
errno_t error)+{
- struct cli_ctx *cctx = cmd_ctx->cctx;
- errno_t ret;
- ret = sss_cmd_send_error(cctx, error);
- if (ret != EOK) {
return ret;- }
- sss_cmd_done(cctx, cmd_ctx);
- return EOK;
+}
+static errno_t +ssh_cmd_send_reply(struct ssh_cmd_ctx *cmd_ctx) +{
- struct cli_ctx *cctx = cmd_ctx->cctx;
- errno_t ret;
- /* create response packet */
- ret = sss_packet_new(cctx->creq, 0,
sss_packet_get_cmd(cctx->creq->in),&cctx->creq->out);- if (ret != EOK) {
return ret;- }
- ret = ssh_cmd_build_reply(cmd_ctx);
- if (ret != EOK) {
return ret;- }
- sss_packet_set_error(cctx->creq->out, EOK);
- sss_cmd_done(cctx, cmd_ctx);
- return EOK;
+}
+static errno_t +ssh_cmd_done(struct ssh_cmd_ctx *cmd_ctx,
errno_t ret)+{
- switch (ret) {
- case EOK:
ret = ssh_cmd_send_reply(cmd_ctx);break;- case EAGAIN:
return EOK;- case EFAULT:
break;- default:
ret = ssh_cmd_send_error(cmd_ctx, ret);break;- }
- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Fatal error, killing connection!\n"));talloc_free(cmd_ctx->cctx);return EFAULT;- }
- return EOK;
+}
+struct cli_protocol_version *register_cli_protocol_version(void) +{
- static struct cli_protocol_version ssh_cli_protocol_version[] = {
{0, NULL, NULL}- };
- return ssh_cli_protocol_version;
+}
+struct sss_cmd_table *get_ssh_cmds(void) {
- static struct sss_cmd_table ssh_cmds[] = {
{SSS_GET_VERSION, sss_cmd_get_version},{SSS_SSH_GET_USER_PUBKEYS, sss_ssh_cmd_get_user_pubkeys},{SSS_SSH_GET_HOST_PUBKEYS, sss_ssh_cmd_get_host_pubkeys},{SSS_CLI_NULL, NULL}- };
- return ssh_cmds;
+} diff --git a/src/responder/ssh/sshsrv_private.h b/src/responder/ssh/sshsrv_private.h new file mode 100644 index 0000000..9246d78 --- /dev/null +++ b/src/responder/ssh/sshsrv_private.h @@ -0,0 +1,49 @@ +/*
- Authors:
Jan Cholasta <jcholast@redhat.com>- Copyright (C) 2012 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/.
+*/
+#ifndef _SSHSRV_PRIVATE_H_ +#define _SSHSRV_PRIVATE_H_
+#include "responder/common/responder.h"
+#define SSS_SSH_SBUS_SERVICE_VERSION 0x0001 +#define SSS_SSH_SBUS_SERVICE_NAME "ssh"
+struct ssh_ctx {
- struct resp_ctx *rctx;
+};
+struct ssh_cmd_ctx {
- struct cli_ctx *cctx;
- enum sss_dp_acct_type type;
- char *name;
- char *domname;
- struct sss_domain_info *domain;
- bool check_next;
- bool check_provider;
- struct ldb_message **results;
- size_t results_len;
+};
+struct sss_cmd_table *get_ssh_cmds(void);
+#endif /* _SSHSRV_PRIVATE_H_ */
1.7.6.5
From ba3426010fc5f87fc6fb72b24a0ac67b2a28ab40 Mon Sep 17 00:00:00 2001 From: Jan Cholasta jcholast@redhat.com Date: Fri, 3 Feb 2012 22:52:08 +0100 Subject: [PATCH 8/9] SSH: Common client code
Makefile.am | 4 +- src/sss_client/common.c | 19 ++++ src/sss_client/ssh/sss_ssh.c | 234 ++++++++++++++++++++++++++++++++++++++++++ src/sss_client/ssh/sss_ssh.h | 52 +++++++++ src/sss_client/sss_cli.h | 10 ++ 5 files changed, 318 insertions(+), 1 deletions(-) create mode 100644 src/sss_client/ssh/sss_ssh.c create mode 100644 src/sss_client/ssh/sss_ssh.h
diff --git a/Makefile.am b/Makefile.am index 796a93b..2ffd453 100644 --- a/Makefile.am +++ b/Makefile.am @@ -368,7 +368,9 @@ dist_noinst_HEADERS = \ src/resolv/ares/ares_parse_srv_reply.h \ src/resolv/ares/ares_parse_txt_reply.h \ src/resolv/ares/ares_data.h \
- src/tests/common.h
- src/tests/common.h \
- src/sss_client/ssh/sss_ssh.h
if HAVE_NSS dist_noinst_HEADERS += src/util/crypto/nss/nss_util.h diff --git a/src/sss_client/common.c b/src/sss_client/common.c index 998f7c8..6fcfad6 100644 --- a/src/sss_client/common.c +++ b/src/sss_client/common.c @@ -379,6 +379,8 @@ static bool sss_cli_check_version(const char *socket_name) expected_version = SSS_PAM_PROTOCOL_VERSION; } else if (strcmp(socket_name, SSS_SUDO_SOCKET_NAME) == 0) { expected_version = SSS_SUDO_PROTOCOL_VERSION;
- } else if (strcmp(socket_name, SSS_SSH_SOCKET_NAME) == 0) {
expected_version = SSS_SSH_PROTOCOL_VERSION;@@ -830,6 +832,23 @@ int sss_sudo_make_request(enum sss_cli_command cmd, return ret; }
+int sss_ssh_make_request(enum sss_cli_command cmd,
struct sss_cli_req_data *rd,uint8_t **repbuf, size_t *replen,int *errnop)+{
- enum sss_status ret = SSS_STATUS_UNAVAIL;
- ret = sss_cli_check_socket(errnop, SSS_SSH_SOCKET_NAME);
- if (ret != SSS_STATUS_SUCCESS) {
return SSS_STATUS_UNAVAIL;- }
- ret = sss_cli_make_request_nochecks(cmd, rd, repbuf, replen, errnop);
- return ret;
+}
const char *ssscli_err2string(int err) { const char *m; diff --git a/src/sss_client/ssh/sss_ssh.c b/src/sss_client/ssh/sss_ssh.c new file mode 100644 index 0000000..0177be9 --- /dev/null +++ b/src/sss_client/ssh/sss_ssh.c @@ -0,0 +1,234 @@ +/*
- Authors:
Jan Cholasta <jcholast@redhat.com>- Copyright (C) 2012 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/.
+*/
+#include <errno.h> +#include <stdlib.h> +#include <stdio.h> +#include <arpa/inet.h> +#include <talloc.h>
+#include "util/crypto/sss_crypto.h" +#include "sss_client/sss_cli.h" +#include "sss_client/ssh/sss_ssh.h"
+errno_t +sss_ssh_get_pubkeys(TALLOC_CTX *mem_ctx,
enum sss_cli_command command,const char *name,struct sss_ssh_pubkey **pubkeys,size_t *pubkeys_len)+{
- TALLOC_CTX *tmp_ctx;
- errno_t ret = EOK;
- uint32_t name_len;
- size_t req_len;
- uint8_t *req = NULL;
- size_t c = 0;
- struct sss_cli_req_data rd;
- int req_ret, req_errno;
- uint8_t *rep = NULL;
- size_t rep_len;
- uint32_t count, reserved, len, i;
- struct sss_ssh_pubkey *result = NULL;
- tmp_ctx = talloc_new(mem_ctx);
- if (!tmp_ctx) {
return ENOMEM;- }
- /* build request */
- name_len = strlen(name)+1;
- req_len = 2*sizeof(uint32_t) + name_len;
- req = talloc_array(tmp_ctx, uint8_t, req_len);
- if (!req) {
ret = ENOMEM;goto done;- }
- SAFEALIGN_SET_UINT32(req+c, 0, &c);
- SAFEALIGN_SET_UINT32(req+c, name_len, &c);
- safealign_memcpy(req+c, name, name_len, &c);
- /* send request */
- rd.data = req;
- rd.len = req_len;
- req_ret = sss_ssh_make_request(command, &rd, &rep, &rep_len, &req_errno);
- if (req_ret != SSS_STATUS_SUCCESS) {
ret = EFAULT;goto done;- }
- if (req_errno != EOK) {
ret = req_errno;goto done;- }
- /* parse reply */
- c = 0;
- if (rep_len-c < 2*sizeof(uint32_t)) {
ret = EINVAL;goto done;- }
- SAFEALIGN_COPY_UINT32(&count, rep+c, &c);
- SAFEALIGN_COPY_UINT32(&reserved, rep+c, &c);
- if (reserved != 0) {
ret = EINVAL;goto done;- }
- if (count > 0) {
result = talloc_zero_array(tmp_ctx, struct sss_ssh_pubkey, count);if (!result) {ret = ENOMEM;goto done;}- }
- for (i = 0; i < count; i++) {
if (rep_len-c < 2*sizeof(uint32_t)) {ret = EINVAL;goto done;}SAFEALIGN_COPY_UINT32(&result[i].flags, rep+c, &c);if (result[i].flags != 0) {ret = EINVAL;goto done;}SAFEALIGN_COPY_UINT32(&len, rep+c, &c);if (rep_len-c < len + sizeof(uint32_t)) {ret = EINVAL;goto done;}result[i].name = talloc_array(result, char, len);if (!result[i].name) {ret = ENOMEM;goto done;}safealign_memcpy(result[i].name, rep+c, len, &c);if (strnlen(result[i].name, len) != len-1) {ret = EINVAL;goto done;}SAFEALIGN_COPY_UINT32(&len, rep+c, &c);if (rep_len-c < len) {ret = EINVAL;goto done;}result[i].key = talloc_array(result, uint8_t, len);if (!result[i].key) {ret = ENOMEM;goto done;}safealign_memcpy(result[i].key, rep+c, len, &c);result[i].key_len = len;- }
- *pubkeys = result ? talloc_steal(mem_ctx, result) : NULL;
- *pubkeys_len = count;
+done:
- talloc_free(tmp_ctx);
- return ret;
+}
+errno_t +sss_ssh_format_pubkey(TALLOC_CTX *mem_ctx,
struct sss_ssh_pubkey *pubkey,enum sss_ssh_pubkey_format format,char **result)+{
- TALLOC_CTX *tmp_ctx;
- errno_t ret = EOK;
- char *pk;
- size_t c = 0;
- uint32_t algo_len;
- char *algo;
- char *out;
- if (!pubkey) {
return EINVAL;- }
- tmp_ctx = talloc_new(mem_ctx);
- if (!tmp_ctx) {
return ENOMEM;- }
- pk = sss_base64_encode(tmp_ctx, pubkey->key, pubkey->key_len);
- if (!pk) {
ret = ENOMEM;goto done;- }
- switch (format) {
- case SSS_SSH_FORMAT_RAW:
/* base64-encoded key blob */out = talloc_steal(mem_ctx, pk);break;- case SSS_SSH_FORMAT_OPENSSH:
/* OpenSSH authorized_keys/known_hosts format */SAFEALIGN_COPY_UINT32(&algo_len, pubkey->key+c, &c);algo_len = ntohl(algo_len);algo = talloc_zero_array(tmp_ctx, char, algo_len+1);if (!algo) {ret = ENOMEM;goto done;}memcpy(algo, pubkey->key+c, algo_len);out = talloc_asprintf(tmp_ctx, "%s %s %s",algo, pk, pubkey->name);if (!out) {ret = ENOMEM;goto done;}talloc_steal(mem_ctx, out);break;- }
- *result = out;
+done:
- talloc_free(tmp_ctx);
- return ret;
+} diff --git a/src/sss_client/ssh/sss_ssh.h b/src/sss_client/ssh/sss_ssh.h new file mode 100644 index 0000000..4c85ff3 --- /dev/null +++ b/src/sss_client/ssh/sss_ssh.h @@ -0,0 +1,52 @@ +/*
- Authors:
Jan Cholasta <jcholast@redhat.com>- Copyright (C) 2012 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/.
+*/
+#ifndef _SSS_SSH_H_ +#define _SSS_SSH_H_
+struct sss_ssh_pubkey {
- uint32_t flags;
- char *name;
- uint8_t *key;
- size_t key_len;
+};
+#define SSS_SSH_PUBKEY_NOFLAGS 0
+errno_t +sss_ssh_get_pubkeys(TALLOC_CTX *mem_ctx,
enum sss_cli_command command,const char *name,struct sss_ssh_pubkey **pubkeys,size_t *pubkeys_len);+enum sss_ssh_pubkey_format {
- SSS_SSH_FORMAT_RAW,
- SSS_SSH_FORMAT_OPENSSH
+};
+errno_t +sss_ssh_format_pubkey(TALLOC_CTX *mem_ctx,
struct sss_ssh_pubkey *pubkey,enum sss_ssh_pubkey_format format,char **result);+#endif /* _SSS_SSH_H_ */ diff --git a/src/sss_client/sss_cli.h b/src/sss_client/sss_cli.h index dc137dc..eca61a8 100644 --- a/src/sss_client/sss_cli.h +++ b/src/sss_client/sss_cli.h @@ -45,6 +45,7 @@ typedef int errno_t; #define SSS_NSS_PROTOCOL_VERSION 1 #define SSS_PAM_PROTOCOL_VERSION 3 #define SSS_SUDO_PROTOCOL_VERSION 0 +#define SSS_SSH_PROTOCOL_VERSION 0
#ifdef LOGIN_NAME_MAX #define SSS_NAME_MAX LOGIN_NAME_MAX @@ -163,6 +164,10 @@ enum sss_cli_command { SSS_SUDO_GET_SUDORULES = 0x00C1, SSS_SUDO_GET_DEFAULTS = 0x00C2,
+/* SSH */
- SSS_SSH_GET_USER_PUBKEYS = 0x00D1,
- SSS_SSH_GET_HOST_PUBKEYS = 0x00D2,
/* PAM related calls */ SSS_PAM_AUTHENTICATE = 0x00F1, /**< see pam_sm_authenticate(3) for * details. @@ -470,6 +475,11 @@ int sss_sudo_make_request(enum sss_cli_command cmd, uint8_t **repbuf, size_t *replen, int *errnop);
+int sss_ssh_make_request(enum sss_cli_command cmd,
struct sss_cli_req_data *rd,uint8_t **repbuf, size_t *replen,int *errnop);#ifndef SAFEALIGN_COPY_UINT32 static inline void safealign_memcpy(void *dest, const void *src, size_t n, size_t *counter) -- 1.7.6.5
From ff0188d592a011a06ff8af6fcf69d693d2c6c81a Mon Sep 17 00:00:00 2001 From: Jan Cholasta jcholast@redhat.com Date: Fri, 3 Feb 2012 22:55:33 +0100 Subject: [PATCH 9/9] SSH: AuthorizedKeysCommand client
Makefile.am | 14 +++++ src/sss_client/ssh/sss_ssh_authorizedkeys.c | 70 +++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 0 deletions(-) create mode 100644 src/sss_client/ssh/sss_ssh_authorizedkeys.c
diff --git a/Makefile.am b/Makefile.am index 2ffd453..9be04cc 100644 --- a/Makefile.am +++ b/Makefile.am @@ -605,6 +605,16 @@ sss_sudo_cli_LDADD = \ libsss_sudo.la endif
+if BUILD_SSH +sss_ssh_authorizedkeys_SOURCES = \
- src/sss_client/common.c \
- src/sss_client/ssh/sss_ssh.c \
- src/sss_client/ssh/sss_ssh_authorizedkeys.c
+sss_ssh_authorizedkeys_CFLAGS = $(AM_CFLAGS) +sss_ssh_authorizedkeys_LDADD = \
- libsss_util.la
+endif
################# # Feature Tests # ################# @@ -860,6 +870,9 @@ noinst_PROGRAMS = pam_test_client if BUILD_SUDO noinst_PROGRAMS += sss_sudo_cli endif +if BUILD_SSH +noinst_PROGRAMS += sss_ssh_authorizedkeys +endif pam_test_client_SOURCES = src/sss_client/pam_test_client.c pam_test_client_LDFLAGS = -lpam -lpam_misc
@@ -1243,6 +1256,7 @@ installsssddirs:: mkdir -p \ $(DESTDIR)$(includedir) \ $(DESTDIR)$(libdir) \
- $(DESTDIR)$(bindir) \ $(DESTDIR)$(sbindir) \ $(DESTDIR)$(mandir) \ $(DESTDIR)$(pluginpath) \
diff --git a/src/sss_client/ssh/sss_ssh_authorizedkeys.c b/src/sss_client/ssh/sss_ssh_authorizedkeys.c new file mode 100644 index 0000000..f067eae --- /dev/null +++ b/src/sss_client/ssh/sss_ssh_authorizedkeys.c @@ -0,0 +1,70 @@ +/*
- Authors:
Jan Cholasta <jcholast@redhat.com>- Copyright (C) 2012 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/.
+*/
+#include <stdio.h> +#include <talloc.h>
+#include "util/util.h" +#include "util/crypto/sss_crypto.h" +#include "sss_client/sss_cli.h" +#include "sss_client/ssh/sss_ssh.h"
+int main(int argc, char *argv[]) +{
- TALLOC_CTX *mem_ctx;
- errno_t ret;
- struct sss_ssh_pubkey *pubkeys;
- size_t num_pubkeys, i;
- char *repr;
- if (argc < 2) {
fprintf(stderr, "Usage: %s <username>\n", argv[0]);return 1;- }
- mem_ctx = talloc_new(NULL);
- if (!mem_ctx) {
fprintf(stderr, "Not enough memory\n");return 1;- }
- ret = sss_ssh_get_pubkeys(mem_ctx, SSS_SSH_GET_USER_PUBKEYS, argv[1],
&pubkeys, &num_pubkeys);- if (ret != EOK) {
fprintf(stderr, "Public key lookup failed (error %d: %s)\n",ret, strerror(ret));talloc_free(mem_ctx);return 2;- }
- for (i = 0; i < num_pubkeys; i++) {
ret = sss_ssh_format_pubkey(mem_ctx, &pubkeys[i],SSS_SSH_FORMAT_OPENSSH, &repr);if (ret != EOK) {fprintf(stderr, "Error formating public key\n");continue;}printf("%s\n", repr);- }
- talloc_free(mem_ctx);
- return 0;
+}
1.7.6.5
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Dne 4.2.2012 11:05, Jakub Hrozek napsal(a):
On Fri, Feb 03, 2012 at 11:29:52PM +0100, Jan Cholasta wrote:
Hi,
this is a set of patches implementing SSH support in SSSD.
To test it, install a SSH-patched (patches are on freeipa-devel) IPA server, create a test user with SSH public keys ("ipa user-add someuser --sshpubkey=<blob>"), and then use the sss_ssh_authorizedkeys command to get the user's SSH public keys ("sss_ssh_authorizedkeys someuser" should output user's public keys in OpenSSH's authorized_keys format).
Is there a way to test these using an actual SSH client yet?
Set AuthorizedKeysCommand option in your sshd_config to "<path>/sss_ssh_authorizedkeys".
This is not a complete set of patches for SSH support, some work is still missing and will be posted soon (SSH host keys support in IPA provider, known_hosts client and documentation).
Also please remember to add the new options to the ConfigAPI.
OK, will do.
[PATCH 1/9] UTIL: Provide base64 encoding and decoding functions
Can you add a DEBUG() message saying that sss_base64_{encode,decode} are not implemented with libcrypto back end?
Sure.
I think the NSS back end sss_base64_{encode,decode} should call nspr_nss_init() otherwise they are just relying on the NSS crypto library being initialized.
OK.
[PATCH 2/9] BUILD: Introduce a --with-ssh config option
ACK to the code, but this patch will likely need a rebase on top of the similar sudo/autofs patches.
Yes, I expect that.
[PATCH 3/9] LDAP: Add support for SSH user public keys
This patch should be rebased on top of Stephen's cache timeout patch that is already acked on the list.
OK.
I don'really like the "continue" special case in sdap_save_user(). Why not simply move SDAP_AT_USER_SSH_PUBLIC_KEY before SDAP_FIRST_EXTRA_USER_AT?
I guess I didn't want to add new attributes in the middle of the user map. I can easily change that.
[PATCH 4/9] DP: Add auxiliary info handler This is used to handle non-NSS accounts, such as hosts.
[PATCH 5/9] IPA: Add auxiliary info handler
I'm not a big fan of the auxilary handler, to be honest. While it saves a lot of code duplication, how do you define what falls into the auxiliary category and what needs its own provider type?
I can see you concern. Would it be OK to make a handler just for host info then?
The rest is TBR, I'll continue later.
Thanks.
[PATCH 6/9] DP: Add support for hosts in sss_dp_get_account Host requests are directed to the auxiliary info handler.
[PATCH 7/9] SSH: Responder
[PATCH 8/9] SSH: Common client code
[PATCH 9/9] SSH: AuthorizedKeysCommand client
Honza
It seems that the patches depend on Jan's SELinux patches or at least the IPA host patch. I think you should just rebase your patches on top of his, even though we usually say that patches should apply on master.
On Sat, Feb 04, 2012 at 11:43:56AM +0100, Jan Cholasta wrote:
Dne 4.2.2012 11:05, Jakub Hrozek napsal(a):
On Fri, Feb 03, 2012 at 11:29:52PM +0100, Jan Cholasta wrote:
Hi,
this is a set of patches implementing SSH support in SSSD.
To test it, install a SSH-patched (patches are on freeipa-devel) IPA server, create a test user with SSH public keys ("ipa user-add someuser --sshpubkey=<blob>"), and then use the sss_ssh_authorizedkeys command to get the user's SSH public keys ("sss_ssh_authorizedkeys someuser" should output user's public keys in OpenSSH's authorized_keys format).
Is there a way to test these using an actual SSH client yet?
Set AuthorizedKeysCommand option in your sshd_config to "<path>/sss_ssh_authorizedkeys".
This is not a complete set of patches for SSH support, some work is still missing and will be posted soon (SSH host keys support in IPA provider, known_hosts client and documentation).
Also please remember to add the new options to the ConfigAPI.
OK, will do.
[PATCH 1/9] UTIL: Provide base64 encoding and decoding functions
Can you add a DEBUG() message saying that sss_base64_{encode,decode} are not implemented with libcrypto back end?
Sure.
I think the NSS back end sss_base64_{encode,decode} should call nspr_nss_init() otherwise they are just relying on the NSS crypto library being initialized.
OK.
[PATCH 2/9] BUILD: Introduce a --with-ssh config option
ACK to the code, but this patch will likely need a rebase on top of the similar sudo/autofs patches.
Yes, I expect that.
Please note that there was a bug in the sudo patch this --with-ssh patch is based on. You need to either remove the "[]" that is the "action-not-found" parameter or put a comma after it, otherwise configure would error out on RHEL5.
[PATCH 3/9] LDAP: Add support for SSH user public keys
This patch should be rebased on top of Stephen's cache timeout patch that is already acked on the list.
OK.
I don'really like the "continue" special case in sdap_save_user(). Why not simply move SDAP_AT_USER_SSH_PUBLIC_KEY before SDAP_FIRST_EXTRA_USER_AT?
I guess I didn't want to add new attributes in the middle of the user map. I can easily change that.
[PATCH 4/9] DP: Add auxiliary info handler This is used to handle non-NSS accounts, such as hosts.
[PATCH 5/9] IPA: Add auxiliary info handler
I'm not a big fan of the auxilary handler, to be honest. While it saves a lot of code duplication, how do you define what falls into the auxiliary category and what needs its own provider type?
I can see you concern. Would it be OK to make a handler just for host info then?
This is more of an architectural decision, so I'd like to hear another opinions here.
I personally think that adding a special handler just for host info is better mostly because it would allow to have a different provider for hosts and for identities for example. Cluttering the configuration is not a big problem because in most cases, the additional data providers (except for auth and chpass) can just inherit the id_provider value.
I see the point of code duplication, too. Could it be solved with code refactoring to add some more generic functions?
The rest is TBR, I'll continue later.
Thanks.
[PATCH 6/9] DP: Add support for hosts in sss_dp_get_account Host requests are directed to the auxiliary info handler.
This patch is fine provided that we decide to go forward with the auxiliary info handler.
[PATCH 7/9] SSH: Responder
The provider does not work offline. There needs to be a fallback case that returns cached data in case the provider can't contact DP. If you think we absolutely need to contact DP every login, you can add some "cache_authorized_keys" option that would default to false.
Do you consider adding any form of cache, either negative or positive? I see the concern of having the keys always accurate, but for fast successive logins, maybe at least short lived in-memory cache would provide better performance.
The while(dom) loops in both of the _search() functions are not needed for this type of responder that always goes to DP. If there are no plans for a cache, then you can simply call sss_dp_get_account_send() in the _search() function. Then in the callback, either advance to the next domain if there are no results (with the optional cache fallback) or process the results. This is just a refactoring though, it can be done later during 1.8
[PATCH 8/9] SSH: Common client code
sss_ssh_get_pubkeys(): Please allocate tmp_ctx on NULL and only steal the return values to mem_ctx.
Otherwise looks good to me.
[PATCH 9/9] SSH: AuthorizedKeysCommand client
It is not clear if this is the commands that SSH is supposed to be using or just a test binary -- it is in noinst_PROGRAMs in Makefile.am and not packaged in the RPMs.
In case it is actually supposed to be used by SSH: Are the return values arbitratily picked or does SSH expect any of them?
Can any of the error messages be returned to the user or does SSH mostly use the binary as a "libexec" ? If the messages can be returned to the user, they should be marked as translatable (see the "ERROR()" macro).
It seems that the patches depend on Jan's SELinux patches or at least the IPA host patch. I think you should just rebase your patches on top of his, even though we usually say that patches should apply on master.
On Sat, Feb 04, 2012 at 11:43:56AM +0100, Jan Cholasta wrote:
Dne 4.2.2012 11:05, Jakub Hrozek napsal(a):
On Fri, Feb 03, 2012 at 11:29:52PM +0100, Jan Cholasta wrote:
Hi,
this is a set of patches implementing SSH support in SSSD.
To test it, install a SSH-patched (patches are on freeipa-devel) IPA server, create a test user with SSH public keys ("ipa user-add someuser --sshpubkey=<blob>"), and then use the sss_ssh_authorizedkeys command to get the user's SSH public keys ("sss_ssh_authorizedkeys someuser" should output user's public keys in OpenSSH's authorized_keys format).
Is there a way to test these using an actual SSH client yet?
Set AuthorizedKeysCommand option in your sshd_config to "<path>/sss_ssh_authorizedkeys".
This is not a complete set of patches for SSH support, some work is still missing and will be posted soon (SSH host keys support in IPA provider, known_hosts client and documentation).
Also please remember to add the new options to the ConfigAPI.
OK, will do.
[PATCH 1/9] UTIL: Provide base64 encoding and decoding functions
Can you add a DEBUG() message saying that sss_base64_{encode,decode} are not implemented with libcrypto back end?
Sure.
I think the NSS back end sss_base64_{encode,decode} should call nspr_nss_init() otherwise they are just relying on the NSS crypto library being initialized.
OK.
[PATCH 2/9] BUILD: Introduce a --with-ssh config option
ACK to the code, but this patch will likely need a rebase on top of the similar sudo/autofs patches.
Yes, I expect that.
Please note that there was a bug in the sudo patch this --with-ssh patch is based on. You need to either remove the "[]" that is the "action-not-found" parameter or put a comma after it, otherwise configure would error out on RHEL5.
[PATCH 3/9] LDAP: Add support for SSH user public keys
This patch should be rebased on top of Stephen's cache timeout patch that is already acked on the list.
OK.
I don'really like the "continue" special case in sdap_save_user(). Why not simply move SDAP_AT_USER_SSH_PUBLIC_KEY before SDAP_FIRST_EXTRA_USER_AT?
I guess I didn't want to add new attributes in the middle of the user map. I can easily change that.
I agree with Jakub here, please move it :-)
[PATCH 4/9] DP: Add auxiliary info handler This is used to handle non-NSS accounts, such as hosts.
Please change all DEBUG levels from numeric to the new declarations
[PATCH 5/9] IPA: Add auxiliary info handler
I'm not a big fan of the auxilary handler, to be honest. While it saves a lot of code duplication, how do you define what falls into the auxiliary category and what needs its own provider type?
I can see you concern. Would it be OK to make a handler just for host info then?
This is more of an architectural decision, so I'd like to hear another opinions here.
I don't like the "auxiliary" solution either. It seems to be halfway solution between extending ID handler and making something like "host target" only for information about hosts.
I personally think that adding a special handler just for host info is better mostly because it would allow to have a different provider for hosts and for identities for example. Cluttering the configuration is not a big problem because in most cases, the additional data providers (except for auth and chpass) can just inherit the id_provider value.
Is there a plan for fetching some more information about the host? If yes, then I agree that is should have its own target. If not, I like the extension of current ID handler better because of the code duplication reduction.
I see the point of code duplication, too. Could it be solved with code refactoring to add some more generic functions?
The rest is TBR, I'll continue later.
Thanks.
Please change all DEBUG levels from numeric to the new declarations.
[PATCH 6/9] DP: Add support for hosts in sss_dp_get_account Host requests are directed to the auxiliary info handler.
This patch is fine provided that we decide to go forward with the auxiliary info handler.
[PATCH 7/9] SSH: Responder
The provider does not work offline. There needs to be a fallback case that returns cached data in case the provider can't contact DP. If you think we absolutely need to contact DP every login, you can add some "cache_authorized_keys" option that would default to false.
Do you consider adding any form of cache, either negative or positive? I see the concern of having the keys always accurate, but for fast successive logins, maybe at least short lived in-memory cache would provide better performance.
The while(dom) loops in both of the _search() functions are not needed for this type of responder that always goes to DP. If there are no plans for a cache, then you can simply call sss_dp_get_account_send() in the _search() function. Then in the callback, either advance to the next domain if there are no results (with the optional cache fallback) or process the results. This is just a refactoring though, it can be done later during 1.8
I agree with this. I didn't do deep review since a lot of code is probably going to be changed here. I'll wait for the second round :)
I'm not sure why are you creating the response packet outside of the ssh_cmd_build_reply() function. I think it would be better there.
Also, change all DEBUG levels from numeric to the new declarations. There is one left I think.
[PATCH 8/9] SSH: Common client code
sss_ssh_get_pubkeys(): Please allocate tmp_ctx on NULL and only steal the return values to mem_ctx.
I agree. In fact, I would prefer not to use talloc at all since the client will be fully synchronous. This argument will be especially valid if you want this common code to be reused in other client programs.
The other thing is about keys getting decoded from base64 in the responder and then encoded again in the client, but I guess that's a design decision and I'll respect that even though I find it redundant.
Otherwise looks good to me.
[PATCH 9/9] SSH: AuthorizedKeysCommand client
It is not clear if this is the commands that SSH is supposed to be using or just a test binary -- it is in noinst_PROGRAMs in Makefile.am and not packaged in the RPMs.
In case it is actually supposed to be used by SSH: Are the return values arbitratily picked or does SSH expect any of them?
Can any of the error messages be returned to the user or does SSH mostly use the binary as a "libexec" ? If the messages can be returned to the user, they should be marked as translatable (see the "ERROR()" macro).
Jan
Updated & rebased the patches on top of current master.
To test them, install a SSH-patched (see freeipa-devel) FreeIPA server, set host public keys using "ipa host-mod" (actually this should be done automatically when the IPA server/client is installed) and set user public keys using "ipa user-mod".
Configure sshd (the SSH server) by putting this line in /etc/ssh/sshd_config:
AuthorizedKeysCommand /usr/bin/sss_ssh_authorizedkeys
or, if you test on RHEL, this line:
PubkeyAgent /usr/bin/sss_ssh_authorizedkeys %u
Configure ssh (the SSH client) by putting these lines in .ssh/config:
ProxyCommand /usr/bin/sss_ssh_knownhostsproxy -p %p %h UserKnownHostsFile2 .ssh/sss_known_hosts
Make sure that the server host key is not present in .ssh/known_hosts before testing.
You should then be able to log in to the server using the user public key stored in IPA (ssh should not attempt to use password authentication). To verify that the host keys have been fetched, see whether .ssh/sss_known_hosts was created.
[PATCH 1/10] UTIL: Provide base64 encoding and decoding functions
[PATCH 2/10] BUILD: Introduce a --with-ssh config option
[PATCH 3/10] LDAP: Add support for SSH user public keys
[PATCH 4/10] DP: Add host info handler
[PATCH 5/10] IPA: Add host info handler
[PATCH 6/10] DP: Add support for hosts in sss_dp_get_account
[PATCH 7/10] SSH: Responder
[PATCH 8/10] SSH: Common client code
[PATCH 9/10] SSH: OpenSSH authorized_keys client
[PATCH 10/10] SSH: OpenSSH known_hosts client
Dne 6.2.2012 14:39, Jan Zelený napsal(a):
It seems that the patches depend on Jan's SELinux patches or at least the IPA host patch. I think you should just rebase your patches on top of his, even though we usually say that patches should apply on master.
On Sat, Feb 04, 2012 at 11:43:56AM +0100, Jan Cholasta wrote:
Dne 4.2.2012 11:05, Jakub Hrozek napsal(a):
On Fri, Feb 03, 2012 at 11:29:52PM +0100, Jan Cholasta wrote:
Hi,
this is a set of patches implementing SSH support in SSSD.
To test it, install a SSH-patched (patches are on freeipa-devel) IPA server, create a test user with SSH public keys ("ipa user-add someuser --sshpubkey=<blob>"), and then use the sss_ssh_authorizedkeys command to get the user's SSH public keys ("sss_ssh_authorizedkeys someuser" should output user's public keys in OpenSSH's authorized_keys format).
Is there a way to test these using an actual SSH client yet?
Set AuthorizedKeysCommand option in your sshd_config to "<path>/sss_ssh_authorizedkeys".
This is not a complete set of patches for SSH support, some work is still missing and will be posted soon (SSH host keys support in IPA provider, known_hosts client and documentation).
Also please remember to add the new options to the ConfigAPI.
OK, will do.
[PATCH 1/9] UTIL: Provide base64 encoding and decoding functions
Can you add a DEBUG() message saying that sss_base64_{encode,decode} are not implemented with libcrypto back end?
Sure.
I think the NSS back end sss_base64_{encode,decode} should call nspr_nss_init() otherwise they are just relying on the NSS crypto library being initialized.
OK.
[PATCH 2/9] BUILD: Introduce a --with-ssh config option
ACK to the code, but this patch will likely need a rebase on top of the similar sudo/autofs patches.
Yes, I expect that.
Please note that there was a bug in the sudo patch this --with-ssh patch is based on. You need to either remove the "[]" that is the "action-not-found" parameter or put a comma after it, otherwise configure would error out on RHEL5.
[PATCH 3/9] LDAP: Add support for SSH user public keys
This patch should be rebased on top of Stephen's cache timeout patch that is already acked on the list.
OK.
I don'really like the "continue" special case in sdap_save_user(). Why not simply move SDAP_AT_USER_SSH_PUBLIC_KEY before SDAP_FIRST_EXTRA_USER_AT?
I guess I didn't want to add new attributes in the middle of the user map. I can easily change that.
I agree with Jakub here, please move it :-)
Fixed, though in a different fashion.
[PATCH 4/9] DP: Add auxiliary info handler This is used to handle non-NSS accounts, such as hosts.
Please change all DEBUG levels from numeric to the new declarations
Done.
[PATCH 5/9] IPA: Add auxiliary info handler
I'm not a big fan of the auxilary handler, to be honest. While it saves a lot of code duplication, how do you define what falls into the auxiliary category and what needs its own provider type?
I can see you concern. Would it be OK to make a handler just for host info then?
This is more of an architectural decision, so I'd like to hear another opinions here.
I don't like the "auxiliary" solution either. It seems to be halfway solution between extending ID handler and making something like "host target" only for information about hosts.
I personally think that adding a special handler just for host info is better mostly because it would allow to have a different provider for hosts and for identities for example. Cluttering the configuration is not a big problem because in most cases, the additional data providers (except for auth and chpass) can just inherit the id_provider value.
Is there a plan for fetching some more information about the host? If yes, then I agree that is should have its own target. If not, I like the extension of current ID handler better because of the code duplication reduction.
I have changed the auxiliary info handler to host(-only) info handler.
Also, I have added some sysdb-related code to patch 5.
I see the point of code duplication, too. Could it be solved with code refactoring to add some more generic functions?
The rest is TBR, I'll continue later.
Thanks.
Please change all DEBUG levels from numeric to the new declarations.
Done.
[PATCH 6/9] DP: Add support for hosts in sss_dp_get_account Host requests are directed to the auxiliary info handler.
This patch is fine provided that we decide to go forward with the auxiliary info handler.
Changed the patch to use the host handler.
[PATCH 7/9] SSH: Responder
The provider does not work offline. There needs to be a fallback case that returns cached data in case the provider can't contact DP. If you think we absolutely need to contact DP every login, you can add some "cache_authorized_keys" option that would default to false.
Fixed.
Do you consider adding any form of cache, either negative or positive? I see the concern of having the keys always accurate, but for fast successive logins, maybe at least short lived in-memory cache would provide better performance.
This is planned.
The while(dom) loops in both of the _search() functions are not needed for this type of responder that always goes to DP. If there are no plans for a cache, then you can simply call sss_dp_get_account_send() in the _search() function. Then in the callback, either advance to the next domain if there are no results (with the optional cache fallback) or process the results. This is just a refactoring though, it can be done later during 1.8
I agree with this. I didn't do deep review since a lot of code is probably going to be changed here. I'll wait for the second round :)
Removed the loop.
I'm not sure why are you creating the response packet outside of the ssh_cmd_build_reply() function. I think it would be better there.
Fixed.
Also, change all DEBUG levels from numeric to the new declarations. There is one left I think.
Done.
[PATCH 8/9] SSH: Common client code
sss_ssh_get_pubkeys(): Please allocate tmp_ctx on NULL and only steal the return values to mem_ctx.
Done.
I agree. In fact, I would prefer not to use talloc at all since the client will be fully synchronous. This argument will be especially valid if you want this common code to be reused in other client programs.
I see no harm in using talloc, even if the clients are fully synchronous. Currently there are no other SSH client programs planned.
The other thing is about keys getting decoded from base64 in the responder and then encoded again in the client, but I guess that's a design decision and I'll respect that even though I find it redundant.
It doesn't seem right to me to transport the keys base64-encoded. The code can be easily changed, though.
Otherwise looks good to me.
[PATCH 9/9] SSH: AuthorizedKeysCommand client
It is not clear if this is the commands that SSH is supposed to be using or just a test binary -- it is in noinst_PROGRAMs in Makefile.am and not packaged in the RPMs.
That was just a temporary thing. The client programs are supposed to be run from ssh/sshd. It should be clearer now that man pages are included.
In case it is actually supposed to be used by SSH: Are the return values arbitratily picked or does SSH expect any of them?
Can any of the error messages be returned to the user or does SSH mostly use the binary as a "libexec" ? If the messages can be returned to the user, they should be marked as translatable (see the "ERROR()" macro).
Fixed (anything from stderr is displayed to the user).
Jan
Honza
On Mon, Feb 06, 2012 at 05:42:15PM +0100, Jan Cholasta wrote:
Updated & rebased the patches on top of current master.
To test them, install a SSH-patched (see freeipa-devel) FreeIPA server, set host public keys using "ipa host-mod" (actually this should be done automatically when the IPA server/client is installed) and set user public keys using "ipa user-mod".
Configure sshd (the SSH server) by putting this line in /etc/ssh/sshd_config:
AuthorizedKeysCommand /usr/bin/sss_ssh_authorizedkeysor, if you test on RHEL, this line:
PubkeyAgent /usr/bin/sss_ssh_authorizedkeys %uConfigure ssh (the SSH client) by putting these lines in .ssh/config:
ProxyCommand /usr/bin/sss_ssh_knownhostsproxy -p %p %h UserKnownHostsFile2 .ssh/sss_known_hostsMake sure that the server host key is not present in .ssh/known_hosts before testing.
You should then be able to log in to the server using the user public key stored in IPA (ssh should not attempt to use password authentication). To verify that the host keys have been fetched, see whether .ssh/sss_known_hosts was created.
[PATCH 1/10] UTIL: Provide base64 encoding and decoding functions
Ack
[PATCH 2/10] BUILD: Introduce a --with-ssh config option
Ack
[PATCH 3/10] LDAP: Add support for SSH user public keys
I really don't like special-casing SYSDB_SSH_PUBKEY in sdap_parse_entry(). However, I'm not sure how to handle this in a way that both nice and is not a huge change.
Maybe we could leave this as-is for 1.8beta and file a ticket (a blocker or something that would be red and flashing) to improve later.
There's two options I can think of: 1) the encoding is currently only needed for hosts and users. We could extend sdap_save_users to base64 encode this attribute and then create a wrapper for saving hosts for use in ipa_hostid.c 2) we could make this a little more generic and add a special callback to the attribute map that would allow munging attributes when we receive them from LDAP.
[PATCH 4/10] DP: Add host info handler
Add the hostid_provider to the configAPI tests, otherwise make check won't work.
The BE_REQ_AUTOFS hunk is an important one and should maybe be split into a separate patch (it *definitely* needs to go into 1.8beta, it is a bad merge of autofs on top of sudo).
[PATCH 5/10] IPA: Add host info handler
sysdb_ssh_host_dn() does not appear to be used anywhere and can be removed.
I'll review the rest when I'm finished with my patches currently on the list.
[PATCH 6/10] DP: Add support for hosts in sss_dp_get_account
[PATCH 7/10] SSH: Responder
[PATCH 8/10] SSH: Common client code
[PATCH 9/10] SSH: OpenSSH authorized_keys client
[PATCH 10/10] SSH: OpenSSH known_hosts client
Dne 6.2.2012 14:39, Jan Zelený napsal(a):
It seems that the patches depend on Jan's SELinux patches or at least the IPA host patch. I think you should just rebase your patches on top of his, even though we usually say that patches should apply on master.
On Sat, Feb 04, 2012 at 11:43:56AM +0100, Jan Cholasta wrote:
Dne 4.2.2012 11:05, Jakub Hrozek napsal(a):
On Fri, Feb 03, 2012 at 11:29:52PM +0100, Jan Cholasta wrote:
Hi,
this is a set of patches implementing SSH support in SSSD.
To test it, install a SSH-patched (patches are on freeipa-devel) IPA server, create a test user with SSH public keys ("ipa user-add someuser --sshpubkey=<blob>"), and then use the sss_ssh_authorizedkeys command to get the user's SSH public keys ("sss_ssh_authorizedkeys someuser" should output user's public keys in OpenSSH's authorized_keys format).
Is there a way to test these using an actual SSH client yet?
Set AuthorizedKeysCommand option in your sshd_config to "<path>/sss_ssh_authorizedkeys".
This is not a complete set of patches for SSH support, some work is still missing and will be posted soon (SSH host keys support in IPA provider, known_hosts client and documentation).
Also please remember to add the new options to the ConfigAPI.
OK, will do.
[PATCH 1/9] UTIL: Provide base64 encoding and decoding functions
Can you add a DEBUG() message saying that sss_base64_{encode,decode} are not implemented with libcrypto back end?
Sure.
I think the NSS back end sss_base64_{encode,decode} should call nspr_nss_init() otherwise they are just relying on the NSS crypto library being initialized.
OK.
[PATCH 2/9] BUILD: Introduce a --with-ssh config option
ACK to the code, but this patch will likely need a rebase on top of the similar sudo/autofs patches.
Yes, I expect that.
Please note that there was a bug in the sudo patch this --with-ssh patch is based on. You need to either remove the "[]" that is the "action-not-found" parameter or put a comma after it, otherwise configure would error out on RHEL5.
[PATCH 3/9] LDAP: Add support for SSH user public keys
This patch should be rebased on top of Stephen's cache timeout patch that is already acked on the list.
OK.
I don'really like the "continue" special case in sdap_save_user(). Why not simply move SDAP_AT_USER_SSH_PUBLIC_KEY before SDAP_FIRST_EXTRA_USER_AT?
I guess I didn't want to add new attributes in the middle of the user map. I can easily change that.
I agree with Jakub here, please move it :-)
Fixed, though in a different fashion.
[PATCH 4/9] DP: Add auxiliary info handler This is used to handle non-NSS accounts, such as hosts.
Please change all DEBUG levels from numeric to the new declarations
Done.
[PATCH 5/9] IPA: Add auxiliary info handler
I'm not a big fan of the auxilary handler, to be honest. While it saves a lot of code duplication, how do you define what falls into the auxiliary category and what needs its own provider type?
I can see you concern. Would it be OK to make a handler just for host info then?
This is more of an architectural decision, so I'd like to hear another opinions here.
I don't like the "auxiliary" solution either. It seems to be halfway solution between extending ID handler and making something like "host target" only for information about hosts.
I personally think that adding a special handler just for host info is better mostly because it would allow to have a different provider for hosts and for identities for example. Cluttering the configuration is not a big problem because in most cases, the additional data providers (except for auth and chpass) can just inherit the id_provider value.
Is there a plan for fetching some more information about the host? If yes, then I agree that is should have its own target. If not, I like the extension of current ID handler better because of the code duplication reduction.
I have changed the auxiliary info handler to host(-only) info handler.
Also, I have added some sysdb-related code to patch 5.
I see the point of code duplication, too. Could it be solved with code refactoring to add some more generic functions?
The rest is TBR, I'll continue later.
Thanks.
Please change all DEBUG levels from numeric to the new declarations.
Done.
[PATCH 6/9] DP: Add support for hosts in sss_dp_get_account Host requests are directed to the auxiliary info handler.
This patch is fine provided that we decide to go forward with the auxiliary info handler.
Changed the patch to use the host handler.
[PATCH 7/9] SSH: Responder
The provider does not work offline. There needs to be a fallback case that returns cached data in case the provider can't contact DP. If you think we absolutely need to contact DP every login, you can add some "cache_authorized_keys" option that would default to false.
Fixed.
Do you consider adding any form of cache, either negative or positive? I see the concern of having the keys always accurate, but for fast successive logins, maybe at least short lived in-memory cache would provide better performance.
This is planned.
The while(dom) loops in both of the _search() functions are not needed for this type of responder that always goes to DP. If there are no plans for a cache, then you can simply call sss_dp_get_account_send() in the _search() function. Then in the callback, either advance to the next domain if there are no results (with the optional cache fallback) or process the results. This is just a refactoring though, it can be done later during 1.8
I agree with this. I didn't do deep review since a lot of code is probably going to be changed here. I'll wait for the second round :)
Removed the loop.
I'm not sure why are you creating the response packet outside of the ssh_cmd_build_reply() function. I think it would be better there.
Fixed.
Also, change all DEBUG levels from numeric to the new declarations. There is one left I think.
Done.
[PATCH 8/9] SSH: Common client code
sss_ssh_get_pubkeys(): Please allocate tmp_ctx on NULL and only steal the return values to mem_ctx.
Done.
I agree. In fact, I would prefer not to use talloc at all since the client will be fully synchronous. This argument will be especially valid if you want this common code to be reused in other client programs.
I see no harm in using talloc, even if the clients are fully synchronous. Currently there are no other SSH client programs planned.
The other thing is about keys getting decoded from base64 in the responder and then encoded again in the client, but I guess that's a design decision and I'll respect that even though I find it redundant.
It doesn't seem right to me to transport the keys base64-encoded. The code can be easily changed, though.
Otherwise looks good to me.
[PATCH 9/9] SSH: AuthorizedKeysCommand client
It is not clear if this is the commands that SSH is supposed to be using or just a test binary -- it is in noinst_PROGRAMs in Makefile.am and not packaged in the RPMs.
That was just a temporary thing. The client programs are supposed to be run from ssh/sshd. It should be clearer now that man pages are included.
In case it is actually supposed to be used by SSH: Are the return values arbitratily picked or does SSH expect any of them?
Can any of the error messages be returned to the user or does SSH mostly use the binary as a "libexec" ? If the messages can be returned to the user, they should be marked as translatable (see the "ERROR()" macro).
Fixed (anything from stderr is displayed to the user).
Jan
Honza
-- Jan Cholasta
On Mon, Feb 06, 2012 at 07:48:10PM +0100, Jakub Hrozek wrote:
On Mon, Feb 06, 2012 at 05:42:15PM +0100, Jan Cholasta wrote:
Updated & rebased the patches on top of current master.
To test them, install a SSH-patched (see freeipa-devel) FreeIPA server, set host public keys using "ipa host-mod" (actually this should be done automatically when the IPA server/client is installed) and set user public keys using "ipa user-mod".
Configure sshd (the SSH server) by putting this line in /etc/ssh/sshd_config:
AuthorizedKeysCommand /usr/bin/sss_ssh_authorizedkeysor, if you test on RHEL, this line:
PubkeyAgent /usr/bin/sss_ssh_authorizedkeys %uConfigure ssh (the SSH client) by putting these lines in .ssh/config:
ProxyCommand /usr/bin/sss_ssh_knownhostsproxy -p %p %h UserKnownHostsFile2 .ssh/sss_known_hostsMake sure that the server host key is not present in .ssh/known_hosts before testing.
You should then be able to log in to the server using the user public key stored in IPA (ssh should not attempt to use password authentication). To verify that the host keys have been fetched, see whether .ssh/sss_known_hosts was created.
[PATCH 1/10] UTIL: Provide base64 encoding and decoding functions
Ack
[PATCH 2/10] BUILD: Introduce a --with-ssh config option
Ack
[PATCH 3/10] LDAP: Add support for SSH user public keys
I really don't like special-casing SYSDB_SSH_PUBKEY in sdap_parse_entry(). However, I'm not sure how to handle this in a way that both nice and is not a huge change.
Maybe we could leave this as-is for 1.8beta and file a ticket (a blocker or something that would be red and flashing) to improve later.
There's two options I can think of:
- the encoding is currently only needed for hosts and users. We could
extend sdap_save_users to base64 encode this attribute and then create a wrapper for saving hosts for use in ipa_hostid.c 2) we could make this a little more generic and add a special callback to the attribute map that would allow munging attributes when we receive them from LDAP.
[PATCH 4/10] DP: Add host info handler
Add the hostid_provider to the configAPI tests, otherwise make check won't work.
The BE_REQ_AUTOFS hunk is an important one and should maybe be split into a separate patch (it *definitely* needs to go into 1.8beta, it is a bad merge of autofs on top of sudo).
[PATCH 5/10] IPA: Add host info handler
sysdb_ssh_host_dn() does not appear to be used anywhere and can be removed.
I'll review the rest when I'm finished with my patches currently on the list.
ipa_host_info_handler(): please put ret = EOK above the "done:" label. The code is OK now but if we added some check that would return != EOK we might be in trouble.
hosts_get_done(): the delete and save should be done in a single transaction. Check out services or autofs code for an example of code that tracks if it's inside transaction and commits or cancels if appropriate.
ipa_host_info_recv() can return multiple hosts (and only a single one is deleted in the code afterwards), but you are saving multiple hosts in a loop. If only one host is expected, put a check for count == 1, otherwise error out and save only hosts[0].
ipa_hostid is included only conditionally in Makefile.am, but ipa_init.c references ipa_host_info_handler unconditionally. This would cause a runtime error if IPA provider was built --without-ssh (Been there, done that with the sudo provider..) I ended up putting all the conditional code into the file that is compiled conditionally, kept as little entry points from the generic part of code (such as sssm_ipa_hostid_init()).
[PATCH 6/10] DP: Add support for hosts in sss_dp_get_account
This is good enough for the beta, but I think we shouldn't misuse dp_get_account_send() for data that is not name-service-switch. I think that creating a dp_get_host_send() would be cleaner (even if duplicating some code).
Please file a ticket on this, we can just close it should we decide to keep using dp_get_account_send().
[PATCH 7/10] SSH: Responder
ssh_cmd_parse_request(): You need to check that the names that are coming in from the client are valid UTF-8 (I just noticed that sudo does not do that either and filed a ticket).
Have you tested what name do you receive when the domain is case insensitive? This should be also tested post-beta.
[PATCH 8/10] SSH: Common client code
Ack
[PATCH 9/10] SSH: OpenSSH authorized_keys client
You call getpwuid() but the DEBUG message says getpwnam.
Otherwise ack.
[PATCH 10/10] SSH: OpenSSH known_hosts client
Please file a ticket to handle concurrent writes of the sss_known_hosts file.
If an operation fails in run_connect(), don't forget to close the socket.
Otherwise ack.
After this is pushed, check if everything works with SELinux in enforcing mode and file a bug against selinux-policy if it does not.
Dne 6.2.2012 14:39, Jan Zelený napsal(a):
It seems that the patches depend on Jan's SELinux patches or at least the IPA host patch. I think you should just rebase your patches on top of his, even though we usually say that patches should apply on master.
On Sat, Feb 04, 2012 at 11:43:56AM +0100, Jan Cholasta wrote:
Dne 4.2.2012 11:05, Jakub Hrozek napsal(a):
On Fri, Feb 03, 2012 at 11:29:52PM +0100, Jan Cholasta wrote: >Hi, > >this is a set of patches implementing SSH support in SSSD. > >To test it, install a SSH-patched (patches are on freeipa-devel) IPA >server, create a test user with SSH public keys ("ipa user-add >someuser --sshpubkey=<blob>"), and then use the >sss_ssh_authorizedkeys command to get the user's SSH public keys >("sss_ssh_authorizedkeys someuser" should output user's public keys >in OpenSSH's authorized_keys format).
Is there a way to test these using an actual SSH client yet?
Set AuthorizedKeysCommand option in your sshd_config to "<path>/sss_ssh_authorizedkeys".
>This is not a complete set of patches for SSH support, some work is >still missing and will be posted soon (SSH host keys support in IPA >provider, known_hosts client and documentation).
Also please remember to add the new options to the ConfigAPI.
OK, will do.
>[PATCH 1/9] UTIL: Provide base64 encoding and decoding functions
Can you add a DEBUG() message saying that sss_base64_{encode,decode} are not implemented with libcrypto back end?
Sure.
I think the NSS back end sss_base64_{encode,decode} should call nspr_nss_init() otherwise they are just relying on the NSS crypto library being initialized.
OK.
>[PATCH 2/9] BUILD: Introduce a --with-ssh config option
ACK to the code, but this patch will likely need a rebase on top of the similar sudo/autofs patches.
Yes, I expect that.
Please note that there was a bug in the sudo patch this --with-ssh patch is based on. You need to either remove the "[]" that is the "action-not-found" parameter or put a comma after it, otherwise configure would error out on RHEL5.
>[PATCH 3/9] LDAP: Add support for SSH user public keys
This patch should be rebased on top of Stephen's cache timeout patch that is already acked on the list.
OK.
I don'really like the "continue" special case in sdap_save_user(). Why not simply move SDAP_AT_USER_SSH_PUBLIC_KEY before SDAP_FIRST_EXTRA_USER_AT?
I guess I didn't want to add new attributes in the middle of the user map. I can easily change that.
I agree with Jakub here, please move it :-)
Fixed, though in a different fashion.
>[PATCH 4/9] DP: Add auxiliary info handler >This is used to handle non-NSS accounts, such as hosts.
Please change all DEBUG levels from numeric to the new declarations
Done.
> >[PATCH 5/9] IPA: Add auxiliary info handler
I'm not a big fan of the auxilary handler, to be honest. While it saves a lot of code duplication, how do you define what falls into the auxiliary category and what needs its own provider type?
I can see you concern. Would it be OK to make a handler just for host info then?
This is more of an architectural decision, so I'd like to hear another opinions here.
I don't like the "auxiliary" solution either. It seems to be halfway solution between extending ID handler and making something like "host target" only for information about hosts.
I personally think that adding a special handler just for host info is better mostly because it would allow to have a different provider for hosts and for identities for example. Cluttering the configuration is not a big problem because in most cases, the additional data providers (except for auth and chpass) can just inherit the id_provider value.
Is there a plan for fetching some more information about the host? If yes, then I agree that is should have its own target. If not, I like the extension of current ID handler better because of the code duplication reduction.
I have changed the auxiliary info handler to host(-only) info handler.
Also, I have added some sysdb-related code to patch 5.
I see the point of code duplication, too. Could it be solved with code refactoring to add some more generic functions?
The rest is TBR, I'll continue later.
Thanks.
Please change all DEBUG levels from numeric to the new declarations.
Done.
>[PATCH 6/9] DP: Add support for hosts in sss_dp_get_account >Host requests are directed to the auxiliary info handler.
This patch is fine provided that we decide to go forward with the auxiliary info handler.
Changed the patch to use the host handler.
>[PATCH 7/9] SSH: Responder
The provider does not work offline. There needs to be a fallback case that returns cached data in case the provider can't contact DP. If you think we absolutely need to contact DP every login, you can add some "cache_authorized_keys" option that would default to false.
Fixed.
Do you consider adding any form of cache, either negative or positive? I see the concern of having the keys always accurate, but for fast successive logins, maybe at least short lived in-memory cache would provide better performance.
This is planned.
The while(dom) loops in both of the _search() functions are not needed for this type of responder that always goes to DP. If there are no plans for a cache, then you can simply call sss_dp_get_account_send() in the _search() function. Then in the callback, either advance to the next domain if there are no results (with the optional cache fallback) or process the results. This is just a refactoring though, it can be done later during 1.8
I agree with this. I didn't do deep review since a lot of code is probably going to be changed here. I'll wait for the second round :)
Removed the loop.
I'm not sure why are you creating the response packet outside of the ssh_cmd_build_reply() function. I think it would be better there.
Fixed.
Also, change all DEBUG levels from numeric to the new declarations. There is one left I think.
Done.
>[PATCH 8/9] SSH: Common client code
sss_ssh_get_pubkeys(): Please allocate tmp_ctx on NULL and only steal the return values to mem_ctx.
Done.
I agree. In fact, I would prefer not to use talloc at all since the client will be fully synchronous. This argument will be especially valid if you want this common code to be reused in other client programs.
I see no harm in using talloc, even if the clients are fully synchronous. Currently there are no other SSH client programs planned.
The other thing is about keys getting decoded from base64 in the responder and then encoded again in the client, but I guess that's a design decision and I'll respect that even though I find it redundant.
It doesn't seem right to me to transport the keys base64-encoded. The code can be easily changed, though.
Otherwise looks good to me.
>[PATCH 9/9] SSH: AuthorizedKeysCommand client
It is not clear if this is the commands that SSH is supposed to be using or just a test binary -- it is in noinst_PROGRAMs in Makefile.am and not packaged in the RPMs.
That was just a temporary thing. The client programs are supposed to be run from ssh/sshd. It should be clearer now that man pages are included.
In case it is actually supposed to be used by SSH: Are the return values arbitratily picked or does SSH expect any of them?
Can any of the error messages be returned to the user or does SSH mostly use the binary as a "libexec" ? If the messages can be returned to the user, they should be marked as translatable (see the "ERROR()" macro).
Fixed (anything from stderr is displayed to the user).
Jan
Honza
-- Jan Cholasta
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Updated patches attached.
Dne 6.2.2012 23:13, Jakub Hrozek napsal(a):
On Mon, Feb 06, 2012 at 07:48:10PM +0100, Jakub Hrozek wrote:
On Mon, Feb 06, 2012 at 05:42:15PM +0100, Jan Cholasta wrote:
Updated& rebased the patches on top of current master.
To test them, install a SSH-patched (see freeipa-devel) FreeIPA server, set host public keys using "ipa host-mod" (actually this should be done automatically when the IPA server/client is installed) and set user public keys using "ipa user-mod".
Configure sshd (the SSH server) by putting this line in /etc/ssh/sshd_config:
AuthorizedKeysCommand /usr/bin/sss_ssh_authorizedkeysor, if you test on RHEL, this line:
PubkeyAgent /usr/bin/sss_ssh_authorizedkeys %uConfigure ssh (the SSH client) by putting these lines in .ssh/config:
ProxyCommand /usr/bin/sss_ssh_knownhostsproxy -p %p %h UserKnownHostsFile2 .ssh/sss_known_hostsMake sure that the server host key is not present in .ssh/known_hosts before testing.
You should then be able to log in to the server using the user public key stored in IPA (ssh should not attempt to use password authentication). To verify that the host keys have been fetched, see whether .ssh/sss_known_hosts was created.
[PATCH 1/10] UTIL: Provide base64 encoding and decoding functions
Ack
[PATCH 2/10] BUILD: Introduce a --with-ssh config option
Ack
[PATCH 3/10] LDAP: Add support for SSH user public keys
I really don't like special-casing SYSDB_SSH_PUBKEY in sdap_parse_entry(). However, I'm not sure how to handle this in a way that both nice and is not a huge change.
Maybe we could leave this as-is for 1.8beta and file a ticket (a blocker or something that would be red and flashing) to improve later.
There's two options I can think of:
- the encoding is currently only needed for hosts and users. We could
extend sdap_save_users to base64 encode this attribute and then create a wrapper for saving hosts for use in ipa_hostid.c 2) we could make this a little more generic and add a special callback to the attribute map that would allow munging attributes when we receive them from LDAP.
[PATCH 4/10] DP: Add host info handler
Add the hostid_provider to the configAPI tests, otherwise make check won't work.
The BE_REQ_AUTOFS hunk is an important one and should maybe be split into a separate patch (it *definitely* needs to go into 1.8beta, it is a bad merge of autofs on top of sudo).
[PATCH 5/10] IPA: Add host info handler
sysdb_ssh_host_dn() does not appear to be used anywhere and can be removed.
I'll review the rest when I'm finished with my patches currently on the list.
ipa_host_info_handler(): please put ret = EOK above the "done:" label. The code is OK now but if we added some check that would return != EOK we might be in trouble.
hosts_get_done(): the delete and save should be done in a single transaction. Check out services or autofs code for an example of code that tracks if it's inside transaction and commits or cancels if appropriate.
ipa_host_info_recv() can return multiple hosts (and only a single one is deleted in the code afterwards), but you are saving multiple hosts in a loop. If only one host is expected, put a check for count == 1, otherwise error out and save only hosts[0].
ipa_hostid is included only conditionally in Makefile.am, but ipa_init.c references ipa_host_info_handler unconditionally. This would cause a runtime error if IPA provider was built --without-ssh (Been there, done that with the sudo provider..) I ended up putting all the conditional code into the file that is compiled conditionally, kept as little entry points from the generic part of code (such as sssm_ipa_hostid_init()).
[PATCH 6/10] DP: Add support for hosts in sss_dp_get_account
This is good enough for the beta, but I think we shouldn't misuse dp_get_account_send() for data that is not name-service-switch. I think that creating a dp_get_host_send() would be cleaner (even if duplicating some code).
Please file a ticket on this, we can just close it should we decide to keep using dp_get_account_send().
[PATCH 7/10] SSH: Responder
ssh_cmd_parse_request(): You need to check that the names that are coming in from the client are valid UTF-8 (I just noticed that sudo does not do that either and filed a ticket).
Have you tested what name do you receive when the domain is case insensitive? This should be also tested post-beta.
[PATCH 8/10] SSH: Common client code
Ack
[PATCH 9/10] SSH: OpenSSH authorized_keys client
You call getpwuid() but the DEBUG message says getpwnam.
Otherwise ack.
[PATCH 10/10] SSH: OpenSSH known_hosts client
Please file a ticket to handle concurrent writes of the sss_known_hosts file.
If an operation fails in run_connect(), don't forget to close the socket.
Otherwise ack.
After this is pushed, check if everything works with SELinux in enforcing mode and file a bug against selinux-policy if it does not.
Dne 6.2.2012 14:39, Jan Zelený napsal(a):
It seems that the patches depend on Jan's SELinux patches or at least the IPA host patch. I think you should just rebase your patches on top of his, even though we usually say that patches should apply on master.
On Sat, Feb 04, 2012 at 11:43:56AM +0100, Jan Cholasta wrote:
Dne 4.2.2012 11:05, Jakub Hrozek napsal(a): > On Fri, Feb 03, 2012 at 11:29:52PM +0100, Jan Cholasta wrote: >> Hi, >> >> this is a set of patches implementing SSH support in SSSD. >> >> To test it, install a SSH-patched (patches are on freeipa-devel) IPA >> server, create a test user with SSH public keys ("ipa user-add >> someuser --sshpubkey=<blob>"), and then use the >> sss_ssh_authorizedkeys command to get the user's SSH public keys >> ("sss_ssh_authorizedkeys someuser" should output user's public keys >> in OpenSSH's authorized_keys format). > > Is there a way to test these using an actual SSH client yet?
Set AuthorizedKeysCommand option in your sshd_config to "<path>/sss_ssh_authorizedkeys".
>> This is not a complete set of patches for SSH support, some work is >> still missing and will be posted soon (SSH host keys support in IPA >> provider, known_hosts client and documentation). > > Also please remember to add the new options to the ConfigAPI.
OK, will do.
>> [PATCH 1/9] UTIL: Provide base64 encoding and decoding functions > > Can you add a DEBUG() message saying that sss_base64_{encode,decode} are > not implemented with libcrypto back end?
Sure.
> I think the NSS back end sss_base64_{encode,decode} should call > nspr_nss_init() otherwise they are just relying on the NSS crypto > library being initialized.
OK.
>> [PATCH 2/9] BUILD: Introduce a --with-ssh config option > > ACK to the code, but this patch will likely need a rebase on top of the > similar sudo/autofs patches.
Yes, I expect that.
Please note that there was a bug in the sudo patch this --with-ssh patch is based on. You need to either remove the "[]" that is the "action-not-found" parameter or put a comma after it, otherwise configure would error out on RHEL5.
>> [PATCH 3/9] LDAP: Add support for SSH user public keys > > This patch should be rebased on top of Stephen's cache timeout patch > that is already acked on the list.
OK.
> I don'really like the "continue" special case in sdap_save_user(). Why > not simply move SDAP_AT_USER_SSH_PUBLIC_KEY before > SDAP_FIRST_EXTRA_USER_AT?
I guess I didn't want to add new attributes in the middle of the user map. I can easily change that.
I agree with Jakub here, please move it :-)
Fixed, though in a different fashion.
>> [PATCH 4/9] DP: Add auxiliary info handler >> This is used to handle non-NSS accounts, such as hosts.
Please change all DEBUG levels from numeric to the new declarations
Done.
>> >> [PATCH 5/9] IPA: Add auxiliary info handler > > I'm not a big fan of the auxilary handler, to be honest. While it saves > a lot of code duplication, how do you define what falls into the > auxiliary category and what needs its own provider type?
I can see you concern. Would it be OK to make a handler just for host info then?
This is more of an architectural decision, so I'd like to hear another opinions here.
I don't like the "auxiliary" solution either. It seems to be halfway solution between extending ID handler and making something like "host target" only for information about hosts.
I personally think that adding a special handler just for host info is better mostly because it would allow to have a different provider for hosts and for identities for example. Cluttering the configuration is not a big problem because in most cases, the additional data providers (except for auth and chpass) can just inherit the id_provider value.
Is there a plan for fetching some more information about the host? If yes, then I agree that is should have its own target. If not, I like the extension of current ID handler better because of the code duplication reduction.
I have changed the auxiliary info handler to host(-only) info handler.
Also, I have added some sysdb-related code to patch 5.
I see the point of code duplication, too. Could it be solved with code refactoring to add some more generic functions?
> The rest is TBR, I'll continue later.
Thanks.
Please change all DEBUG levels from numeric to the new declarations.
Done.
>> [PATCH 6/9] DP: Add support for hosts in sss_dp_get_account >> Host requests are directed to the auxiliary info handler.
This patch is fine provided that we decide to go forward with the auxiliary info handler.
Changed the patch to use the host handler.
>> [PATCH 7/9] SSH: Responder
The provider does not work offline. There needs to be a fallback case that returns cached data in case the provider can't contact DP. If you think we absolutely need to contact DP every login, you can add some "cache_authorized_keys" option that would default to false.
Fixed.
Do you consider adding any form of cache, either negative or positive? I see the concern of having the keys always accurate, but for fast successive logins, maybe at least short lived in-memory cache would provide better performance.
This is planned.
The while(dom) loops in both of the _search() functions are not needed for this type of responder that always goes to DP. If there are no plans for a cache, then you can simply call sss_dp_get_account_send() in the _search() function. Then in the callback, either advance to the next domain if there are no results (with the optional cache fallback) or process the results. This is just a refactoring though, it can be done later during 1.8
I agree with this. I didn't do deep review since a lot of code is probably going to be changed here. I'll wait for the second round :)
Removed the loop.
I'm not sure why are you creating the response packet outside of the ssh_cmd_build_reply() function. I think it would be better there.
Fixed.
Also, change all DEBUG levels from numeric to the new declarations. There is one left I think.
Done.
>> [PATCH 8/9] SSH: Common client code
sss_ssh_get_pubkeys(): Please allocate tmp_ctx on NULL and only steal the return values to mem_ctx.
Done.
I agree. In fact, I would prefer not to use talloc at all since the client will be fully synchronous. This argument will be especially valid if you want this common code to be reused in other client programs.
I see no harm in using talloc, even if the clients are fully synchronous. Currently there are no other SSH client programs planned.
The other thing is about keys getting decoded from base64 in the responder and then encoded again in the client, but I guess that's a design decision and I'll respect that even though I find it redundant.
It doesn't seem right to me to transport the keys base64-encoded. The code can be easily changed, though.
Otherwise looks good to me.
>> [PATCH 9/9] SSH: AuthorizedKeysCommand client
It is not clear if this is the commands that SSH is supposed to be using or just a test binary -- it is in noinst_PROGRAMs in Makefile.am and not packaged in the RPMs.
That was just a temporary thing. The client programs are supposed to be run from ssh/sshd. It should be clearer now that man pages are included.
In case it is actually supposed to be used by SSH: Are the return values arbitratily picked or does SSH expect any of them?
Can any of the error messages be returned to the user or does SSH mostly use the binary as a "libexec" ? If the messages can be returned to the user, they should be marked as translatable (see the "ERROR()" macro).
Fixed (anything from stderr is displayed to the user).
Jan
Honza
-- Jan Cholasta
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Jan Cholasta jcholast@redhat.com wrote:
Updated patches attached.
Dne 6.2.2012 23:13, Jakub Hrozek napsal(a):
On Mon, Feb 06, 2012 at 07:48:10PM +0100, Jakub Hrozek wrote:
On Mon, Feb 06, 2012 at 05:42:15PM +0100, Jan Cholasta wrote:
Updated& rebased the patches on top of current master.
To test them, install a SSH-patched (see freeipa-devel) FreeIPA server, set host public keys using "ipa host-mod" (actually this should be done automatically when the IPA server/client is installed) and set user public keys using "ipa user-mod".
Configure sshd (the SSH server) by putting this line in
/etc/ssh/sshd_config: AuthorizedKeysCommand /usr/bin/sss_ssh_authorizedkeys
or, if you test on RHEL, this line: PubkeyAgent /usr/bin/sss_ssh_authorizedkeys %u
Configure ssh (the SSH client) by putting these lines in .ssh/config: ProxyCommand /usr/bin/sss_ssh_knownhostsproxy -p %p %h UserKnownHostsFile2 .ssh/sss_known_hosts
Make sure that the server host key is not present in .ssh/known_hosts before testing.
You should then be able to log in to the server using the user public key stored in IPA (ssh should not attempt to use password authentication). To verify that the host keys have been fetched, see whether .ssh/sss_known_hosts was created.
[PATCH 1/10] UTIL: Provide base64 encoding and decoding functions
Ack
[PATCH 2/10] BUILD: Introduce a --with-ssh config option
Ack
[PATCH 3/10] LDAP: Add support for SSH user public keys
I really don't like special-casing SYSDB_SSH_PUBKEY in sdap_parse_entry(). However, I'm not sure how to handle this in a way that both nice and is not a huge change.
Maybe we could leave this as-is for 1.8beta and file a ticket (a blocker or something that would be red and flashing) to improve later.
There's two options I can think of:
- the encoding is currently only needed for hosts and users. We could
extend sdap_save_users to base64 encode this attribute and then create a wrapper for saving hosts for use in ipa_hostid.c 2) we could make this a little more generic and add a special callback to the attribute map that would allow munging attributes when we receive them from LDAP.
[PATCH 4/10] DP: Add host info handler
Add the hostid_provider to the configAPI tests, otherwise make check won't work.
The BE_REQ_AUTOFS hunk is an important one and should maybe be split into a separate patch (it *definitely* needs to go into 1.8beta, it is a bad merge of autofs on top of sudo).
[PATCH 5/10] IPA: Add host info handler
sysdb_ssh_host_dn() does not appear to be used anywhere and can be removed.
I'll review the rest when I'm finished with my patches currently on the list.
ipa_host_info_handler(): please put ret = EOK above the "done:" label. The code is OK now but if we added some check that would return != EOK we might be in trouble.
hosts_get_done(): the delete and save should be done in a single transaction. Check out services or autofs code for an example of code that tracks if it's inside transaction and commits or cancels if appropriate.
ipa_host_info_recv() can return multiple hosts (and only a single one is deleted in the code afterwards), but you are saving multiple hosts in a loop. If only one host is expected, put a check for count == 1, otherwise error out and save only hosts[0].
ipa_hostid is included only conditionally in Makefile.am, but ipa_init.c references ipa_host_info_handler unconditionally. This would cause a runtime error if IPA provider was built --without-ssh (Been there, done that with the sudo provider..) I ended up putting all the conditional code into the file that is compiled conditionally, kept as little entry points from the generic part of code (such as sssm_ipa_hostid_init()).
[PATCH 6/10] DP: Add support for hosts in sss_dp_get_account
This is good enough for the beta, but I think we shouldn't misuse dp_get_account_send() for data that is not name-service-switch. I think that creating a dp_get_host_send() would be cleaner (even if duplicating some code).
Please file a ticket on this, we can just close it should we decide to keep using dp_get_account_send().
[PATCH 7/10] SSH: Responder
ssh_cmd_parse_request(): You need to check that the names that are coming in from the client are valid UTF-8 (I just noticed that sudo does not do that either and filed a ticket).
Have you tested what name do you receive when the domain is case insensitive? This should be also tested post-beta.
[PATCH 8/10] SSH: Common client code
Ack
[PATCH 9/10] SSH: OpenSSH authorized_keys client
You call getpwuid() but the DEBUG message says getpwnam.
Otherwise ack.
[PATCH 10/10] SSH: OpenSSH known_hosts client
Please file a ticket to handle concurrent writes of the sss_known_hosts file.
If an operation fails in run_connect(), don't forget to close the socket.
Otherwise ack.
After this is pushed, check if everything works with SELinux in enforcing mode and file a bug against selinux-policy if it does not.
Dne 6.2.2012 14:39, Jan Zelený napsal(a):
It seems that the patches depend on Jan's SELinux patches or at least the IPA host patch. I think you should just rebase your patches on top of his, even though we usually say that patches should apply on master.
On Sat, Feb 04, 2012 at 11:43:56AM +0100, Jan Cholasta wrote: > Dne 4.2.2012 11:05, Jakub Hrozek napsal(a): >> On Fri, Feb 03, 2012 at 11:29:52PM +0100, Jan Cholasta wrote: >>> Hi, >>> >>> this is a set of patches implementing SSH support in SSSD. >>> >>> To test it, install a SSH-patched (patches are on freeipa-devel) >>> IPA server, create a test user with SSH public keys ("ipa >>> user-add someuser --sshpubkey=<blob>"), and then use the >>> sss_ssh_authorizedkeys command to get the user's SSH public keys >>> ("sss_ssh_authorizedkeys someuser" should output user's public >>> keys in OpenSSH's authorized_keys format). >> >> Is there a way to test these using an actual SSH client yet? > > Set AuthorizedKeysCommand option in your sshd_config to > "<path>/sss_ssh_authorizedkeys". > >>> This is not a complete set of patches for SSH support, some work >>> is still missing and will be posted soon (SSH host keys support >>> in IPA provider, known_hosts client and documentation). >> >> Also please remember to add the new options to the ConfigAPI. > > OK, will do. > >>> [PATCH 1/9] UTIL: Provide base64 encoding and decoding functions >> >> Can you add a DEBUG() message saying that >> sss_base64_{encode,decode} are not implemented with libcrypto back >> end? > > Sure. > >> I think the NSS back end sss_base64_{encode,decode} should call >> nspr_nss_init() otherwise they are just relying on the NSS crypto >> library being initialized. > > OK. > >>> [PATCH 2/9] BUILD: Introduce a --with-ssh config option >> >> ACK to the code, but this patch will likely need a rebase on top of >> the similar sudo/autofs patches. > > Yes, I expect that.
Please note that there was a bug in the sudo patch this --with-ssh patch is based on. You need to either remove the "[]" that is the "action-not-found" parameter or put a comma after it, otherwise configure would error out on RHEL5.
>>> [PATCH 3/9] LDAP: Add support for SSH user public keys >> >> This patch should be rebased on top of Stephen's cache timeout >> patch that is already acked on the list. > > OK. > >> I don'really like the "continue" special case in sdap_save_user(). >> Why not simply move SDAP_AT_USER_SSH_PUBLIC_KEY before >> SDAP_FIRST_EXTRA_USER_AT? > > I guess I didn't want to add new attributes in the middle of the > user map. I can easily change that.
I agree with Jakub here, please move it :-)
Fixed, though in a different fashion.
>>> [PATCH 4/9] DP: Add auxiliary info handler >>> This is used to handle non-NSS accounts, such as hosts.
Please change all DEBUG levels from numeric to the new declarations
Done.
>>> [PATCH 5/9] IPA: Add auxiliary info handler >> >> I'm not a big fan of the auxilary handler, to be honest. While it >> saves a lot of code duplication, how do you define what falls into >> the auxiliary category and what needs its own provider type? > > I can see you concern. Would it be OK to make a handler just for > host info then?
This is more of an architectural decision, so I'd like to hear another opinions here.
I don't like the "auxiliary" solution either. It seems to be halfway solution between extending ID handler and making something like "host target" only for information about hosts.
I personally think that adding a special handler just for host info is better mostly because it would allow to have a different provider for hosts and for identities for example. Cluttering the configuration is not a big problem because in most cases, the additional data providers (except for auth and chpass) can just inherit the id_provider value.
Is there a plan for fetching some more information about the host? If yes, then I agree that is should have its own target. If not, I like the extension of current ID handler better because of the code duplication reduction.
I have changed the auxiliary info handler to host(-only) info handler.
Also, I have added some sysdb-related code to patch 5.
I see the point of code duplication, too. Could it be solved with code refactoring to add some more generic functions?
>> The rest is TBR, I'll continue later. > > Thanks.
Please change all DEBUG levels from numeric to the new declarations.
Done.
>>> [PATCH 6/9] DP: Add support for hosts in sss_dp_get_account >>> Host requests are directed to the auxiliary info handler.
This patch is fine provided that we decide to go forward with the auxiliary info handler.
Changed the patch to use the host handler.
>>> [PATCH 7/9] SSH: Responder
The provider does not work offline. There needs to be a fallback case that returns cached data in case the provider can't contact DP. If you think we absolutely need to contact DP every login, you can add some "cache_authorized_keys" option that would default to false.
Fixed.
Do you consider adding any form of cache, either negative or positive? I see the concern of having the keys always accurate, but for fast successive logins, maybe at least short lived in-memory cache would provide better performance.
This is planned.
The while(dom) loops in both of the _search() functions are not needed for this type of responder that always goes to DP. If there are no plans for a cache, then you can simply call sss_dp_get_account_send() in the _search() function. Then in the callback, either advance to the next domain if there are no results (with the optional cache fallback) or process the results. This is just a refactoring though, it can be done later during 1.8
I agree with this. I didn't do deep review since a lot of code is probably going to be changed here. I'll wait for the second round :)
Removed the loop.
I'm not sure why are you creating the response packet outside of the ssh_cmd_build_reply() function. I think it would be better there.
Fixed.
Also, change all DEBUG levels from numeric to the new declarations. There is one left I think.
Done.
>>> [PATCH 8/9] SSH: Common client code
sss_ssh_get_pubkeys(): Please allocate tmp_ctx on NULL and only steal the return values to mem_ctx.
Done.
I agree. In fact, I would prefer not to use talloc at all since the client will be fully synchronous. This argument will be especially valid if you want this common code to be reused in other client programs.
I see no harm in using talloc, even if the clients are fully synchronous. Currently there are no other SSH client programs planned.
The other thing is about keys getting decoded from base64 in the responder and then encoded again in the client, but I guess that's a design decision and I'll respect that even though I find it redundant.
It doesn't seem right to me to transport the keys base64-encoded. The code can be easily changed, though.
Otherwise looks good to me.
>>> [PATCH 9/9] SSH: AuthorizedKeysCommand client
It is not clear if this is the commands that SSH is supposed to be using or just a test binary -- it is in noinst_PROGRAMs in Makefile.am and not packaged in the RPMs.
That was just a temporary thing. The client programs are supposed to be run from ssh/sshd. It should be clearer now that man pages are included.
In case it is actually supposed to be used by SSH: Are the return values arbitratily picked or does SSH expect any of them?
Can any of the error messages be returned to the user or does SSH mostly use the binary as a "libexec" ? If the messages can be returned to the user, they should be marked as translatable (see the "ERROR()" macro).
Fixed (anything from stderr is displayed to the user).
Ack for all patches, there are couple things left though. After consulting them with Honza, I decided that they can be fixed later. I'll file a ticket for them first thing tomorrow.
Thanks Jan
A few more issues were found.
Updates patches attached.
Dne 7.2.2012 00:08, Jan Zeleny napsal(a):
Jan Cholastajcholast@redhat.com wrote:
Updated patches attached.
Dne 6.2.2012 23:13, Jakub Hrozek napsal(a):
On Mon, Feb 06, 2012 at 07:48:10PM +0100, Jakub Hrozek wrote:
On Mon, Feb 06, 2012 at 05:42:15PM +0100, Jan Cholasta wrote:
Updated& rebased the patches on top of current master.
To test them, install a SSH-patched (see freeipa-devel) FreeIPA server, set host public keys using "ipa host-mod" (actually this should be done automatically when the IPA server/client is installed) and set user public keys using "ipa user-mod".
Configure sshd (the SSH server) by putting this line in
/etc/ssh/sshd_config: AuthorizedKeysCommand /usr/bin/sss_ssh_authorizedkeys
or, if you test on RHEL, this line: PubkeyAgent /usr/bin/sss_ssh_authorizedkeys %u
Configure ssh (the SSH client) by putting these lines in .ssh/config: ProxyCommand /usr/bin/sss_ssh_knownhostsproxy -p %p %h UserKnownHostsFile2 .ssh/sss_known_hosts
Make sure that the server host key is not present in .ssh/known_hosts before testing.
You should then be able to log in to the server using the user public key stored in IPA (ssh should not attempt to use password authentication). To verify that the host keys have been fetched, see whether .ssh/sss_known_hosts was created.
[PATCH 1/10] UTIL: Provide base64 encoding and decoding functions
Ack
[PATCH 2/10] BUILD: Introduce a --with-ssh config option
Ack
[PATCH 3/10] LDAP: Add support for SSH user public keys
I really don't like special-casing SYSDB_SSH_PUBKEY in sdap_parse_entry(). However, I'm not sure how to handle this in a way that both nice and is not a huge change.
Maybe we could leave this as-is for 1.8beta and file a ticket (a blocker or something that would be red and flashing) to improve later.
There's two options I can think of:
- the encoding is currently only needed for hosts and users. We could
extend sdap_save_users to base64 encode this attribute and then create a wrapper for saving hosts for use in ipa_hostid.c 2) we could make this a little more generic and add a special callback to the attribute map that would allow munging attributes when we receive them from LDAP.
[PATCH 4/10] DP: Add host info handler
Add the hostid_provider to the configAPI tests, otherwise make check won't work.
The BE_REQ_AUTOFS hunk is an important one and should maybe be split into a separate patch (it *definitely* needs to go into 1.8beta, it is a bad merge of autofs on top of sudo).
[PATCH 5/10] IPA: Add host info handler
sysdb_ssh_host_dn() does not appear to be used anywhere and can be removed.
I'll review the rest when I'm finished with my patches currently on the list.
ipa_host_info_handler(): please put ret = EOK above the "done:" label. The code is OK now but if we added some check that would return != EOK we might be in trouble.
hosts_get_done(): the delete and save should be done in a single transaction. Check out services or autofs code for an example of code that tracks if it's inside transaction and commits or cancels if appropriate.
ipa_host_info_recv() can return multiple hosts (and only a single one is deleted in the code afterwards), but you are saving multiple hosts in a loop. If only one host is expected, put a check for count == 1, otherwise error out and save only hosts[0].
ipa_hostid is included only conditionally in Makefile.am, but ipa_init.c references ipa_host_info_handler unconditionally. This would cause a runtime error if IPA provider was built --without-ssh (Been there, done that with the sudo provider..) I ended up putting all the conditional code into the file that is compiled conditionally, kept as little entry points from the generic part of code (such as sssm_ipa_hostid_init()).
[PATCH 6/10] DP: Add support for hosts in sss_dp_get_account
This is good enough for the beta, but I think we shouldn't misuse dp_get_account_send() for data that is not name-service-switch. I think that creating a dp_get_host_send() would be cleaner (even if duplicating some code).
Please file a ticket on this, we can just close it should we decide to keep using dp_get_account_send().
[PATCH 7/10] SSH: Responder
ssh_cmd_parse_request(): You need to check that the names that are coming in from the client are valid UTF-8 (I just noticed that sudo does not do that either and filed a ticket).
Have you tested what name do you receive when the domain is case insensitive? This should be also tested post-beta.
[PATCH 8/10] SSH: Common client code
Ack
[PATCH 9/10] SSH: OpenSSH authorized_keys client
You call getpwuid() but the DEBUG message says getpwnam.
Otherwise ack.
[PATCH 10/10] SSH: OpenSSH known_hosts client
Please file a ticket to handle concurrent writes of the sss_known_hosts file.
If an operation fails in run_connect(), don't forget to close the socket.
Otherwise ack.
After this is pushed, check if everything works with SELinux in enforcing mode and file a bug against selinux-policy if it does not.
Dne 6.2.2012 14:39, Jan Zelený napsal(a):
> It seems that the patches depend on Jan's SELinux patches or at least > the IPA host patch. I think you should just rebase your patches on > top of his, even though we usually say that patches should apply on > master. > > On Sat, Feb 04, 2012 at 11:43:56AM +0100, Jan Cholasta wrote: >> Dne 4.2.2012 11:05, Jakub Hrozek napsal(a): >>> On Fri, Feb 03, 2012 at 11:29:52PM +0100, Jan Cholasta wrote: >>>> Hi, >>>> >>>> this is a set of patches implementing SSH support in SSSD. >>>> >>>> To test it, install a SSH-patched (patches are on freeipa-devel) >>>> IPA server, create a test user with SSH public keys ("ipa >>>> user-add someuser --sshpubkey=<blob>"), and then use the >>>> sss_ssh_authorizedkeys command to get the user's SSH public keys >>>> ("sss_ssh_authorizedkeys someuser" should output user's public >>>> keys in OpenSSH's authorized_keys format). >>> >>> Is there a way to test these using an actual SSH client yet? >> >> Set AuthorizedKeysCommand option in your sshd_config to >> "<path>/sss_ssh_authorizedkeys". >> >>>> This is not a complete set of patches for SSH support, some work >>>> is still missing and will be posted soon (SSH host keys support >>>> in IPA provider, known_hosts client and documentation). >>> >>> Also please remember to add the new options to the ConfigAPI. >> >> OK, will do. >> >>>> [PATCH 1/9] UTIL: Provide base64 encoding and decoding functions >>> >>> Can you add a DEBUG() message saying that >>> sss_base64_{encode,decode} are not implemented with libcrypto back >>> end? >> >> Sure. >> >>> I think the NSS back end sss_base64_{encode,decode} should call >>> nspr_nss_init() otherwise they are just relying on the NSS crypto >>> library being initialized. >> >> OK. >> >>>> [PATCH 2/9] BUILD: Introduce a --with-ssh config option >>> >>> ACK to the code, but this patch will likely need a rebase on top of >>> the similar sudo/autofs patches. >> >> Yes, I expect that. > > Please note that there was a bug in the sudo patch this --with-ssh > patch is based on. You need to either remove the "[]" that is the > "action-not-found" parameter or put a comma after it, otherwise > configure would error out on RHEL5. > >>>> [PATCH 3/9] LDAP: Add support for SSH user public keys >>> >>> This patch should be rebased on top of Stephen's cache timeout >>> patch that is already acked on the list. >> >> OK. >> >>> I don'really like the "continue" special case in sdap_save_user(). >>> Why not simply move SDAP_AT_USER_SSH_PUBLIC_KEY before >>> SDAP_FIRST_EXTRA_USER_AT? >> >> I guess I didn't want to add new attributes in the middle of the >> user map. I can easily change that.
I agree with Jakub here, please move it :-)
Fixed, though in a different fashion.
>>>> [PATCH 4/9] DP: Add auxiliary info handler >>>> This is used to handle non-NSS accounts, such as hosts.
Please change all DEBUG levels from numeric to the new declarations
Done.
>>>> [PATCH 5/9] IPA: Add auxiliary info handler >>> >>> I'm not a big fan of the auxilary handler, to be honest. While it >>> saves a lot of code duplication, how do you define what falls into >>> the auxiliary category and what needs its own provider type? >> >> I can see you concern. Would it be OK to make a handler just for >> host info then? > > This is more of an architectural decision, so I'd like to hear > another opinions here.
I don't like the "auxiliary" solution either. It seems to be halfway solution between extending ID handler and making something like "host target" only for information about hosts.
> I personally think that adding a special handler just for host info > is better mostly because it would allow to have a different provider > for hosts and for identities for example. Cluttering the > configuration is not a big problem because in most cases, the > additional data providers (except for auth and chpass) can just > inherit the id_provider value.
Is there a plan for fetching some more information about the host? If yes, then I agree that is should have its own target. If not, I like the extension of current ID handler better because of the code duplication reduction.
I have changed the auxiliary info handler to host(-only) info handler.
Also, I have added some sysdb-related code to patch 5.
> I see the point of code duplication, too. Could it be solved with > code refactoring to add some more generic functions? > >>> The rest is TBR, I'll continue later. >> >> Thanks.
Please change all DEBUG levels from numeric to the new declarations.
Done.
>>>> [PATCH 6/9] DP: Add support for hosts in sss_dp_get_account >>>> Host requests are directed to the auxiliary info handler. > > This patch is fine provided that we decide to go forward with the > auxiliary info handler.
Changed the patch to use the host handler.
>>>> [PATCH 7/9] SSH: Responder > > The provider does not work offline. There needs to be a fallback case > that returns cached data in case the provider can't contact DP. If > you think we absolutely need to contact DP every login, you can add > some "cache_authorized_keys" option that would default to false.
Fixed.
> Do you consider adding any form of cache, either negative or > positive? I see the concern of having the keys always accurate, but > for fast successive logins, maybe at least short lived in-memory > cache would provide better performance.
This is planned.
> The while(dom) loops in both of the _search() functions are not > needed for this type of responder that always goes to DP. If there > are no plans for a cache, then you can simply call > sss_dp_get_account_send() in the _search() function. Then in the > callback, either advance to the next domain if there are no results > (with the optional cache fallback) or process the results. This is > just a refactoring though, it can be done later during 1.8
I agree with this. I didn't do deep review since a lot of code is probably going to be changed here. I'll wait for the second round :)
Removed the loop.
I'm not sure why are you creating the response packet outside of the ssh_cmd_build_reply() function. I think it would be better there.
Fixed.
Also, change all DEBUG levels from numeric to the new declarations. There is one left I think.
Done.
>>>> [PATCH 8/9] SSH: Common client code > > sss_ssh_get_pubkeys(): Please allocate tmp_ctx on NULL and only steal > the return values to mem_ctx.
Done.
I agree. In fact, I would prefer not to use talloc at all since the client will be fully synchronous. This argument will be especially valid if you want this common code to be reused in other client programs.
I see no harm in using talloc, even if the clients are fully synchronous. Currently there are no other SSH client programs planned.
The other thing is about keys getting decoded from base64 in the responder and then encoded again in the client, but I guess that's a design decision and I'll respect that even though I find it redundant.
It doesn't seem right to me to transport the keys base64-encoded. The code can be easily changed, though.
> Otherwise looks good to me. > >>>> [PATCH 9/9] SSH: AuthorizedKeysCommand client > > It is not clear if this is the commands that SSH is supposed to be > using or just a test binary -- it is in noinst_PROGRAMs in > Makefile.am and not packaged in the RPMs.
That was just a temporary thing. The client programs are supposed to be run from ssh/sshd. It should be clearer now that man pages are included.
> In case it is actually supposed to be used by SSH: > Are the return values arbitratily picked or does SSH expect any of > them? > > Can any of the error messages be returned to the user or does SSH > mostly use the binary as a "libexec" ? If the messages can be > returned to the user, they should be marked as translatable (see the > "ERROR()" macro).
Fixed (anything from stderr is displayed to the user).
Ack for all patches, there are couple things left though. After consulting them with Honza, I decided that they can be fixed later. I'll file a ticket for them first thing tomorrow.
Thanks Jan
On Tue, Feb 07, 2012 at 12:10:50AM +0100, Jan Cholasta wrote:
A few more issues were found.
Updates patches attached.
Dne 7.2.2012 00:08, Jan Zeleny napsal(a):
Jan Cholastajcholast@redhat.com wrote:
Updated patches attached.
Dne 6.2.2012 23:13, Jakub Hrozek napsal(a):
On Mon, Feb 06, 2012 at 07:48:10PM +0100, Jakub Hrozek wrote:
On Mon, Feb 06, 2012 at 05:42:15PM +0100, Jan Cholasta wrote:
Updated& rebased the patches on top of current master.
To test them, install a SSH-patched (see freeipa-devel) FreeIPA server, set host public keys using "ipa host-mod" (actually this should be done automatically when the IPA server/client is installed) and set user public keys using "ipa user-mod".
Configure sshd (the SSH server) by putting this line in
/etc/ssh/sshd_config: AuthorizedKeysCommand /usr/bin/sss_ssh_authorizedkeys
or, if you test on RHEL, this line: PubkeyAgent /usr/bin/sss_ssh_authorizedkeys %u
Configure ssh (the SSH client) by putting these lines in .ssh/config: ProxyCommand /usr/bin/sss_ssh_knownhostsproxy -p %p %h UserKnownHostsFile2 .ssh/sss_known_hosts
Make sure that the server host key is not present in .ssh/known_hosts before testing.
You should then be able to log in to the server using the user public key stored in IPA (ssh should not attempt to use password authentication). To verify that the host keys have been fetched, see whether .ssh/sss_known_hosts was created.
[PATCH 1/10] UTIL: Provide base64 encoding and decoding functions
Ack
[PATCH 2/10] BUILD: Introduce a --with-ssh config option
Ack
[PATCH 3/10] LDAP: Add support for SSH user public keys
I really don't like special-casing SYSDB_SSH_PUBKEY in sdap_parse_entry(). However, I'm not sure how to handle this in a way that both nice and is not a huge change.
Maybe we could leave this as-is for 1.8beta and file a ticket (a blocker or something that would be red and flashing) to improve later.
There's two options I can think of:
- the encoding is currently only needed for hosts and users. We could
extend sdap_save_users to base64 encode this attribute and then create a wrapper for saving hosts for use in ipa_hostid.c 2) we could make this a little more generic and add a special callback to the attribute map that would allow munging attributes when we receive them from LDAP.
[PATCH 4/10] DP: Add host info handler
Add the hostid_provider to the configAPI tests, otherwise make check won't work.
The BE_REQ_AUTOFS hunk is an important one and should maybe be split into a separate patch (it *definitely* needs to go into 1.8beta, it is a bad merge of autofs on top of sudo).
[PATCH 5/10] IPA: Add host info handler
sysdb_ssh_host_dn() does not appear to be used anywhere and can be removed.
I'll review the rest when I'm finished with my patches currently on the list.
ipa_host_info_handler(): please put ret = EOK above the "done:" label. The code is OK now but if we added some check that would return != EOK we might be in trouble.
hosts_get_done(): the delete and save should be done in a single transaction. Check out services or autofs code for an example of code that tracks if it's inside transaction and commits or cancels if appropriate.
ipa_host_info_recv() can return multiple hosts (and only a single one is deleted in the code afterwards), but you are saving multiple hosts in a loop. If only one host is expected, put a check for count == 1, otherwise error out and save only hosts[0].
ipa_hostid is included only conditionally in Makefile.am, but ipa_init.c references ipa_host_info_handler unconditionally. This would cause a runtime error if IPA provider was built --without-ssh (Been there, done that with the sudo provider..) I ended up putting all the conditional code into the file that is compiled conditionally, kept as little entry points from the generic part of code (such as sssm_ipa_hostid_init()).
[PATCH 6/10] DP: Add support for hosts in sss_dp_get_account
This is good enough for the beta, but I think we shouldn't misuse dp_get_account_send() for data that is not name-service-switch. I think that creating a dp_get_host_send() would be cleaner (even if duplicating some code).
Please file a ticket on this, we can just close it should we decide to keep using dp_get_account_send().
[PATCH 7/10] SSH: Responder
ssh_cmd_parse_request(): You need to check that the names that are coming in from the client are valid UTF-8 (I just noticed that sudo does not do that either and filed a ticket).
Have you tested what name do you receive when the domain is case insensitive? This should be also tested post-beta.
[PATCH 8/10] SSH: Common client code
Ack
[PATCH 9/10] SSH: OpenSSH authorized_keys client
You call getpwuid() but the DEBUG message says getpwnam.
Otherwise ack.
[PATCH 10/10] SSH: OpenSSH known_hosts client
Please file a ticket to handle concurrent writes of the sss_known_hosts file.
If an operation fails in run_connect(), don't forget to close the socket.
Otherwise ack.
After this is pushed, check if everything works with SELinux in enforcing mode and file a bug against selinux-policy if it does not.
Dne 6.2.2012 14:39, Jan Zelený napsal(a): >>It seems that the patches depend on Jan's SELinux patches or at least >>the IPA host patch. I think you should just rebase your patches on >>top of his, even though we usually say that patches should apply on >>master. >> >>On Sat, Feb 04, 2012 at 11:43:56AM +0100, Jan Cholasta wrote: >>>Dne 4.2.2012 11:05, Jakub Hrozek napsal(a): >>>>On Fri, Feb 03, 2012 at 11:29:52PM +0100, Jan Cholasta wrote: >>>>>Hi, >>>>> >>>>>this is a set of patches implementing SSH support in SSSD. >>>>> >>>>>To test it, install a SSH-patched (patches are on freeipa-devel) >>>>>IPA server, create a test user with SSH public keys ("ipa >>>>>user-add someuser --sshpubkey=<blob>"), and then use the >>>>>sss_ssh_authorizedkeys command to get the user's SSH public keys >>>>>("sss_ssh_authorizedkeys someuser" should output user's public >>>>>keys in OpenSSH's authorized_keys format). >>>> >>>>Is there a way to test these using an actual SSH client yet? >>> >>>Set AuthorizedKeysCommand option in your sshd_config to >>>"<path>/sss_ssh_authorizedkeys". >>> >>>>>This is not a complete set of patches for SSH support, some work >>>>>is still missing and will be posted soon (SSH host keys support >>>>>in IPA provider, known_hosts client and documentation). >>>> >>>>Also please remember to add the new options to the ConfigAPI. >>> >>>OK, will do. >>> >>>>>[PATCH 1/9] UTIL: Provide base64 encoding and decoding functions >>>> >>>>Can you add a DEBUG() message saying that >>>>sss_base64_{encode,decode} are not implemented with libcrypto back >>>>end? >>> >>>Sure. >>> >>>>I think the NSS back end sss_base64_{encode,decode} should call >>>>nspr_nss_init() otherwise they are just relying on the NSS crypto >>>>library being initialized. >>> >>>OK. >>> >>>>>[PATCH 2/9] BUILD: Introduce a --with-ssh config option >>>> >>>>ACK to the code, but this patch will likely need a rebase on top of >>>>the similar sudo/autofs patches. >>> >>>Yes, I expect that. >> >>Please note that there was a bug in the sudo patch this --with-ssh >>patch is based on. You need to either remove the "[]" that is the >>"action-not-found" parameter or put a comma after it, otherwise >>configure would error out on RHEL5. >> >>>>>[PATCH 3/9] LDAP: Add support for SSH user public keys >>>> >>>>This patch should be rebased on top of Stephen's cache timeout >>>>patch that is already acked on the list. >>> >>>OK. >>> >>>>I don'really like the "continue" special case in sdap_save_user(). >>>>Why not simply move SDAP_AT_USER_SSH_PUBLIC_KEY before >>>>SDAP_FIRST_EXTRA_USER_AT? >>> >>>I guess I didn't want to add new attributes in the middle of the >>>user map. I can easily change that. > >I agree with Jakub here, please move it :-)
Fixed, though in a different fashion.
>>>>>[PATCH 4/9] DP: Add auxiliary info handler >>>>>This is used to handle non-NSS accounts, such as hosts. > >Please change all DEBUG levels from numeric to the new declarations
Done.
>>>>>[PATCH 5/9] IPA: Add auxiliary info handler >>>> >>>>I'm not a big fan of the auxilary handler, to be honest. While it >>>>saves a lot of code duplication, how do you define what falls into >>>>the auxiliary category and what needs its own provider type? >>> >>>I can see you concern. Would it be OK to make a handler just for >>>host info then? >> >>This is more of an architectural decision, so I'd like to hear >>another opinions here. > >I don't like the "auxiliary" solution either. It seems to be halfway >solution between extending ID handler and making something like "host >target" only for information about hosts. > >>I personally think that adding a special handler just for host info >>is better mostly because it would allow to have a different provider >>for hosts and for identities for example. Cluttering the >>configuration is not a big problem because in most cases, the >>additional data providers (except for auth and chpass) can just >>inherit the id_provider value. > >Is there a plan for fetching some more information about the host? If >yes, then I agree that is should have its own target. If not, I like >the extension of current ID handler better because of the code >duplication reduction.
I have changed the auxiliary info handler to host(-only) info handler.
Also, I have added some sysdb-related code to patch 5.
>>I see the point of code duplication, too. Could it be solved with >>code refactoring to add some more generic functions? >> >>>>The rest is TBR, I'll continue later. >>> >>>Thanks. > >Please change all DEBUG levels from numeric to the new declarations.
Done.
>>>>>[PATCH 6/9] DP: Add support for hosts in sss_dp_get_account >>>>>Host requests are directed to the auxiliary info handler. >> >>This patch is fine provided that we decide to go forward with the >>auxiliary info handler.
Changed the patch to use the host handler.
>>>>>[PATCH 7/9] SSH: Responder >> >>The provider does not work offline. There needs to be a fallback case >>that returns cached data in case the provider can't contact DP. If >>you think we absolutely need to contact DP every login, you can add >>some "cache_authorized_keys" option that would default to false.
Fixed.
>>Do you consider adding any form of cache, either negative or >>positive? I see the concern of having the keys always accurate, but >>for fast successive logins, maybe at least short lived in-memory >>cache would provide better performance.
This is planned.
>>The while(dom) loops in both of the _search() functions are not >>needed for this type of responder that always goes to DP. If there >>are no plans for a cache, then you can simply call >>sss_dp_get_account_send() in the _search() function. Then in the >>callback, either advance to the next domain if there are no results >>(with the optional cache fallback) or process the results. This is >>just a refactoring though, it can be done later during 1.8 > >I agree with this. I didn't do deep review since a lot of code is >probably going to be changed here. I'll wait for the second round :)
Removed the loop.
>I'm not sure why are you creating the response packet outside of the >ssh_cmd_build_reply() function. I think it would be better there.
Fixed.
>Also, change all DEBUG levels from numeric to the new declarations. >There is one left I think.
Done.
>>>>>[PATCH 8/9] SSH: Common client code >> >>sss_ssh_get_pubkeys(): Please allocate tmp_ctx on NULL and only steal >>the return values to mem_ctx.
Done.
>I agree. In fact, I would prefer not to use talloc at all since the >client will be fully synchronous. This argument will be especially >valid if you want this common code to be reused in other client >programs.
I see no harm in using talloc, even if the clients are fully synchronous. Currently there are no other SSH client programs planned.
>The other thing is about keys getting decoded from base64 in the >responder and then encoded again in the client, but I guess that's a >design decision and I'll respect that even though I find it >redundant.
It doesn't seem right to me to transport the keys base64-encoded. The code can be easily changed, though.
>>Otherwise looks good to me. >> >>>>>[PATCH 9/9] SSH: AuthorizedKeysCommand client >> >>It is not clear if this is the commands that SSH is supposed to be >>using or just a test binary -- it is in noinst_PROGRAMs in >>Makefile.am and not packaged in the RPMs.
That was just a temporary thing. The client programs are supposed to be run from ssh/sshd. It should be clearer now that man pages are included.
>>In case it is actually supposed to be used by SSH: >>Are the return values arbitratily picked or does SSH expect any of >>them? >> >>Can any of the error messages be returned to the user or does SSH >>mostly use the binary as a "libexec" ? If the messages can be >>returned to the user, they should be marked as translatable (see the >>"ERROR()" macro).
Fixed (anything from stderr is displayed to the user).
Ack for all patches, there are couple things left though. After consulting them with Honza, I decided that they can be fixed later. I'll file a ticket for them first thing tomorrow.
Thanks Jan
Pushed to master.
Jan Cholasta jcholast@redhat.com wrote:
Updated & rebased the patches on top of current master.
To test them, install a SSH-patched (see freeipa-devel) FreeIPA server, set host public keys using "ipa host-mod" (actually this should be done automatically when the IPA server/client is installed) and set user public keys using "ipa user-mod".
Configure sshd (the SSH server) by putting this line in /etc/ssh/sshd_config:
AuthorizedKeysCommand /usr/bin/sss_ssh_authorizedkeysor, if you test on RHEL, this line:
PubkeyAgent /usr/bin/sss_ssh_authorizedkeys %uConfigure ssh (the SSH client) by putting these lines in .ssh/config:
ProxyCommand /usr/bin/sss_ssh_knownhostsproxy -p %p %h UserKnownHostsFile2 .ssh/sss_known_hostsMake sure that the server host key is not present in .ssh/known_hosts before testing.
You should then be able to log in to the server using the user public key stored in IPA (ssh should not attempt to use password authentication). To verify that the host keys have been fetched, see whether .ssh/sss_known_hosts was created.
[PATCH 1/10] UTIL: Provide base64 encoding and decoding functions
[PATCH 2/10] BUILD: Introduce a --with-ssh config option
[PATCH 3/10] LDAP: Add support for SSH user public keys
[PATCH 4/10] DP: Add host info handler
[PATCH 5/10] IPA: Add host info handler
[PATCH 6/10] DP: Add support for hosts in sss_dp_get_account
[PATCH 7/10] SSH: Responder
[PATCH 8/10] SSH: Common client code
[PATCH 9/10] SSH: OpenSSH authorized_keys client
[PATCH 10/10] SSH: OpenSSH known_hosts client
Dne 6.2.2012 14:39, Jan Zelený napsal(a):
It seems that the patches depend on Jan's SELinux patches or at least the IPA host patch. I think you should just rebase your patches on top of his, even though we usually say that patches should apply on master.
On Sat, Feb 04, 2012 at 11:43:56AM +0100, Jan Cholasta wrote:
Dne 4.2.2012 11:05, Jakub Hrozek napsal(a):
On Fri, Feb 03, 2012 at 11:29:52PM +0100, Jan Cholasta wrote:
Hi,
this is a set of patches implementing SSH support in SSSD.
To test it, install a SSH-patched (patches are on freeipa-devel) IPA server, create a test user with SSH public keys ("ipa user-add someuser --sshpubkey=<blob>"), and then use the sss_ssh_authorizedkeys command to get the user's SSH public keys ("sss_ssh_authorizedkeys someuser" should output user's public keys in OpenSSH's authorized_keys format).
Is there a way to test these using an actual SSH client yet?
Set AuthorizedKeysCommand option in your sshd_config to "<path>/sss_ssh_authorizedkeys".
This is not a complete set of patches for SSH support, some work is still missing and will be posted soon (SSH host keys support in IPA provider, known_hosts client and documentation).
Also please remember to add the new options to the ConfigAPI.
OK, will do.
[PATCH 1/9] UTIL: Provide base64 encoding and decoding functions
Can you add a DEBUG() message saying that sss_base64_{encode,decode} are not implemented with libcrypto back end?
Sure.
I think the NSS back end sss_base64_{encode,decode} should call nspr_nss_init() otherwise they are just relying on the NSS crypto library being initialized.
OK.
Nothing to add here, Ack
[PATCH 2/9] BUILD: Introduce a --with-ssh config option
ACK to the code, but this patch will likely need a rebase on top of the similar sudo/autofs patches.
Yes, I expect that.
Please note that there was a bug in the sudo patch this --with-ssh patch is based on. You need to either remove the "[]" that is the "action-not-found" parameter or put a comma after it, otherwise configure would error out on RHEL5.
Nothing to add here, Ack
[PATCH 3/9] LDAP: Add support for SSH user public keys
This patch should be rebased on top of Stephen's cache timeout patch that is already acked on the list.
OK.
I don'really like the "continue" special case in sdap_save_user(). Why not simply move SDAP_AT_USER_SSH_PUBLIC_KEY before SDAP_FIRST_EXTRA_USER_AT?
I guess I didn't want to add new attributes in the middle of the user map. I can easily change that.
I agree with Jakub here, please move it :-)
Fixed, though in a different fashion.
Ack I guess. I tried to figure out different solution but in the end this seems to be as good as any.
[PATCH 4/9] DP: Add auxiliary info handler This is used to handle non-NSS accounts, such as hosts.
Please change all DEBUG levels from numeric to the new declarations
Done.
Plase review lines 213-237 in the patch. I believe the service filter splitting is not useful for hosts, or am I mistaken? I'm also curious about the ENUM - do you plan to support enumeration for hosts?
[PATCH 5/9] IPA: Add auxiliary info handler
I'm not a big fan of the auxilary handler, to be honest. While it saves a lot of code duplication, how do you define what falls into the auxiliary category and what needs its own provider type?
I can see you concern. Would it be OK to make a handler just for host info then?
This is more of an architectural decision, so I'd like to hear another opinions here.
I don't like the "auxiliary" solution either. It seems to be halfway solution between extending ID handler and making something like "host target" only for information about hosts.
I personally think that adding a special handler just for host info is better mostly because it would allow to have a different provider for hosts and for identities for example. Cluttering the configuration is not a big problem because in most cases, the additional data providers (except for auth and chpass) can just inherit the id_provider value.
Is there a plan for fetching some more information about the host? If yes, then I agree that is should have its own target. If not, I like the extension of current ID handler better because of the code duplication reduction.
I have changed the auxiliary info handler to host(-only) info handler.
Also, I have added some sysdb-related code to patch 5.
I see the point of code duplication, too. Could it be solved with code refactoring to add some more generic functions?
The rest is TBR, I'll continue later.
Thanks.
Please change all DEBUG levels from numeric to the new declarations.
Done.
Some more comments: Is the the line 182 of the patch corrent? IMO it should contain ret = ENOMEM;
Looking at the condition on line 192 and how you subsequently use the function sysdb_search_ssh_hosts(), I think you should have ret = EOK; in that condition. Even better solution would be to fix it in the patch #0007 (see my comment there)
This is just a nitpick I guess, but I find the ipa_host_info_handler() a bit messy, you are calling sdap_handler_done() on quite a few places. I think just using the "done" label would be better.
Another nitpick: the return on line 590 is redundant
The ENOENT value you are testing for on line 603 will never appear there IMO. Therefore the condition couple lines below is not needed either.
I don't like the change in host fetching - if you are about to use host map (which is certainly the right thing to do), you should not need a list of attributes as well - they should be constructed from the map. If you won't make it till the freeze, please file a high-priority ticket so we can get this fixed soon. Thanks
[PATCH 6/9] DP: Add support for hosts in sss_dp_get_account Host requests are directed to the auxiliary info handler.
This patch is fine provided that we decide to go forward with the auxiliary info handler.
Changed the patch to use the host handler.
I'm not sure if we want this code to be a part of the routine which clearly serves for connection to ID provider, but I don't have any exprience with this part of the code, hence I'll leave this topic open for someone else. I have no problem with it, so Ack.
[PATCH 7/9] SSH: Responder
The provider does not work offline. There needs to be a fallback case that returns cached data in case the provider can't contact DP. If you think we absolutely need to contact DP every login, you can add some "cache_authorized_keys" option that would default to false.
Fixed.
Do you consider adding any form of cache, either negative or positive? I see the concern of having the keys always accurate, but for fast successive logins, maybe at least short lived in-memory cache would provide better performance.
This is planned.
The while(dom) loops in both of the _search() functions are not needed for this type of responder that always goes to DP. If there are no plans for a cache, then you can simply call sss_dp_get_account_send() in the _search() function. Then in the callback, either advance to the next domain if there are no results (with the optional cache fallback) or process the results. This is just a refactoring though, it can be done later during 1.8
I agree with this. I didn't do deep review since a lot of code is probably going to be changed here. I'll wait for the second round :)
Removed the loop.
I'm not sure why are you creating the response packet outside of the ssh_cmd_build_reply() function. I think it would be better there.
Fixed.
Also, change all DEBUG levels from numeric to the new declarations. There is one left I think.
Done.
On line 696 you should also check for ENOENT (the "better" solution from patch #5)
[PATCH 8/9] SSH: Common client code
sss_ssh_get_pubkeys(): Please allocate tmp_ctx on NULL and only steal the return values to mem_ctx.
Done.
I agree. In fact, I would prefer not to use talloc at all since the client will be fully synchronous. This argument will be especially valid if you want this common code to be reused in other client programs.
I see no harm in using talloc, even if the clients are fully synchronous. Currently there are no other SSH client programs planned.
The other thing is about keys getting decoded from base64 in the responder and then encoded again in the client, but I guess that's a design decision and I'll respect that even though I find it redundant.
It doesn't seem right to me to transport the keys base64-encoded. The code can be easily changed, though.
Otherwise looks good to me.
Nothing to add, Ack
[PATCH 9/9] SSH: AuthorizedKeysCommand client
It is not clear if this is the commands that SSH is supposed to be using or just a test binary -- it is in noinst_PROGRAMs in Makefile.am and not packaged in the RPMs.
That was just a temporary thing. The client programs are supposed to be run from ssh/sshd. It should be clearer now that man pages are included.
In case it is actually supposed to be used by SSH: Are the return values arbitratily picked or does SSH expect any of them?
Can any of the error messages be returned to the user or does SSH mostly use the binary as a "libexec" ? If the messages can be returned to the user, they should be marked as translatable (see the "ERROR()" macro).
Fixed (anything from stderr is displayed to the user).
Nothing to add, Ack
As for patch #0010: Please initialize errno somewhere in run_connect()
I'm not sure about EAGAIN, but you probably want to check for EINTR on line 379 of the patch
That's all I've found
Thanks Jan
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jan Cholasta -
Jan Zelený