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