[PATCHES] SSH support

[Jan Cholasta]

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

-- 
Jan Cholasta

[Jakub Hrozek]

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(a)redhat.com&gt;
>  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.c
>       SSS_CRYPT_CFLAGS = $(NSS_CFLAGS)
>       SSS_CRYPT_LIBS = $(NSS_LIBS)
>   else
>  -    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.c
>       SSS_CRYPT_CFLAGS = $(CRYPTO_CFLAGS)
>       SSS_CRYPT_LIBS = $(CRYPTO_LIBS)
>  diff --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(a)redhat.com&gt;
>  +
>  +    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(a)redhat.com&gt;
>  +
>  +    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(a)redhat.com&gt;
>  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; then
>  +            with_ssh=yes
>  +        fi
>  +    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(a)redhat.com&gt;
>  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(a)redhat.com&gt;
>  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);
>       if (ret != EOK) {
>           err_maj = DP_ERR_FATAL;
>  @@ -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(a)redhat.com&gt;
>  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(a)redhat.com&gt;
>  +
>  +    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(a)redhat.com&gt;
>  +
>  +    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(a)redhat.com&gt;
>  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);
>       if (msg == NULL) {
>           talloc_free(filter);
>           DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory?!\n"));
>  -- 
>  1.7.6.5
>   
>  From 5c9e073df33f3e9eadb3233b050806f8932634db Mon Sep 17 00:00:00
> 2001
>  From: Jan Cholasta <jcholast(a)redhat.com&gt;
>  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;
>       } else {
>           DEBUG(1, ("Unknown client! [%s]\n", cli_name));
>       }
>  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(a)redhat.com&gt;
>  +
>  +    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_AUTOHELP
>  +        SSSD_MAIN_OPTS
>  +        POPT_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(a)redhat.com&gt;
>  +
>  +    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(a)redhat.com&gt;
>  +
>  +    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(a)redhat.com&gt;
>  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;
>       } else {
>           return false;
>       }
>  @@ -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(a)redhat.com&gt;
>  +
>  +    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(a)redhat.com&gt;
>  +
>  +    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(a)redhat.com&gt;
>  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(a)redhat.com&gt;
>  +
>  +    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(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel

[Jan Cholasta]

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
> > 
-- 
Jan Cholasta

[Jakub Hrozek]

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).

[Jan Zelený]

>  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

[Jan Cholasta]

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

-- 
Jan Cholasta

[Jakub Hrozek]

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:
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

[Jakub Hrozek]

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:
>  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.
>   
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(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel

[Jan Cholasta]

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:
> > 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.
> >
>
>  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(a)lists.fedorahosted.org
> > https://fedorahosted.org/mailman/listinfo/sssd-devel 

-- 
Jan Cholasta

[Jan Zelený]

Jan Cholasta <jcholast(a)redhat.com&gt; 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:
>  >> 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.
>  > 
>  > 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

[Jan Cholasta]

A few more issues were found.

Updates patches attached.

Dne 7.2.2012 00:08, Jan Zeleny napsal(a):
>  Jan Cholasta<jcholast(a)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:
> >>> 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.
> >>
> >> 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 

-- 
Jan Cholasta

[Jakub Hrozek]

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 Cholasta<jcholast(a)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:
>  >>>>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.
>  >>>
>  >>>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 Zelený]

Jan Cholasta <jcholast(a)redhat.com&gt; 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
>  
>  [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