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