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
--
Jan Cholasta