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<jcholast(a)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
Pushed to master.