Re: [SSSD] [PATCHES] SSH support

[Jan Cholasta]

A few more issues were found.

Updates patches attached.

Dne 7.2.2012 00:08, Jan Zeleny napsal(a):

Jan Cholasta<jchol...@redhat.com>  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

--
Jan Cholasta

sssd-ssh-3.tar.bz2
Description: application/bzip

_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel