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).
>>
>>>
>>>
>>> Jan
>>
>> Honza
>>
>> --
>> Jan Cholasta
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://fedorahosted.org/mailman/listinfo/sssd-devel