Jan Cholasta jcholast@redhat.com 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_authorizedkeysor, if you test on RHEL, this line:
PubkeyAgent /usr/bin/sss_ssh_authorizedkeys %uConfigure ssh (the SSH client) by putting these lines in .ssh/config:
ProxyCommand /usr/bin/sss_ssh_knownhostsproxy -p %p %h UserKnownHostsFile2 .ssh/sss_known_hostsMake 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