On Wed, Dec 21, 2011 at 04:20:09PM +0100, Jan Zelený wrote:
https://fedorahosted.org/sssd/ticket/1105 (review ticket)
https://fedorahosted.org/sssd/ticket/623 (sudo integration)
Hello,
it is finally here.
These patches *assume* that "Responders: Split getting domain by name
into separate function" patch has been applied, it can be found in
"Ability to set a domain as case sensitive or insensitive" thread.
Design page:
https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOIntegrationNewApproac
h
What's left:
1. implementing support of sssd in sudo sudoers plugin
2. periodical update of all rules
3. provide documentation
4. in memory cache in responder
5. refactor DP request using "RESPONDER: Refactor DP requests into
tevent_req style" patch
6. provide few more configuration options
7. support the IPA scheme
How to configure it:
1. configure --enable-all-experimental-features
2. sssd.conf:
services += sudo
domain options: sudo_provider, ldap_sudo_search_base
How to test it:
> sss_sudo_cli username
This will display rules for %username. It performs two lookups:
1. using native sssd api to get and display raw date (it prints every
byte as char so don't be scared of silly characters that are shown
instead of numbers)
2. using api that we provide to sudo and use it to display data in user
readable format
Big thanks to Jakub, who has written whole sysdb api and a big part of
responder.
Regards,
Pavel Březina.
Ok, I've spent last couple days reviewing the code. My brief comments are
below. Please let me know if any of those comments needs an explanation,
I realize they are written in super-brief format.
I'd also like to point out that the code is pretty complex, so it is very
likely that I missed some issues. If anyone else is interested, feel free
to do the review as well.
If no file is specified, the line number means the line in the patch
itself.
Pavel, please note that I've not taken your second set of patches into
account, I just didn't have the strength for that, perhaps next year ;-)
#0001:
line 105: talloc_array -> talloc
talloc_array is better choice there...we want to allocate an array of
"struct sysdb_attrs pointers"
I know, I wrote a wrong line number. I meant line 40 (in the DEBUG message).
Sorry for the confusion.
#0002:
I'm not an autotools expert
Me neither :)
, but don't lines 34-36 mean the same as line 33?
AM_CONDITIONAL defines an automake conditional for use in Makefile.am,
but we also need AC_DEFINE to have a constant #defined in config.h
Ok then, Ack for patch #0002.
#0003:
I'm not sure about the NULL_CHECK macro. I don't condemn it but if I
choose to accept it, I'll want it to be used everywhere it's possible.
At least in the scope if this patch.
That was the intent but I forgot.
Nitpick: in the for on line 231 I'd add spaces around the = char
OK
line 359: I don't think it is necessary to end purging the tree. I think
continuing is better action in this case.
I was trying to be defensive and avoid ending up with rules that are no
longer present on the server which might grant access to users that are
not supposed to be in sudoers anymore..
Exactly my point. If you get out of purging cycle on the first rule that
doesn't have a name, you can potentially skip many more rules.
line 408: shouldn't the SUDORULE_SUBDIR be something like cn=sudorules
rather than just sudorules?
No, the SYSDB_TMPL_CUSTOM filter hardcodes cn=
Ok, thanks for clearing that up.
#0004:
Nitpick: line 166: I'd move the condition to the following block starting
with if (!dbus_ret)
Yes, it's supposed to be there.
line 199: really EOK?
Thanks, it's supposed to say 'ret'
line 270: is there a reason why we have one-member struct? Is there a
plan for the future with this?
Not sure, but wrapping the data in a structure provides encapsulation for
the future and does no harm.
Ok. In general I have no problem with that, it just caught my eye.
#0005:
line 33: really SDAP_NETGROUP_SEARCH_BASE?
That should say SDAP_SUDO_SEARCH_BASE. This code is commented out with
#if 0, though, until we provide support for IPA sudo rules.
I know, but for the sake of reducing possibility to overlook it in the future,
I'd prefer to fix it now.
line 109: I find this misleading - the map is called native but several
lines above, there is a statement that native sudo rules are not
supported
Right, that's a mismatch between "native SUDO rules" and "native IPA
SUDO rules".
line 131: for the comment to be true, you have to add
SDAP_SUDO_SEARCH_BASE to search_base_options
Correct.
lines 148-168: I don't get this logic. Is it forbidden to set sudo search
base if no ldap_search_base is specified?
Yes I think that the logic is backwards.
I was under the impression that ldap_search_base is only a fallback in case
the more specific search base is not defined. But that impression came from IPA
provider, now I see that the code in LDAP provider is different. Therefore I
withdraw my comment.
#0006:
Ack
#0007:
line 534: is the TODO still valid?
No. I love removing TODOs :)
line 461: in the NSS responder, there is a check for NULL termination of
the string. Can something similar happen here as well?
According to the current sudo sources, it can't, but better safe than
sorry.
responder patches:
Is there a plan to implement negative cache like in NSS responder? At
least for the sudosrv_get_user part.
I was planning to reuse sss_ncache_check_user()
I also wonder if the original NSS responder
routing nss_cmd_getpwnam_search could be utilized somehow instead of
writing its copy. That would solve the negative cache issue.
The same routing is used all over the place. With the number of
responders and supported maps growing the basic patterns of iterating
over domains is used very often. I was thinking about using a more
generic function (or perhaps a macro would be better..) to reuse the
commonly used code. Maybe something like NSS_GETENT() in nss-pam-ldapd
code.
Yes, that was basically my idea. I filed a ticket to track this:
https://fedorahosted.org/sssd/ticket/1126
sudosrv_dp.c: 92 - missing dbus_message_unref() in the error handling
block
Right. We should fix it until we reuse the tevent_req DP requests in
that code.
client patches:
First of all I'd like to have sss_sudo_get_result() renamed. The name is
quite confusing considering that it doesn't only fetch the result, but
it also makes the request. I suggest using something like
sss_sudo_get_rules(), that would make things much more readable.
ok, let's rename it to sss_sudo_send_recv()
in sss_sudo_parse_rule: I don't think it's necessary to start variables
with underscore if you don't have their local variable counterparts. But
that's just a readability suggestion.
I'm not entirely sure about this newxt issue, but when you send response
to the client, you use SAFEALIGN_SET_UINT32 macro, but when receiving
the variable you don't use anything like that. I'm a bit concerned that
this might cause some issues on exotic architectures.
sorry, but where in the code is the issue? Plain casting pointer
dereference would be bad on architectures that don't align on word
boundary.
Well, it's not quite like that, memcpy is used. See sss_sudo_parse_uint32(). I
think that's the only place that hit me.
