[PATCHES] SSH support
by Jan Cholasta
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).
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).
[PATCH 1/9] UTIL: Provide base64 encoding and decoding functions
[PATCH 2/9] BUILD: Introduce a --with-ssh config option
[PATCH 3/9] LDAP: Add support for SSH user public keys
[PATCH 4/9] DP: Add auxiliary info handler
This is used to handle non-NSS accounts, such as hosts.
[PATCH 5/9] IPA: Add auxiliary info handler
[PATCH 6/9] DP: Add support for hosts in sss_dp_get_account
Host requests are directed to the auxiliary info handler.
[PATCH 7/9] SSH: Responder
[PATCH 8/9] SSH: Common client code
[PATCH 9/9] SSH: AuthorizedKeysCommand client
Honza
--
Jan Cholasta
11 years, 1 month
No /etc/sssd/sssd.conf file distributed in Fedora16?
by Marco Pizzoli
Hi again,
On a fresh install of a Fedora16 system I notice that does not exist the
/etc/sssd/sssd.conf file, although doing "rpm -ql sssd" I can see it listed.
Obviously I'm talking of the sssd package distributed with the release:
sssd-1.6.2-5.fc16.x86_64.
Is it a wanted behaviour?
Thanks
Marco
--
_________________________________________
Non è forte chi non cade, ma chi cadendo ha la forza di rialzarsi.
Jim Morrison
11 years, 1 month
Error in man sssd-ldap ?
by Marco Pizzoli
Hi, I hope to be of help in asking/reporting this kind of things.
In man sssd-ldap:
- ldap_enumeration_refresh_timeout (integer)
The LDAP attribute that contains how many seconds SSSD has to wait before
refreshing its cache of enumerated records.
Default: 300
Probably this is not a LDAP attribute. Am I right?
---------------------------------
- ldap_purge_cache_timeout <missing parameter type>
Marco
--
_________________________________________
Non è forte chi non cade, ma chi cadendo ha la forza di rialzarsi.
Jim Morrison
11 years, 1 month
sssd.conf: is it case sensitive?
by Marco Pizzoli
Hi,
in man sssd.conf I see boolean directives listed as lower case in the
services section and upper case in the domains section.
Do I have to follow the case sensitiveness specified or is it simply a typo?
Thanks again
Marco
--
_________________________________________
Non è forte chi non cade, ma chi cadendo ha la forza di rialzarsi.
Jim Morrison
11 years, 1 month
id_provider -> local: SSSD internal local provider
by Marco Pizzoli
Hi guys,
please could you explain what the meaning of "local" as id_provider is?
I can figure out it is a SSSD-local user database. By specifing this I'm
not considering the /etc/passwd file, am I right?
If I would like to point to /etc/passwd do I have to specify "proxy" and
eventually in /etc/nsswitch.conf "passwd files". Am I right again?
Thanks a lot as usual
Marco
--
_________________________________________
Non è forte chi non cade, ma chi cadendo ha la forza di rialzarsi.
Jim Morrison
11 years, 1 month
[PATCH] SUDO - fix sysdb_sudo_check_time()
by Pavel Březina
There is an invalid condition in sysdb_sudo_check_time() which results
in all rules having been filtered out if sudo_timed is on. This patch
fixes it.
11 years, 1 month
[PATCH] Changing password in shadow expiration policy
by Jan Zelený
Please note that I haven't fully tested this yet, the LDAP server configuration
needed for this is a little bit twisted ;-) I will perform more testing during
the weekend. Consider this patch being preliminary and don't push it until
it's tested.
Thanks
Jan
11 years, 1 month
[PATCHES] Support of SELinux user maps in SSSD
by Jan Zelený
#084:
Original #0001, already Acked
#085:
Original #0002, already Acked
#086:
Original #0003, already Acked
#087:
Original #0004, already Acked
#088:
Original #0005, already Acked
#089:
Original #0007, comments:
> > > In match_entity() and sss_selinux_match(), do we have a guarantee that
> > > the entities being matched are UTF-8 strings? If not, you should
> > > probably use the sss_utf8_case_eq() routine.
> >
> > They are DNs. I'm not quite sure they need to be treated as UTF-8
> > strings.
>
> Sorry, I meant ASCII above, but you answered the question I meant to ask
> anyway :)
>
> That said, I think our better bet would be to actually convert them to
> ldb_dn objects with ldb_dn_new() and use ldb_dn_compare() to determine
> if they're the same. This will handle any case-sensitivity issues in an
> LDB-appropriate way.
Actually I already looked into that and it would require to pass ldb context
to the matching function. I don't like that, this solution seems to be good
enough to me.
> That said, you have a prototype for sss_selinux_extract_usermaps() but
> it's not defined anywhere. I think this is where I got confused.
Done
#090:
Original #0006, comments:
> When using an enum, prefer a switch statement around 'type' (and skip
> the 'default' case). This way the compiler would catch any potential
> mistakes here such as adding a new type and not checking for it.
Done. Although I'm considering also implementing if after this switch (just in
case on some place an ordinary int value will be given instead of the proper
enum value).
> > TODO: I just noticed that it isn't fixed before sending the email and I
> > don't have the strength to do it right now. I'll send the fix later. How
> > about a conditional ack for the patch until this is done? ;-)
>
> Well, there are other things left to fix, so we might as well get this
> in too.
Done
> > > The number of usermaps in the cache is theoretically unbounded. It's
> > > probably not safe to allocate an array to contain all of them if we're
> > > going to be matching and holding onto only a subset of them. I'd rather
> > > that you just realloc to fit each one that we filter (or if you believe
> > > that this is going to occur computationally-often, write a simple
> > > doubling algorithm). As it is, I'm wary of doing ALL of the processing
> > > as a post-operation. Is there any way to improve the sysdb search
> > > filter to reduce what we have to filter through?
> >
> > I thought about this for quite some time. About the memory allocation:
> > I'm against reallocation of the final array one by one. The allocation
> > of the entire array could be solved by two-pass in place filtering
> > algorithm (with subsequent realloc), but it won't solve the issue that
> > potentially large amount of memory has to be allocated to store all the
> > ldb search results (that's why I left the second array - it's quite
> > small compared to the memory we need to store all fetched results).
>
> That's a good point. The array of pointers is going to be negligible in
> comparison to the array of sysdb_attrs.
>
> > I don't believe there's a good solution for this, since the user can be
> > member of unlimited number of groups (and our experience says the real
> > number is sometimes pretty high). That's why the post-processing is
> > necessary. If the goal was to improve memory consumption no matter what,
> > the solution would be to do multiple ldb searches, one for each group
> > the user is member of. But I don't like that scenario.
>
> Hmm, it's more work, but it WOULD keep the memory down. This area of the
> code really could become a pretty serious memory bottleneck in large
> deployments. That said, I'm going to suggest that we go with this for
> now and open a ticket to switch to a search per group later on. At the
> very least I want to have a ticket to identify a possible pain point in
> the future.
>
> And obviously such an approach would have a much better effect of
> keeping the memory down than just limiting the pointer array.
I want to consult the alternative approach with Rob first. I will CC you in the
conversation.
#091:
original #0009, comments:
> While I recognize that the way you're splitting the order_array is
> fairly clever, it's also somewhat difficult to read (and its memory
> hierarchy is somewhat wrong, since the array and the contents are both
> allocated at the same level on the tmp_ctx).
>
> I'd prefer if you just walked through the order string until you hit a
> '$', set that '$' to a '\0' and then did
> order_array[i] = talloc_strdup(order_array, p);
>
> (Where p is a pointer to the start of the 'word'.
>
> You can then free 'order' after that and the order_array will have a
> clear memory hierarchy (and the code will be more easily read).
That approach wouldn't make much difference, I still would need to make it two-
pass, it would change just one line of code and add unnecessary overhead. If
you insist on the correct memory hierarchy (which isn't particularly useful in
that part of the code), I could steal the original string on top of the order
array.
> However, I'm pretty sure usernames in PAM can be unicode strings, so you
> need to use the utf-8 case-insensitive string compare for matching the
> users to the order list.
Perhaps, but these are SELinux user identities, not pam usernames. I'm not
completely sure about these, but I assume they are ASCII strings.
> > > Among other things, it might be nicer to write a comparison routine
> > > that, if it passes, you append to file_content. This convoluted
> > > triply-nested loop is hard to grok.
> >
> > Done
>
> Thanks, but see above comment about unicode comparisons.
I'm not sure what do you exactly mean? Use UTF-8/ldb_dn comparison for this? I
Based on my previous comments I think this is good as it is.
#092:
Original #0010, comments:
> Please issue a syslog message if the mkstemp() or write() fails. This is
> probably going to end up being an SELinux or permission issue.
>
> Also, please test whether this change is going to necessitate a change
> to the SELinux policy shipped in Fedora. I don't know offhand if all
> applications have privileges to write to mkstemp().
Done
#093 - #096:
Original #0008, comments:
> First point: yes, this needs to be at least two patches. The first
> should create the common session creation functionality. The second
> should implement it in the IPA provider.
Done, although the final granularity is a bit finer.
> Please reorder ipa_session.c so that the _recv() functions follow the
> _send/<steps>/_done functions for a tevent request.
> ipa_get_selinux_recv() does not belong between ipa_session_handler() and
> ipa_get_selinux_done().
Done
> Also, your naming of ipa_get_selinux_done() is wrong. Call it something
> like ipa_session_handler_done(). It's not part of the
> ipa_get_selinux_*() tevent_req, so don't name it like it is.
Done
> In ipa_get_selinux_done() you have FIXME to add an outer transaction.
> Please do this. Both sysdb_store_selinux_config() and
> ipa_save_user_maps() are synchronous actions, so it's perfectly safe.
Done
> Please don't use numeric DEBUG levels any more. Use the appropriate
> macro level. For example, in ipa_get_selinux_send(), the connection
> status should probably be SSSDBG_TRACE_INTERNAL, not level 9 (AKA
> SSSDBG_TRACE_ALL). SSSDBG_TRACE_ALL should be reserved for REALLY
> esoteric data (like pointer addresses in the sbus connection code).
Sorry, that was copy-paste remainder. Corrected.
> In ipa_get_selinux_send(), I'd rather you handled the label like this:
>
> ...
> return req;
>
> immediate:
> if (ret == EOK) {
> tevent_req_done(req);
> } else {
> tevent_req_error(req, ret);
> }
> tevent_req_post(req, ev);
> return req;
> }
Done
> Furthermore, I don't think we want to return success for offline. You
> should add support for dp_err, errnop and err_msg to the
> ipa_get_selinux_state and pass those back in ipa_get_selinux_recv().
> Then extend ipa_session_reply() to return these values. This way you
> will properly return DP_ERR_OFFLINE and how to handle that will belong
> to the responder. You should handle this appropriately in each of the
> step functions here as well.
Done, although I removed the the ipa_session_reply() because it was basically
just an alias.
> I just made this same comment on a private review of the SSH key patches
> in progress:
> "Please do not reimplement ipa_get_hosts_send() completely. Please
> modify ipa_hbac_host_info_send() so that you can maintain a common
> routine. (You should be able to just add a boolean to skip the hostgroup
> lookup and whatever new attributes you need)"
>
> Please work with Honza (jcholast(a)redhat.com) to come up with a common
> routine that all three features (SELinux, SSH and HBAC) can use.
Done, see patch #093
> ipa_selinux_get_maps_state should not be public. Proper encapsulation
> means that it should only ever be visible to the
> ipa_selinux_get_maps_send() tevent_req. If any external application
> needs data from it, they should be getting it from
> ipa_selinux_get_maps_recv();
I was planning to do that, I guess I forgot. Done
> If you get to the done: label in ipa_get_selinux_maps_done() via
> receiving no selinux maps, you're not calling tevent_req_done(). Thus
> the request will never complete.
Actually the same error could happen on more occasions. Good catch, thank you.
Corrected.
> I have the same concern here about the greedy array allocation that I
> had in the sysdb code. There could be hundreds (or thousands) of
> possible user maps to filter through. I don't want to waste that much
> memory on the confirmed and possible matches.
TBD later
> Wrap the call to ipa_get_config_send() in a function to be used in both
> ipa_get_selinux_maps_done() and ipa_get_selinux_hbac_done().
I'm not sure what would be the benefit here. I looked into this briefly and it
would probably add more redundant code than it would remove.
Thanks for the review. I'm sorry for this format of the email, but it still
seems more suitable than to respond to two different emails.
Jan
11 years, 1 month