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
_______________________________________________
sssd-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/sssd-devel
