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