[PATCHES] SUDO Integration

[Pavel Březina]

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/SUDOIntegrationNewApproach

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.

[Jakub Hrozek]

On Fri, Dec 16, 2011 at 02:02:11PM +0100, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/1105 (review ticket)
> https://fedorahosted.org/sssd/ticket/623 (sudo integration)
>
> Hello,
> it is finally here.

yay!
> 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/SUDOIntegrationNewApproach
>
> What's left:
> implementing support of sssd in sudo sudoers plugin

This is Daniel's part using the API we provided. The whole sudo
integration feature could be moved out of "experimental" stage when
there's sudo package that is able to talk to SSSD.

One thing I'd like to get feedback on at this point is how do we
collaborate on the API with sudo.

The API is a collection of functions that are able to pack the sudo
request and send it over the socket to SSSD and parse the wire data
received from SSSD.

I'm not sure it would be wise to release the API as a full-fledged shared
library (although technically it makes sense) - some distributions such
as RHEL keep their libraries ABI compatible for the lifetime of the
distribution. What if we need to modify the API next release?

So far, Daniel just copied the API source into his WIP sudo tree so he
could develop the sudo part.
> periodical update of all rules

Basically an equivalent of the "enumerate" task.
https://fedorahosted.org/sssd/ticket/1110
> provide documentation

Include all the options and the responder itself in the manual pages
https://fedorahosted.org/sssd/ticket/1109
> in memory cache in responder

Based on the design discussed on the list
https://fedorahosted.org/sssd/ticket/1111
> refactor DP request using "RESPONDER: Refactor DP requests into
>
> tevent_req style" patch

https://fedorahosted.org/sssd/ticket/1115

With a little more detail - I'd like to see a new tevent_req style request
along the lines of sss_dp_get_account_send/_done/_recv just for sudo
rules. I'd like to reuse sss_dp_get_account_int_* request - there's a
couple of lines that are account specific, but I believe we could reuse
and share the code after very little refactoring.
> provide few more configuration options

https://fedorahosted.org/sssd/ticket/1116
> support the IPA scheme

https://fedorahosted.org/sssd/ticket/1108
> How to configure it:
> configure --enable-all-experimental-features
> 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:
> 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

This is exactly what sudo will be doing so the SSSD part of the effort
is quite testable at the moment.
> Big thanks to Jakub, who has written whole sysdb api and a big part
> of responder.
>
> Regards,
> Pavel Březina.

[Stephen Gallagher]

On Fri, 2011-12-16 at 17:19 +0100, Jakub Hrozek wrote:
> I'm not sure it would be wise to release the API as a full-fledged shared
> library (although technically it makes sense) - some distributions such
> as RHEL keep their libraries ABI compatible for the lifetime of the
> distribution. What if we need to modify the API next release?

I think this is an exceptional case, as there will never be any consumer
besides sudo. So I think we'll be okay with getting ABI exceptions if
such a need arises.

[Stephen Gallagher]

On Fri, 2011-12-16 at 14:02 +0100, Pavel Březina 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/SUDOIntegrationNewApproach
>
> What's left:
> implementing support of sssd in sudo sudoers plugin
> periodical update of all rules
> provide documentation
> in memory cache in responder
> 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:
> configure --enable-all-experimental-features
> 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:
> 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.

I'm acking this and pushing it to master provisionally. It's
self-contained code that only builds with the
--enable-all-experimental-features flag, so we're going to break with
our usual process here somewhat and get the changes in now. We'll tweak
what needs tweaking during the 1.8.0 process, but there's no reason to
hold it back in the meantime.

I'm pushing the patches with one minor modification to the sysdb patch
to fix a -Wformat-security warning due to passing a variable directly to
a talloc_asprintf.

Pushed to master.

[Jakub Hrozek]

On Fri, Dec 16, 2011 at 02:49:18PM -0500, Stephen Gallagher wrote:
> On Fri, 2011-12-16 at 14:02 +0100, Pavel Březina 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/SUDOIntegrationNewApproach
> >
> > What's left:
> > implementing support of sssd in sudo sudoers plugin
> > periodical update of all rules
> > provide documentation
> > in memory cache in responder
> > 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:
> > configure --enable-all-experimental-features
> > 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:
> > 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.
>
> I'm acking this and pushing it to master provisionally. It's
> self-contained code that only builds with the
> --enable-all-experimental-features flag, so we're going to break with
> our usual process here somewhat and get the changes in now. We'll tweak
> what needs tweaking during the 1.8.0 process, but there's no reason to
> hold it back in the meantime.
>
> I'm pushing the patches with one minor modification to the sysdb patch
> to fix a -Wformat-security warning due to passing a variable directly to
> a talloc_asprintf.
>
> Pushed to master.

Thank you. I noticed on IRC that Jan expressed interest in reviewing the
patches. I still think this is a good idea. We could put the review
items into a new ticket and handle it in either 1.7 or 1.8.

[Stephen Gallagher]

On Dec 18, 2011, at 5:07 PM, Jakub Hrozek  wrote:
> On Fri, Dec 16, 2011 at 02:49:18PM -0500, Stephen Gallagher wrote:
> > On Fri, 2011-12-16 at 14:02 +0100, Pavel Březina 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/SUDOIntegrationNewApproach
> > >
> > > What's left:
> > > implementing support of sssd in sudo sudoers plugin
> > > periodical update of all rules
> > > provide documentation
> > > in memory cache in responder
> > > 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:
> > > configure --enable-all-experimental-features
> > > 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:
> > > 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.
> >
> > I'm acking this and pushing it to master provisionally. It's
> > self-contained code that only builds with the
> > --enable-all-experimental-features flag, so we're going to break with
> > our usual process here somewhat and get the changes in now. We'll tweak
> > what needs tweaking during the 1.8.0 process, but there's no reason to
> > hold it back in the meantime.
> >
> > I'm pushing the patches with one minor modification to the sysdb patch
> > to fix a -Wformat-security warning due to passing a variable directly to
> > a talloc_asprintf.
> >
> > Pushed to master.
>
> Thank you. I noticed on IRC that Jan expressed interest in reviewing the
> patches. I still think this is a good idea. We could put the review
> items into a new ticket and handle it in either 1.7 or 1.8.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

I agree. Please open a ticket.

[Jakub Hrozek]

On Sun, Dec 18, 2011 at 05:37:17PM -0500, Stephen Gallagher wrote:
> On Dec 18, 2011, at 5:07 PM, Jakub Hrozek  wrote:
> > On Fri, Dec 16, 2011 at 02:49:18PM -0500, Stephen Gallagher wrote:
> > > On Fri, 2011-12-16 at 14:02 +0100, Pavel Březina 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/SUDOIntegrationNewApproach
> > > >
> > > > What's left:
> > > > implementing support of sssd in sudo sudoers plugin
> > > > periodical update of all rules
> > > > provide documentation
> > > > in memory cache in responder
> > > > 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:
> > > > configure --enable-all-experimental-features
> > > > 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:
> > > > 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.
> > >
> > > I'm acking this and pushing it to master provisionally. It's
> > > self-contained code that only builds with the
> > > --enable-all-experimental-features flag, so we're going to break with
> > > our usual process here somewhat and get the changes in now. We'll tweak
> > > what needs tweaking during the 1.8.0 process, but there's no reason to
> > > hold it back in the meantime.
> > >
> > > I'm pushing the patches with one minor modification to the sysdb patch
> > > to fix a -Wformat-security warning due to passing a variable directly to
> > > a talloc_asprintf.
> > >
> > > Pushed to master.
> >
> > Thank you. I noticed on IRC that Jan expressed interest in reviewing the
> > patches. I still think this is a good idea. We could put the review
> > items into a new ticket and handle it in either 1.7 or 1.8.
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://fedorahosted.org/mailman/listinfo/sssd-devel
>
> I agree. Please open a ticket.

https://fedorahosted.org/sssd/ticket/1105

[Jan Zelený]

> On Sun, Dec 18, 2011 at 05:37:17PM -0500, Stephen Gallagher wrote:
> > On Dec 18, 2011, at 5:07 PM, Jakub Hrozek  wrote:
> > > On Fri, Dec 16, 2011 at 02:49:18PM -0500, Stephen Gallagher wrote:
> > > > On Fri, 2011-12-16 at 14:02 +0100, Pavel Březina 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/SUDOIntegrationNewAppro
> > > > > ach
> > > > >
> > > > > What's left:
> > > > > implementing support of sssd in sudo sudoers plugin
> > > > > periodical update of all rules
> > > > > provide documentation
> > > > > in memory cache in responder
> > > > > 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:
> > > > >
> > > > > configure --enable-all-experimental-features
> > > > >
> > > > > 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:
> > > > > 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.
> > > >
> > > > I'm acking this and pushing it to master provisionally. It's
> > > > self-contained code that only builds with the
> > > > --enable-all-experimental-features flag, so we're going to break with
> > > > our usual process here somewhat and get the changes in now. We'll
> > > > tweak what needs tweaking during the 1.8.0 process, but there's no
> > > > reason to hold it back in the meantime.
> > > >
> > > > I'm pushing the patches with one minor modification to the sysdb patch
> > > > to fix a -Wformat-security warning due to passing a variable directly
> > > > to a talloc_asprintf.
> > > >
> > > > Pushed to master.
> > >
> > > Thank you. I noticed on IRC that Jan expressed interest in reviewing
> > > the patches. I still think this is a good idea. We could put the
> > > review items into a new ticket and handle it in either 1.7 or 1.8.
> > > _______________________________________________
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > I agree. Please open a ticket.
>
> https://fedorahosted.org/sssd/ticket/1105

Thanks, I'll take a look at the code during this afternoon.

Jan

[Pavel Březina]

While working on periodical updates I've noticed two things:

[PATCH 1/2] removes be_sudo_req from sudo_ctx because it is not used 
anywhere.

[PATCH 2/2] fixes possible memory leak if sdap_sudo_handler() fails.

[Jakub Hrozek]

On Tue, Dec 20, 2011 at 10:31:04AM +0100, Pavel Březina wrote:
> While working on periodical updates I've noticed two things:
>
> [PATCH 1/2] removes be_sudo_req from sudo_ctx because it is not used
> anywhere.

Ack
> [PATCH 2/2] fixes possible memory leak if sdap_sudo_handler() fails.

The memory wouldn't be leaked, it was allocated on be_req, so we would
clean it as soon as the client disconnects. But releasing memory when
it's no longer needed is good manners, so Ack

[Stephen Gallagher]

On Tue, 2011-12-20 at 11:11 +0100, Jakub Hrozek wrote:
> On Tue, Dec 20, 2011 at 10:31:04AM +0100, Pavel Březina wrote:
> > While working on periodical updates I've noticed two things:
> >
> > [PATCH 1/2] removes be_sudo_req from sudo_ctx because it is not used
> > anywhere.
>
> Ack
> > [PATCH 2/2] fixes possible memory leak if sdap_sudo_handler() fails.
>
> The memory wouldn't be leaked, it was allocated on be_req, so we would
> clean it as soon as the client disconnects. But releasing memory when
> it's no longer needed is good manners, so Ack

Pushed both to master.

[Jan Zelený]

> 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/SUDOIntegrationNewApproach
>
> What's left:
> implementing support of sssd in sudo sudoers plugin
> periodical update of all rules
> provide documentation
> in memory cache in responder
> 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:
> configure --enable-all-experimental-features
> 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:
> 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

#0002:
I'm not an autotools expert, but don't lines 34-36 mean the same as line 33?

#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.

Nitpick: in the for on line 231 I'd add spaces around the = char

line 359: I don't think it is necessary to end purging the tree. I think 
continuing is better action in this case.

line 408: shouldn't the SUDORULE_SUBDIR be something like cn=sudorules rather 
than just sudorules?

#0004:
Nitpick: line 166: I'd move the condition to the following block starting with 
if (!dbus_ret)
line 199: really EOK?
line 270: is there a reason why we have one-member struct? Is there a plan for 
the future with this?

#0005:
line 33: really SDAP_NETGROUP_SEARCH_BASE?
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
line 131: for the comment to be true, you have to add SDAP_SUDO_SEARCH_BASE to 
search_base_options
lines 148-168: I don't get this logic. Is it forbidden to set sudo search base 
if no ldap_search_base is specified?

#0006:
Ack

#0007:
line 534: is the TODO still valid?
line 461: in the NSS responder, there is a check for NULL termination of the 
string. Can something similar happen here as well?

responder patches:
Is there a plan to implement negative cache like in NSS responder? At least 
for the sudosrv_get_user part. 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.

sudosrv_dp.c: 92 - missing dbus_message_unref() in the error handling block

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.

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.

That's all folks
Sorry if I was too strict or nit-picky
Jan

[Jakub Hrozek]

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/SUDOIntegrationNewApproach
> >
> > What's left:
> > implementing support of sssd in sudo sudoers plugin
> > periodical update of all rules
> > provide documentation
> > in memory cache in responder
> > 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:
> > configure --enable-all-experimental-features
> > 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:
> > 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"
> #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
> #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..
> line 408: shouldn't the SUDORULE_SUBDIR be something like cn=sudorules rather 
> than just sudorules?

No, the SYSDB_TMPL_CUSTOM filter hardcodes cn=
> #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.
> #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.
> 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.
> #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.
> 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.

Thank you for the review. I filed
https://fedorahosted.org/sssd/ticket/1125 to track the comments.

[Jan Zelený]

> 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:
> > > implementing support of sssd in sudo sudoers plugin
> > > periodical update of all rules
> > > provide documentation
> > > in memory cache in responder
> > > 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:
> > >
> > > configure --enable-all-experimental-features
> > >
> > > 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:
> > > 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.

Jan

[Pavel Březina]

Dne 3.1.2012 09:29, Jan Zelený napsal(a):
> > 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:
> > > > implementing support of sssd in sudo sudoers plugin
> > > > periodical update of all rules
> > > > provide documentation
> > > > in memory cache in responder
> > > > 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:
> > > >
> > > > configure --enable-all-experimental-features
> > > >
> > > > 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:
> > > > 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.

We want the client API to be as small as possible because it will be 
moved to sudo. SAFEALIGN_* macros are defined in util.h so I decided to 
use memcpy() directly.
> Jan
>
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

[Jan Zelený]

Pavel Březina  wrote:
> Dne 3.1.2012 09:29, Jan Zelený napsal(a):
> > > 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/SUDOIntegrationNewApproa
> > > > > c h
> > > > >
> > > > > What's left:
> > > > > implementing support of sssd in sudo sudoers plugin
> > > > > periodical update of all rules
> > > > > provide documentation
> > > > > in memory cache in responder
> > > > > 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:
> > > > >
> > > > > configure --enable-all-experimental-features
> > > > >
> > > > > 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:
> > > > > 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.
>
> We want the client API to be as small as possible because it will be
> moved to sudo. SAFEALIGN_* macros are defined in util.h so I decided to
> use memcpy() directly.

Ok, please disregard my previous comment. I suppose that SAFEALIGN_* macros 
won't be subjects to change in the foreseeable future. Please consider at 
least a comment in the code that this way is safe because it is the same what 
SAFEALIGN_* does.

Thanks
Jan

[Pavel Březina]

Dne 5.1.2012 08:30, Jan Zeleny napsal(a):
> > > > > #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.
> > > > > #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.
> > > > > #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'

Actually there should be EOK. At least the other be handlers returns EOK 
in any case.

If it returns value different than EOK it will send some generic message 
back to the responder. This way I can specify format of the message.
> > > > > #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.

The comment is invalid, we load sudo search base in ldap_get_sudo_options().
> > > > > #0007:
> > > > > line 534: is the TODO still valid?
> > > >
> > > > No. I love removing TODOs :)

I used the first patch from "SUDO Integration - periodical update thread".
> > > > > 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.

If you mean line 461 in #0008 then I believe it is done in 
sudosrv_get_sudorules_parse_query().
> > > > > 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.
> >
> > We want the client API to be as small as possible because it will be
> > moved to sudo. SAFEALIGN_* macros are defined in util.h so I decided to
> > use memcpy() directly.
>
> Ok, please disregard my previous comment. I suppose that SAFEALIGN_* macros
> won't be subjects to change in the foreseeable future. Please consider at
> least a comment in the code that this way is safe because it is the same what
> SAFEALIGN_* does.

Thank you for the review.

Patch addressing these issues is attached.
It also includes:
  - sudo configuration options added to SSSDConfig.py
  - attrs[] in sysdb_sudo_purge_subdir() is now terminated with NULL
  - in sudosrv_get_sudorules_parse_query() I use strnlen() instead of
    strlen() to check validity of the input

Ticket for this patch is https://fedorahosted.org/sssd/ticket/1125

[Pavel Březina]

Dne 12.1.2012 12:17, Pavel Březina napsal(a):
> Dne 5.1.2012 08:30, Jan Zeleny napsal(a):
> > > > > > #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.
> > > > > > #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.
> > > > > > #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'
>
> Actually there should be EOK. At least the other be handlers returns EOK
> in any case.
>
> If it returns value different than EOK it will send some generic message
> back to the responder. This way I can specify format of the message.
> > > > > > #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.
>
> The comment is invalid, we load sudo search base in
> ldap_get_sudo_options().
> > > > > > #0007:
> > > > > > line 534: is the TODO still valid?
> > > > >
> > > > > No. I love removing TODOs :)
>
> I used the first patch from "SUDO Integration - periodical update thread".
> > > > > > 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.
>
> If you mean line 461 in #0008 then I believe it is done in
> sudosrv_get_sudorules_parse_query().
> > > > > > 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.
> > >
> > > We want the client API to be as small as possible because it will be
> > > moved to sudo. SAFEALIGN_* macros are defined in util.h so I decided to
> > > use memcpy() directly.
> >
> > Ok, please disregard my previous comment. I suppose that SAFEALIGN_*
> > macros
> > won't be subjects to change in the foreseeable future. Please consider at
> > least a comment in the code that this way is safe because it is the
> > same what
> > SAFEALIGN_* does.
>
> Thank you for the review.
>
> Patch addressing these issues is attached.
> It also includes:
> sudo configuration options added to SSSDConfig.py
> attrs[] in sysdb_sudo_purge_subdir() is now terminated with NULL
> in sudosrv_get_sudorules_parse_query() I use strnlen() instead of
>
> strlen() to check validity of the input
>
> Ticket for this patch is https://fedorahosted.org/sssd/ticket/1125

I'm sending it one more time with the options included in 
config/etc/sssd.api.d/sssd-ldap.conf

[Pavel Březina]

Dne 12.1.2012 12:42, Pavel Březina napsal(a):
> Dne 12.1.2012 12:17, Pavel Březina napsal(a):
> > Dne 5.1.2012 08:30, Jan Zeleny napsal(a):
> > > > > > > #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.
> > > > > > > #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.
> > > > > > > #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'
> >
> > Actually there should be EOK. At least the other be handlers returns EOK
> > in any case.
> >
> > If it returns value different than EOK it will send some generic message
> > back to the responder. This way I can specify format of the message.
> > > > > > > #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.
> >
> > The comment is invalid, we load sudo search base in
> > ldap_get_sudo_options().
> > > > > > > #0007:
> > > > > > > line 534: is the TODO still valid?
> > > > > >
> > > > > > No. I love removing TODOs :)
> >
> > I used the first patch from "SUDO Integration - periodical update
> > thread".
> > > > > > > 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.
> >
> > If you mean line 461 in #0008 then I believe it is done in
> > sudosrv_get_sudorules_parse_query().
> > > > > > > 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.
> > > >
> > > > We want the client API to be as small as possible because it will be
> > > > moved to sudo. SAFEALIGN_* macros are defined in util.h so I decided to
> > > > use memcpy() directly.
> > >
> > > Ok, please disregard my previous comment. I suppose that SAFEALIGN_*
> > > macros
> > > won't be subjects to change in the foreseeable future. Please
> > > consider at
> > > least a comment in the code that this way is safe because it is the
> > > same what
> > > SAFEALIGN_* does.
> >
> > Thank you for the review.
> >
> > Patch addressing these issues is attached.
> > It also includes:
> > sudo configuration options added to SSSDConfig.py
> > attrs[] in sysdb_sudo_purge_subdir() is now terminated with NULL
> > in sudosrv_get_sudorules_parse_query() I use strnlen() instead of
> >
> > strlen() to check validity of the input
> >
> > Ticket for this patch is https://fedorahosted.org/sssd/ticket/1125
>
> I'm sending it one more time with the options included in
> config/etc/sssd.api.d/sssd-ldap.conf

I forgot to put sudo_provider option to sssd.api.conf... I hope this is 
finally everything :-)

[Pavel Březina]

Dne 13.1.2012 10:00, Pavel Březina napsal(a):
> Dne 12.1.2012 12:42, Pavel Březina napsal(a):
> > Dne 12.1.2012 12:17, Pavel Březina napsal(a):
> > > Dne 5.1.2012 08:30, Jan Zeleny napsal(a):
> > > > > > >> #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.
> > > > > >
> > > > > > >> #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.
> > > > > >
> > > > > > >> #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'
> > >
> > > Actually there should be EOK. At least the other be handlers returns EOK
> > > in any case.
> > >
> > > If it returns value different than EOK it will send some generic message
> > > back to the responder. This way I can specify format of the message.
> > > > > > >> #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.
> > >
> > > The comment is invalid, we load sudo search base in
> > > ldap_get_sudo_options().
> > > > > > >> #0007:
> > > > > > >> line 534: is the TODO still valid?
> > > > > > >
> > > > > > > No. I love removing TODOs :)
> > >
> > > I used the first patch from "SUDO Integration - periodical update
> > > thread".
> > > > > > >> 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.
> > >
> > > If you mean line 461 in #0008 then I believe it is done in
> > > sudosrv_get_sudorules_parse_query().
> > > > > > >> 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.
> > > > >
> > > > > We want the client API to be as small as possible because it will be
> > > > > moved to sudo. SAFEALIGN_* macros are defined in util.h so I
> > > > > decided to
> > > > > use memcpy() directly.
> > > >
> > > > Ok, please disregard my previous comment. I suppose that SAFEALIGN_*
> > > > macros
> > > > won't be subjects to change in the foreseeable future. Please
> > > > consider at
> > > > least a comment in the code that this way is safe because it is the
> > > > same what
> > > > SAFEALIGN_* does.
> > >
> > > Thank you for the review.
> > >
> > > Patch addressing these issues is attached.
> > > It also includes:
> > > sudo configuration options added to SSSDConfig.py
> > > attrs[] in sysdb_sudo_purge_subdir() is now terminated with NULL
> > > in sudosrv_get_sudorules_parse_query() I use strnlen() instead of
> > >
> > > strlen() to check validity of the input
> > >
> > > Ticket for this patch is https://fedorahosted.org/sssd/ticket/1125
> >
> > I'm sending it one more time with the options included in
> > config/etc/sssd.api.d/sssd-ldap.conf
>
> I forgot to put sudo_provider option to sssd.api.conf... I hope this is
> finally everything :-)

While working on another ticket I noticed missing attribute in attr[] in 
sysdb_purge_sudorule_subtree(). New review patch is attached and I swear 
I won't send another one till this one is acked or nacked :)

[Jakub Hrozek]

On Fri, Jan 13, 2012 at 02:48:26PM +0100, Pavel Březina wrote:
> Dne 13.1.2012 10:00, Pavel Březina napsal(a):
> > Dne 12.1.2012 12:42, Pavel Březina napsal(a):
> > > Dne 12.1.2012 12:17, Pavel Březina napsal(a):
> > > > Dne 5.1.2012 08:30, Jan Zeleny napsal(a):
> > > > > > >>>#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.
> > > > > > >
> > > > > > >>>#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.
> > > > > > >
> > > > > > >>>#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'
> > > >
> > > > Actually there should be EOK. At least the other be handlers returns EOK
> > > > in any case.
> > > >
> > > > If it returns value different than EOK it will send some generic message
> > > > back to the responder. This way I can specify format of the message.
> > > > > > >>>#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.
> > > >
> > > > The comment is invalid, we load sudo search base in
> > > > ldap_get_sudo_options().
> > > > > > >>>#0007:
> > > > > > >>>line 534: is the TODO still valid?
> > > > > > >>
> > > > > > >>No. I love removing TODOs :)
> > > >
> > > > I used the first patch from "SUDO Integration - periodical update
> > > > thread".
> > > > > > >>>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.
> > > >
> > > > If you mean line 461 in #0008 then I believe it is done in
> > > > sudosrv_get_sudorules_parse_query().
> > > > > > >>>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.
> > > > > >
> > > > > > We want the client API to be as small as possible because it will be
> > > > > > moved to sudo. SAFEALIGN_* macros are defined in util.h so I
> > > > > > decided to
> > > > > > use memcpy() directly.
> > > > >
> > > > > Ok, please disregard my previous comment. I suppose that SAFEALIGN_*
> > > > > macros
> > > > > won't be subjects to change in the foreseeable future. Please
> > > > > consider at
> > > > > least a comment in the code that this way is safe because it is the
> > > > > same what
> > > > > SAFEALIGN_* does.
> > > >
> > > > Thank you for the review.
> > > >
> > > > Patch addressing these issues is attached.
> > > > It also includes:
> > > > sudo configuration options added to SSSDConfig.py
> > > > attrs[] in sysdb_sudo_purge_subdir() is now terminated with NULL
> > > > in sudosrv_get_sudorules_parse_query() I use strnlen() instead of
> > > >
> > > > strlen() to check validity of the input
> > > >
> > > > Ticket for this patch is https://fedorahosted.org/sssd/ticket/1125
> > >
> > > I'm sending it one more time with the options included in
> > > config/etc/sssd.api.d/sssd-ldap.conf
> >
> > I forgot to put sudo_provider option to sssd.api.conf... I hope this is
> > finally everything :-)
>
> While working on another ticket I noticed missing attribute in
> attr[] in sysdb_purge_sudorule_subtree(). New review patch is
> attached and I swear I won't send another one till this one is acked
> or nacked :)

Ack

I've notified Dan that we renamed sss_sudo_get_result() to
sss_sudo_send_recv()

[Stephen Gallagher]

On Sun, 2012-01-15 at 16:24 +0100, Jakub Hrozek wrote:
> On Fri, Jan 13, 2012 at 02:48:26PM +0100, Pavel Březina wrote:
> > Dne 13.1.2012 10:00, Pavel Březina napsal(a):
> > > Dne 12.1.2012 12:42, Pavel Březina napsal(a):
> > > > Dne 12.1.2012 12:17, Pavel Březina napsal(a):
> > > > > Dne 5.1.2012 08:30, Jan Zeleny napsal(a):
> > > > > > >>>>#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.
> > > > > > >>
> > > > > > >>>>#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.
> > > > > > >>
> > > > > > >>>>#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'
> > > > >
> > > > > Actually there should be EOK. At least the other be handlers returns EOK
> > > > > in any case.
> > > > >
> > > > > If it returns value different than EOK it will send some generic message
> > > > > back to the responder. This way I can specify format of the message.
> > > > > > >>>>#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.
> > > > >
> > > > > The comment is invalid, we load sudo search base in
> > > > > ldap_get_sudo_options().
> > > > > > >>>>#0007:
> > > > > > >>>>line 534: is the TODO still valid?
> > > > > > >>>
> > > > > > >>>No. I love removing TODOs :)
> > > > >
> > > > > I used the first patch from "SUDO Integration - periodical update
> > > > > thread".
> > > > > > >>>>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.
> > > > >
> > > > > If you mean line 461 in #0008 then I believe it is done in
> > > > > sudosrv_get_sudorules_parse_query().
> > > > > > >>>>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.
> > > > > > >
> > > > > > >We want the client API to be as small as possible because it will be
> > > > > > >moved to sudo. SAFEALIGN_* macros are defined in util.h so I
> > > > > > >decided to
> > > > > > >use memcpy() directly.
> > > > > >
> > > > > > Ok, please disregard my previous comment. I suppose that SAFEALIGN_*
> > > > > > macros
> > > > > > won't be subjects to change in the foreseeable future. Please
> > > > > > consider at
> > > > > > least a comment in the code that this way is safe because it is the
> > > > > > same what
> > > > > > SAFEALIGN_* does.
> > > > >
> > > > > Thank you for the review.
> > > > >
> > > > > Patch addressing these issues is attached.
> > > > > It also includes:
> > > > > sudo configuration options added to SSSDConfig.py
> > > > > attrs[] in sysdb_sudo_purge_subdir() is now terminated with NULL
> > > > > in sudosrv_get_sudorules_parse_query() I use strnlen() instead of
> > > > >
> > > > > strlen() to check validity of the input
> > > > >
> > > > > Ticket for this patch is https://fedorahosted.org/sssd/ticket/1125
> > > >
> > > > I'm sending it one more time with the options included in
> > > > config/etc/sssd.api.d/sssd-ldap.conf
> > >
> > > I forgot to put sudo_provider option to sssd.api.conf... I hope this is
> > > finally everything :-)
> >
> > While working on another ticket I noticed missing attribute in
> > attr[] in sysdb_purge_sudorule_subtree(). New review patch is
> > attached and I swear I won't send another one till this one is acked
> > or nacked :)
>
> Ack
>
> I've notified Dan that we renamed sss_sudo_get_result() to
> sss_sudo_send_recv()

Pushed to master.

[Stephen Gallagher]

On Fri, 2012-01-13 at 10:00 +0100, Pavel Březina wrote:
> Dne 12.1.2012 12:42, Pavel Březina napsal(a):
> > Dne 12.1.2012 12:17, Pavel Březina napsal(a):
> > > Dne 5.1.2012 08:30, Jan Zeleny napsal(a):
> > > > > > >> #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.
> > > > > >
> > > > > > >> #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.
> > > > > >
> > > > > > >> #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'
> > >
> > > Actually there should be EOK. At least the other be handlers returns EOK
> > > in any case.
> > >
> > > If it returns value different than EOK it will send some generic message
> > > back to the responder. This way I can specify format of the message.
> > > > > > >> #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.
> > >
> > > The comment is invalid, we load sudo search base in
> > > ldap_get_sudo_options().
> > > > > > >> #0007:
> > > > > > >> line 534: is the TODO still valid?
> > > > > > >
> > > > > > > No. I love removing TODOs :)
> > >
> > > I used the first patch from "SUDO Integration - periodical update
> > > thread".
> > > > > > >> 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.
> > >
> > > If you mean line 461 in #0008 then I believe it is done in
> > > sudosrv_get_sudorules_parse_query().
> > > > > > >> 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.
> > > > >
> > > > > We want the client API to be as small as possible because it will be
> > > > > moved to sudo. SAFEALIGN_* macros are defined in util.h so I decided to
> > > > > use memcpy() directly.
> > > >
> > > > Ok, please disregard my previous comment. I suppose that SAFEALIGN_*
> > > > macros
> > > > won't be subjects to change in the foreseeable future. Please
> > > > consider at
> > > > least a comment in the code that this way is safe because it is the
> > > > same what
> > > > SAFEALIGN_* does.
> > >
> > > Thank you for the review.
> > >
> > > Patch addressing these issues is attached.
> > > It also includes:
> > > sudo configuration options added to SSSDConfig.py
> > > attrs[] in sysdb_sudo_purge_subdir() is now terminated with NULL
> > > in sudosrv_get_sudorules_parse_query() I use strnlen() instead of
> > >
> > > strlen() to check validity of the input
> > >
> > > Ticket for this patch is https://fedorahosted.org/sssd/ticket/1125
> >
> > I'm sending it one more time with the options included in
> > config/etc/sssd.api.d/sssd-ldap.conf
>
> I forgot to put sudo_provider option to sssd.api.conf... I hope this is 
> finally everything :-)

Sorry, I forgot to note in my original email that this addresses
https://fedorahosted.org/sssd/ticket/1138 and I attached patches for
both master and sssd-1-5 (they differ only by the debug level
specifications)

I have not directly tested these patches, but I have asked the original
reporter of the bug to do so and provided him with a scratch build. I
will include the results of those tests when they are available.

[Stephen Gallagher]

On Fri, 2012-01-13 at 09:54 -0500, Stephen Gallagher wrote:
> On Fri, 2012-01-13 at 10:00 +0100, Pavel Březina wrote:
> > Dne 12.1.2012 12:42, Pavel Březina napsal(a):
> > > Dne 12.1.2012 12:17, Pavel Březina napsal(a):
> > > > Dne 5.1.2012 08:30, Jan Zeleny napsal(a):
> > > > > > >>> #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.
> > > > > > >
> > > > > > >>> #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.
> > > > > > >
> > > > > > >>> #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'
> > > >
> > > > Actually there should be EOK. At least the other be handlers returns EOK
> > > > in any case.
> > > >
> > > > If it returns value different than EOK it will send some generic message
> > > > back to the responder. This way I can specify format of the message.
> > > > > > >>> #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.
> > > >
> > > > The comment is invalid, we load sudo search base in
> > > > ldap_get_sudo_options().
> > > > > > >>> #0007:
> > > > > > >>> line 534: is the TODO still valid?
> > > > > > >>
> > > > > > >> No. I love removing TODOs :)
> > > >
> > > > I used the first patch from "SUDO Integration - periodical update
> > > > thread".
> > > > > > >>> 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.
> > > >
> > > > If you mean line 461 in #0008 then I believe it is done in
> > > > sudosrv_get_sudorules_parse_query().
> > > > > > >>> 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.
> > > > > >
> > > > > > We want the client API to be as small as possible because it will be
> > > > > > moved to sudo. SAFEALIGN_* macros are defined in util.h so I decided to
> > > > > > use memcpy() directly.
> > > > >
> > > > > Ok, please disregard my previous comment. I suppose that SAFEALIGN_*
> > > > > macros
> > > > > won't be subjects to change in the foreseeable future. Please
> > > > > consider at
> > > > > least a comment in the code that this way is safe because it is the
> > > > > same what
> > > > > SAFEALIGN_* does.
> > > >
> > > > Thank you for the review.
> > > >
> > > > Patch addressing these issues is attached.
> > > > It also includes:
> > > > sudo configuration options added to SSSDConfig.py
> > > > attrs[] in sysdb_sudo_purge_subdir() is now terminated with NULL
> > > > in sudosrv_get_sudorules_parse_query() I use strnlen() instead of
> > > >
> > > > strlen() to check validity of the input
> > > >
> > > > Ticket for this patch is https://fedorahosted.org/sssd/ticket/1125
> > >
> > > I'm sending it one more time with the options included in
> > > config/etc/sssd.api.d/sssd-ldap.conf
> >
> > I forgot to put sudo_provider option to sssd.api.conf... I hope this is 
> > finally everything :-)
>
> Sorry, I forgot to note in my original email that this addresses
> https://fedorahosted.org/sssd/ticket/1138 and I attached patches for
> both master and sssd-1-5 (they differ only by the debug level
> specifications)
>
> I have not directly tested these patches, but I have asked the original
> reporter of the bug to do so and provided him with a scratch build. I
> will include the results of those tests when they are available.

Please disregard. I seem to have replied to the wrong email...