8:46 a.m.
https://fedorahosted.org/sssd/ticket/1239
[PATCH 1/2]
Finally removes EOK constant from sudo api header. It is not used in
the SUDO code so it does not require their changes.
[PATCH 2/2]
This does what is requested in the ticket. It seems to be very huge but
in fact it is mainly changing the variable. Basically I tried to get
rid of domain ctx where possible, leave it only in initgroups part and
use command ctx elsewhere.
The in-memory cache is not yet implemented, I want to discuss the
possible ways of doing it.
The basic problem is that we need to get the domain during the request
for default options. How will we do it? I think there are two options:
1. always try to perform the initgroups - find the domain and the check
the in-memory cache (which may be slow if the user is in the last
domain, but that will be probably handled as part of
https://fedorahosted.org/sssd/ticket/1126
2. store uid:username = domain in the in-memory cache (same cache as
results or a new one?)
This patch contains a modified version of sysdb_get_sudo_user_info()
where the uid is not mandatory. I want to replace this function with
sysdb_sudo_get_user_groups() (or make it generic and place it in
sysdb_ops?) because the groupnames are the only thing we don't know.
However this requires a modification of the data provider protocol as
well so I'm keeping it for later.
2:47 p.m.
On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1239
[PATCH 1/2]
Finally removes EOK constant from sudo api header. It is not used in
the SUDO code so it does not require their changes.
Looks good to me, it'w weird to use EOK in an external header (even
though it is correctly defined).
(Note: this patch shouldn't be pushed even though I don't have any
comments. It is an ABI break and we need to coordinate with Daniel K.)
[PATCH 2/2]
This does what is requested in the ticket. It seems to be very huge but
in fact it is mainly changing the variable. Basically I tried to get
rid of domain ctx where possible, leave it only in initgroups part and
use command ctx elsewhere.
Still, it is hard to review the huge patch. Can you split it into
smaller ones? What about creating one that removes the duplication
between _get_sudorules and _get_defaults, one that converts to using
cmdctx and one that adds the uid support?
What is the rationale for removing the dctx? It would seem logical to me
to continue using it, the user is found in a particular domain anyway..
sudosrv_cmd: we should free cmd_ctx if retreiving cmd_ctx->sudo_ctx
fails (I know the problem was there before)
sudosrv_get_user: can you add a DEBUG() message when the UID number does
not match?
You shouldn't add add _GNU_SOURCE to sss_sudo.c, it is part of config.h
in SSSD (see AC_GNU_SOURCE)
While we are touching sss_sudo_create_query, can you use
safealign_memcpy? It does virtually the same as your code, but would be
more error prone when/if we extend the protocol again.
The in-memory cache is not yet implemented, I want to discuss the
possible ways of doing it.
The basic problem is that we need to get the domain during the request
for default options. How will we do it? I think there are two options:
1. always try to perform the initgroups - find the domain and the
check the in-memory cache (which may be slow if the user is in the
last domain, but that will be probably handled as part of
https://fedorahosted.org/sssd/ticket/1126
I'm not sure if this is what are you proposing in 1), but when I look
at sudo_sssd_display_defaults() in the sudo tree, I see that it knows
the user we are performing sudo on behalf on already. So can we extend
it to send username/uid and perform initgroups during the "get default
options operation?". I don't think there is much slowness here, as you
said, ticket #1226 would bring some improvement and with the current
model we need to search all the domains anyway. Also, only the first
request would perform searches, the other would be caught by the
in-memory cache, right?
2. store uid:username = domain in the in-memory cache (same cache as
results or a new one?)
Sorry, I don't get this, can you explain?
This patch contains a modified version of sysdb_get_sudo_user_info()
where the uid is not mandatory. I want to replace this function with
sysdb_sudo_get_user_groups() (or make it generic and place it in
sysdb_ops?) because the groupnames are the only thing we don't know.
However this requires a modification of the data provider protocol
as well so I'm keeping it for later.
5:35 p.m.
On Mon, 2012-03-26 at 15:47 -0400, Jakub Hrozek wrote:
On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/1239
>
> [PATCH 1/2]
> Finally removes EOK constant from sudo api header. It is not used in
> the SUDO code so it does not require their changes.
>
Looks good to me, it'w weird to use EOK in an external header (even
though it is correctly defined).
(Note: this patch shouldn't be pushed even though I don't have any
comments. It is an ABI break and we need to coordinate with Daniel K.)
I do not see any ABI change in the first patch, what are you referring
to ?
> [PATCH 2/2]
> This does what is requested in the ticket. It seems to be very huge but
> in fact it is mainly changing the variable. Basically I tried to get
> rid of domain ctx where possible, leave it only in initgroups part and
> use command ctx elsewhere.
>
Still, it is hard to review the huge patch. Can you split it into
smaller ones? What about creating one that removes the duplication
between _get_sudorules and _get_defaults, one that converts to using
cmdctx and one that adds the uid support?
What is the rationale for removing the dctx? It would seem logical to me
to continue using it, the user is found in a particular domain anyway..
sudosrv_cmd: we should free cmd_ctx if retreiving cmd_ctx->sudo_ctx
fails (I know the problem was there before)
sudosrv_get_user: can you add a DEBUG() message when the UID number does
not match?
You shouldn't add add _GNU_SOURCE to sss_sudo.c, it is part of config.h
in SSSD (see AC_GNU_SOURCE)
While we are touching sss_sudo_create_query, can you use
safealign_memcpy? It does virtually the same as your code, but would be
more error prone when/if we extend the protocol again.
> The in-memory cache is not yet implemented, I want to discuss the
> possible ways of doing it.
>
> The basic problem is that we need to get the domain during the request
> for default options. How will we do it? I think there are two options:
>
> 1. always try to perform the initgroups - find the domain and the
> check the in-memory cache (which may be slow if the user is in the
> last domain, but that will be probably handled as part of
> https://fedorahosted.org/sssd/ticket/1126
>
I'm not sure if this is what are you proposing in 1), but when I look
at sudo_sssd_display_defaults() in the sudo tree, I see that it knows
the user we are performing sudo on behalf on already. So can we extend
it to send username/uid and perform initgroups during the "get default
options operation?". I don't think there is much slowness here, as you
said, ticket #1226 would bring some improvement and with the current
model we need to search all the domains anyway. Also, only the first
request would perform searches, the other would be caught by the
in-memory cache, right?
> 2. store uid:username = domain in the in-memory cache (same cache as
> results or a new one?)
>
May I ask what 'in-memory' cache are we talking about ?
I would like to understand why looking up into LDB is not considered a
cache look-up.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
9 p.m.
On Mon, Mar 26, 2012 at 06:35:08PM -0400, Simo Sorce wrote:
On Mon, 2012-03-26 at 15:47 -0400, Jakub Hrozek wrote:
> On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:
> > https://fedorahosted.org/sssd/ticket/1239
> >
> > [PATCH 1/2]
> > Finally removes EOK constant from sudo api header. It is not used in
> > the SUDO code so it does not require their changes.
> >
>
> Looks good to me, it'w weird to use EOK in an external header (even
> though it is correctly defined).
>
> (Note: this patch shouldn't be pushed even though I don't have any
> comments. It is an ABI break and we need to coordinate with Daniel K.)
I do not see any ABI change in the first patch, what are you referring
to ?
I misread, I thought Pavel also changed a definiton of a function.
Please ignore me.
> > [PATCH 2/2]
> > This does what is requested in the ticket. It seems to be very huge but
> > in fact it is mainly changing the variable. Basically I tried to get
> > rid of domain ctx where possible, leave it only in initgroups part and
> > use command ctx elsewhere.
> >
>
> Still, it is hard to review the huge patch. Can you split it into
> smaller ones? What about creating one that removes the duplication
> between _get_sudorules and _get_defaults, one that converts to using
> cmdctx and one that adds the uid support?
>
> What is the rationale for removing the dctx? It would seem logical to me
> to continue using it, the user is found in a particular domain anyway..
>
> sudosrv_cmd: we should free cmd_ctx if retreiving cmd_ctx->sudo_ctx
> fails (I know the problem was there before)
>
> sudosrv_get_user: can you add a DEBUG() message when the UID number does
> not match?
>
> You shouldn't add add _GNU_SOURCE to sss_sudo.c, it is part of config.h
> in SSSD (see AC_GNU_SOURCE)
>
> While we are touching sss_sudo_create_query, can you use
> safealign_memcpy? It does virtually the same as your code, but would be
> more error prone when/if we extend the protocol again.
>
> > The in-memory cache is not yet implemented, I want to discuss the
> > possible ways of doing it.
> >
> > The basic problem is that we need to get the domain during the request
> > for default options. How will we do it? I think there are two options:
> >
> > 1. always try to perform the initgroups - find the domain and the
> > check the in-memory cache (which may be slow if the user is in the
> > last domain, but that will be probably handled as part of
> > https://fedorahosted.org/sssd/ticket/1126
> >
>
> I'm not sure if this is what are you proposing in 1), but when I look
> at sudo_sssd_display_defaults() in the sudo tree, I see that it knows
> the user we are performing sudo on behalf on already. So can we extend
> it to send username/uid and perform initgroups during the "get default
> options operation?". I don't think there is much slowness here, as you
> said, ticket #1226 would bring some improvement and with the current
> model we need to search all the domains anyway. Also, only the first
> request would perform searches, the other would be caught by the
> in-memory cache, right?
>
> > 2. store uid:username = domain in the in-memory cache (same cache as
> > results or a new one?)
> >
May I ask what 'in-memory' cache are we talking about ?
I would like to understand why looking up into LDB is not considered a
cache look-up.
Simo.
This cache works much like the in-memory cache in the PAM responder that
caches initgroups results and the actual sudo rules. It's a short-lived
cache, 180 seconds by default.
In general, we want to always go to the remote directory and look up the
sudoers rules online to make sure we are not allowing something we
shouldn't be. At the same time, it's quite common to run a couple of
sudo commands in quick succession, so we implemented this cache.
Here's the original design mini-thread:
https://fedorahosted.org/pipermail/sssd-devel/2011-December/007712.html
3:05 a.m.
On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/1239
>
> [PATCH 1/2]
> Finally removes EOK constant from sudo api header. It is not used in
> the SUDO code so it does not require their changes.
>
Looks good to me, it'w weird to use EOK in an external header (even
though it is correctly defined).
(Note: this patch shouldn't be pushed even though I don't have any
comments. It is an ABI break and we need to coordinate with Daniel K.)
> [PATCH 2/2]
> This does what is requested in the ticket. It seems to be very huge but
> in fact it is mainly changing the variable. Basically I tried to get
> rid of domain ctx where possible, leave it only in initgroups part and
> use command ctx elsewhere.
>
Still, it is hard to review the huge patch. Can you split it into
smaller ones? What about creating one that removes the duplication
between _get_sudorules and _get_defaults, one that converts to using
cmdctx and one that adds the uid support?
I'll try.
What is the rationale for removing the dctx? It would seem logical to
me
to continue using it, the user is found in a particular domain anyway..
1. It makes sudosrv_cmd_send_error() and sudosrv_cmd_done() unified and
error handling is a little bit easier (if dctx is NULL).
2. Some information were taken from cmd_ctx some from dctx and I found
it confusing when adding new fields to structure - where should I put
it? It semantically belongs to both? So I decided to use only one of
them and leave dctx only for initgroups part (which will be removed in
#1126).
sudosrv_cmd: we should free cmd_ctx if retreiving
cmd_ctx->sudo_ctx
fails (I know the problem was there before)
sudosrv_get_user: can you add a DEBUG() message when the UID number does
not match?
You shouldn't add add _GNU_SOURCE to sss_sudo.c, it is part of config.h
in SSSD (see AC_GNU_SOURCE)
While we are touching sss_sudo_create_query, can you use
safealign_memcpy? It does virtually the same as your code, but would be
more error prone when/if we extend the protocol again.
> The in-memory cache is not yet implemented, I want to discuss the
> possible ways of doing it.
>
> The basic problem is that we need to get the domain during the request
> for default options. How will we do it? I think there are two options:
>
> 1. always try to perform the initgroups - find the domain and the
> check the in-memory cache (which may be slow if the user is in the
> last domain, but that will be probably handled as part of
> https://fedorahosted.org/sssd/ticket/1126
>
I'm not sure if this is what are you proposing in 1), but when I look
at sudo_sssd_display_defaults() in the sudo tree, I see that it knows
the user we are performing sudo on behalf on already. So can we extend
it to send username/uid and perform initgroups during the "get default
options operation?". I don't think there is much slowness here, as you
said, ticket #1226 would bring some improvement and with the current
model we need to search all the domains anyway. Also, only the first
request would perform searches, the other would be caught by the
in-memory cache, right?
> 2. store uid:username = domain in the in-memory cache (same cache as
> results or a new one?)
>
Sorry, I don't get this, can you explain?
Sudo sends us username and uid in sudo_sssd_display_defaults() - yes
(that's what this patch does :-)).
I will try to rephrase the problem first:
Phase 1) sudo requests for default option giving us username and uid
- (search in-memory cache)*
- responder finds the domain user is a member of and performs initgroups
- provider will download the cn=defaults from LDAP and stores them in
sysdb
- responder fetches the entry
- stores it into in-memory cache with key "$domain"
- and returns it to the sudo with the domain name
*Current master does it the wrong way (more info in the ticket
description). In this patch it is the problematical part.
Phase 2) sudo requests rules for username@domain with uid
- search in-memory cache, if found then return else continue
- provider will download the rules from LDAP and stores them in sysdb
- responder fetches the entry
- stores it into in-memory cache with key "$domain:$username"
- and returns it to the sudo
All the entries are basically considered expired immediately, that's why
we are going into LDAP everytime. But it is also slow so we have created
the in-memory cache.
The problem here is that we have to know the domain before we can start
searching in the in-memory cache. This is fine with Phase 2 (sudo sends
us this information), but make problems in Phase 1.
We can do this in two ways in Phase 1 (Phase 2 stays intact):
1.
- find the domain user is a member of and perform initgroups
- search in-memory cache for default options
- if found then return else continue
- provider, ...
Advantage - a little bit lesser memory consumption
Disadvantage - more time spend in initgroups if it is in the last domain
2. store the pair (user,domainname) in the in-memory cache as well
- find domain name in in-memory cache
- if not found then perform initgroups, go into provider, ...
- find default options in in-memory cache
- if not found then go into provider, ...
Advantage - faster, easier to implement
Disadvantage - bigger memory dump if the sudo is used by many users
I hope that this is understandable enough.
> This patch contains a modified version of
sysdb_get_sudo_user_info()
> where the uid is not mandatory. I want to replace this function with
> sysdb_sudo_get_user_groups() (or make it generic and place it in
> sysdb_ops?) because the groupnames are the only thing we don't know.
> However this requires a modification of the data provider protocol
> as well so I'm keeping it for later.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
9:09 a.m.
On Tue, Mar 27, 2012 at 10:05:04AM +0200, Pavel Březina wrote:
2. store the pair (user,domainname) in the in-memory cache as well
- find domain name in in-memory cache
- if not found then perform initgroups, go into provider, ...
- find default options in in-memory cache
- if not found then go into provider, ...
Advantage - faster, easier to implement
Disadvantage - bigger memory dump if the sudo is used by many users
I hope that this is understandable enough.
The cache places a timer on the cached data, so the memory is only
occupied for a short and defined period of time. I think the advantages
outweight the slightly bigger memory consumption, so I would prefer this
option.
10:33 a.m.
On Wed, 2012-03-28 at 10:09 -0400, Jakub Hrozek wrote:
On Tue, Mar 27, 2012 at 10:05:04AM +0200, Pavel Březina wrote:
> 2. store the pair (user,domainname) in the in-memory cache as well
> - find domain name in in-memory cache
> - if not found then perform initgroups, go into provider, ...
>
> - find default options in in-memory cache
> - if not found then go into provider, ...
>
> Advantage - faster, easier to implement
> Disadvantage - bigger memory dump if the sudo is used by many users
>
> I hope that this is understandable enough.
>
The cache places a timer on the cached data, so the memory is only
occupied for a short and defined period of time. I think the advantages
outweight the slightly bigger memory consumption, so I would prefer this
option.
Why this data is not simply stored in LDB ?
Simo.
--
Simo Sorce * Red Hat, Inc * New York
6:23 a.m.
On 28.3.2012 17:33, Simo Sorce wrote:
On Wed, 2012-03-28 at 10:09 -0400, Jakub Hrozek wrote:
> On Tue, Mar 27, 2012 at 10:05:04AM +0200, Pavel Březina wrote:
>> 2. store the pair (user,domainname) in the in-memory cache as well
>> - find domain name in in-memory cache
>> - if not found then perform initgroups, go into provider, ...
>>
>> - find default options in in-memory cache
>> - if not found then go into provider, ...
>>
>> Advantage - faster, easier to implement
>> Disadvantage - bigger memory dump if the sudo is used by many users
>>
>> I hope that this is understandable enough.
>>
>
> The cache places a timer on the cached data, so the memory is only
> occupied for a short and defined period of time. I think the advantages
> outweight the slightly bigger memory consumption, so I would prefer this
> option.
Why this data is not simply stored in LDB ?
Simo.
I'm sorry, what data do you mean? The (user,domain) pair?
8:34 a.m.
On Fri, Mar 30, 2012 at 01:23:10PM +0200, Pavel Březina wrote:
On 28.3.2012 17:33, Simo Sorce wrote:
>On Wed, 2012-03-28 at 10:09 -0400, Jakub Hrozek wrote:
>>On Tue, Mar 27, 2012 at 10:05:04AM +0200, Pavel Březina wrote:
>>>2. store the pair (user,domainname) in the in-memory cache as well
>>>- find domain name in in-memory cache
>>>- if not found then perform initgroups, go into provider, ...
>>>
>>>- find default options in in-memory cache
>>>- if not found then go into provider, ...
>>>
>>>Advantage - faster, easier to implement
>>>Disadvantage - bigger memory dump if the sudo is used by many users
>>>
>>>I hope that this is understandable enough.
>>>
>>
>>The cache places a timer on the cached data, so the memory is only
>>occupied for a short and defined period of time. I think the advantages
>>outweight the slightly bigger memory consumption, so I would prefer this
>>option.
>
>Why this data is not simply stored in LDB ?
>
>Simo.
>
I'm sorry, what data do you mean? The (user,domain) pair?
We talked with Simo recently about abandoning the memory cache completely
in favor of just performing LDB lookups(*) in order to avoid another cache
level. Sorry, I just didn't catch you online yesterday to explain. I'll
send you the IRC log in an e-mail.
(*) unless the performance regresses drastically
3:19 p.m.
On 03/30/2012 09:34 AM, Jakub Hrozek wrote:
On Fri, Mar 30, 2012 at 01:23:10PM +0200, Pavel Březina wrote:
> On 28.3.2012 17:33, Simo Sorce wrote:
>> On Wed, 2012-03-28 at 10:09 -0400, Jakub Hrozek wrote:
>>> On Tue, Mar 27, 2012 at 10:05:04AM +0200, Pavel Březina wrote:
>>>> 2. store the pair (user,domainname) in the in-memory cache as well
>>>> - find domain name in in-memory cache
>>>> - if not found then perform initgroups, go into provider, ...
>>>>
>>>> - find default options in in-memory cache
>>>> - if not found then go into provider, ...
>>>>
>>>> Advantage - faster, easier to implement
>>>> Disadvantage - bigger memory dump if the sudo is used by many users
>>>>
>>>> I hope that this is understandable enough.
>>>>
>>> The cache places a timer on the cached data, so the memory is only
>>> occupied for a short and defined period of time. I think the advantages
>>> outweight the slightly bigger memory consumption, so I would prefer this
>>> option.
>> Why this data is not simply stored in LDB ?
>>
>> Simo.
>>
> I'm sorry, what data do you mean? The (user,domain) pair?
We talked with Simo recently about abandoning the memory cache completely
Which memory cache you are referring to here?
in favor of just performing LDB lookups(*) in order to avoid another
cache
level. Sorry, I just didn't catch you online yesterday to explain. I'll
send you the IRC log in an e-mail.
(*) unless the performance regresses drastically
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
7:48 p.m.
On Mon, Apr 02, 2012 at 04:19:52PM -0400, Dmitri Pal wrote:
On 03/30/2012 09:34 AM, Jakub Hrozek wrote:
> On Fri, Mar 30, 2012 at 01:23:10PM +0200, Pavel Březina wrote:
>> On 28.3.2012 17:33, Simo Sorce wrote:
>>> On Wed, 2012-03-28 at 10:09 -0400, Jakub Hrozek wrote:
>>>> On Tue, Mar 27, 2012 at 10:05:04AM +0200, Pavel Březina wrote:
>>>>> 2. store the pair (user,domainname) in the in-memory cache as well
>>>>> - find domain name in in-memory cache
>>>>> - if not found then perform initgroups, go into provider, ...
>>>>>
>>>>> - find default options in in-memory cache
>>>>> - if not found then go into provider, ...
>>>>>
>>>>> Advantage - faster, easier to implement
>>>>> Disadvantage - bigger memory dump if the sudo is used by many users
>>>>>
>>>>> I hope that this is understandable enough.
>>>>>
>>>> The cache places a timer on the cached data, so the memory is only
>>>> occupied for a short and defined period of time. I think the advantages
>>>> outweight the slightly bigger memory consumption, so I would prefer
this
>>>> option.
>>> Why this data is not simply stored in LDB ?
>>>
>>> Simo.
>>>
>> I'm sorry, what data do you mean? The (user,domain) pair?
> We talked with Simo recently about abandoning the memory cache completely
Which memory cache you are referring to here?
It is a different memory cache than what Simo wrote (sorry, memory cache
seems to be an overloaded term..). The sudo responder caches the set of
sudo rules for a user in a short-lived entry in a hash table.
Simo didn't like this another level of cache and suggested we always
perform sysdb lookups. Pavel, who actually wrote the sudo cache is now
looking into whether the removal would cause any issues (we do some
post-processing of the rules after fetching them from sysdb).
12:52 p.m.
First of all I have never thought of the in-memory cache as a
performance issue (we are only sorting them), but as a nice way to
handle rules expiration.
I will try to explain why the sysdb is not enough in this case.
Because it is a security issue, when user runs SUDO, we want to always
go to an LDAP server and download current rules - not all of them, just
the ones that apply to him to minimize the traffic.
But running SUDO multiple times in a short period of time is not an
exception, therefore we cannot afford this approach because it would be
too. It would make SUDO unusable.
Therefore we need to implement expiration mechanism so we can reuse the
rules in a short period (currently set to 180 seconds). The question is
*how*?
1. *Download every time all rules instead of per user*
This would be probably too costly to do, similar to users and groups
enumeration. There is already implemented an option that enables
periodical update of all rules.
I admit I have never seen an enterprise sudoers. I assume that it
can get very big with hundreds of rules. If this is not a valid
assumption, just tell me that I am stupid and we can do it this
way :-)
2. *Store the rules in sysdb per user*
There would be many data duplication and the database may get very
huge.
3. Store the expiration time with each rule
*update only those rules that are expired*
We wouldn't be able to decide whether a new rule was created without
downloading all rules that apply for the user.
(And we might be delivering to SUDO half of the updated rules and
half of the old rules.)
4. Store the expiration time with each rule
*if one rule has different timestamp than others, perform an update*
Here we may end up in a not complete set of rules and a race
condition when the cache will not be used at all. Details are below
(*).
5. *Storing set of rules per user in in-memory cache* (current approach)
Once the rules that apply for the user are downloaded, they are
stored in the sysdb for the offline times but they are also stored
in a hash table. We set a tevent timer that will remove them from
the hash table after the expiration time exceeds.
We will refresh the rules in case of in-memory cache miss.
There is a possible duplication of the rules in the memory, but only
for a short time and for a small amount of users (I don't expect
many administrators to be using sudo at one moment).
6. If there is some approach I have not thought of, please tell me.
=======================================================================
(*)
**Rules in LDAP**
cn=rule1
sudoUser: A
cn=rule2
sudoUser: B
cn=rule3
sudoUser: A
sudoUser: B
**Timeline**
*Timestamp: 0*
user A does: sudo ls
Downloaded rules: rule1 and rule3
Sysdb contains:
cn=rule1
expire: 180
cn=rule3
expire: 180
*Timestamp: 10*
user B does: sudo ls
rule1 and rule3 are found in sysdb
expiration time equals
return them
!!! rule2 is missing !!!
*Timestamp: 190*
user B does: sudo ls
rule3 in sysdb is expired
perform an LDAP search for user B
sysdb contains:
cn=rule1
expire: 180
cn=rule2
expire: 370
cn=rule3
expire: 370
*Timestamp: 200*
user A does: sudo ls
rule1 and rule3 have different expiration time
perform an LDAP search for user A
sysdb contains:
cn=rule1
expire: 380
cn=rule2
expire: 370
cn=rule3
expire: 380
*Timestamp: 210*
user B does: sudo ls
rule2 and rule3 have different expiration time
perform an LDAP search for user B
sysdb contains:
cn=rule1
expire: 380
cn=rule2
expire: 390
cn=rule3
expire: 390
!!! always performing an LDAP search !!!
Regards,
Pavel.
6:52 p.m.
On Wed, 2012-04-04 at 19:52 +0200, Pavel Březina wrote:
First of all I have never thought of the in-memory cache as a
performance issue (we are only sorting them), but as a nice way to
handle rules expiration.
I will try to explain why the sysdb is not enough in this case.
Because it is a security issue,
Ok Pavel, stop right there.
Explain exactly what would be the security issue and why sysdb implies
it while a second in-memory cache wouldn't.
when user runs SUDO, we want to always
go to an LDAP server and download current rules - not all of them, just
the ones that apply to him to minimize the traffic.
No we do not always want to, that's not necessarily the case, I can well
see admins decide that updating every X minutes is good enough, it's a
balance between performance and freshness of rules.
But running SUDO multiple times in a short period of time is not an
exception, therefore we cannot afford this approach because it would be
too. It would make SUDO unusable.
Correct, a period of time within which sudo rules are considered valid
is quite ok.
Therefore we need to implement expiration mechanism so we can reuse
the
rules in a short period (currently set to 180 seconds).
Why do we need a new expiration mechanism ? In sysdb we already store
"expiration time" for the entries normally. At least we do that for
users/groups. If it is not done for sudo rules it is an error, as you
wouldn't be able to asses rules validity on sssd process restart.
The question is *how*?
1. *Download every time all rules instead of per user*
This would be probably too costly to do, similar to users and groups
enumeration. There is already implemented an option that enables
periodical update of all rules.
I admit I have never seen an enterprise sudoers. I assume that it
can get very big with hundreds of rules. If this is not a valid
assumption, just tell me that I am stupid and we can do it this
way :-)
sudo rules can get very large indeed, whether it is ok to enumerate them
or not is a different thing. Enumerating a few hudreds of rules is not a
big deal.
For users it is different because we expect up to hundreds of thousands
users/groups.
Perhaps we should have a settable threshold under which we do enumerate
frequently, over which we automatically stop doing that unless forced
to.
2. *Store the rules in sysdb per user*
There would be many data duplication and the database may get very
huge.
Why per-user ? Doesn't make sense to me.
3. Store the expiration time with each rule
*update only those rules that are expired*
I totally expect this to be done already, if it is not done I consider
it a bug.
We wouldn't be able to decide whether a new rule was created
without
downloading all rules that apply for the user.
This assumption and logic is incorrect.
The assumption is incorrect because we already do smart updates for
users/groups enumeration by downloading only rules that were changed
since the last time they were enumerated. So you can easily download
only newer rules by using the changetime of the newest rule you have in
sysdb.
But here the logic seem also incorrect. The main issue I see is not much
in finding if new rules have been added. But in invalidating rules that
have been removed. However this could also be done in a smart way. First
refresh only for new rules. Then apply rule matching. For the handful of
rules that match the sudo request you check how old they are. For each
rule older than X minutes you do active validation by trying to fetch
them individually. This way you refresh only the matching rules.
If any rules is changed/missing you update/remove it from the evaluation
set.
(And we might be delivering to SUDO half of the updated rules
and
half of the old rules.)
I do not see how this would happen.
4. Store the expiration time with each rule
*if one rule has different timestamp than others, perform an update*
I do not think this makes much sense, you want to be able to update only
individual rules, always fetching all rules that apply to a user seem a
waste of bandwidth and time spent waiting by the user.
Here we may end up in a not complete set of rules and a race
condition when the cache will not be used at all. Details are below
(*).
5. *Storing set of rules per user in in-memory cache* (current approach)
Once the rules that apply for the user are downloaded, they are
stored in the sysdb for the offline times but they are also stored
in a hash table. We set a tevent timer that will remove them from
the hash table after the expiration time exceeds.
What's the point here ?
You can achieve this exaclty same result by setting an expiration date
on each sysdb entry.
We will refresh the rules in case of in-memory cache miss.
You can do the same with sysdb rules, just consider any expired one as
missing.
There is a possible duplication of the rules in the memory, but
only
for a short time and for a small amount of users (I don't expect
many administrators to be using sudo at one moment).
I wouldn't count on such assumptions, however I do not see how a
duplicated in-memory cache is helping here.
You can do exactly the same operations with exactly the same effects
using sysdb entries, so why is it not done that way ?
6. If there is some approach I have not thought of, please tell me.
See above.
=======================================================================
(*)
**Rules in LDAP**
cn=rule1
sudoUser: A
cn=rule2
sudoUser: B
cn=rule3
sudoUser: A
sudoUser: B
**Timeline**
*Timestamp: 0*
user A does: sudo ls
Downloaded rules: rule1 and rule3
Sysdb contains:
cn=rule1
expire: 180
cn=rule3
expire: 180
*Timestamp: 10*
user B does: sudo ls
rule1 and rule3 are found in sysdb
expiration time equals
return them
!!! rule2 is missing !!!
*Timestamp: 190*
user B does: sudo ls
rule3 in sysdb is expired
perform an LDAP search for user B
sysdb contains:
cn=rule1
expire: 180
cn=rule2
expire: 370
cn=rule3
expire: 370
*Timestamp: 200*
user A does: sudo ls
rule1 and rule3 have different expiration time
perform an LDAP search for user A
sysdb contains:
cn=rule1
expire: 380
cn=rule2
expire: 370
cn=rule3
expire: 380
*Timestamp: 210*
user B does: sudo ls
rule2 and rule3 have different expiration time
perform an LDAP search for user B
sysdb contains:
cn=rule1
expire: 380
cn=rule2
expire: 390
cn=rule3
expire: 390
!!! always performing an LDAP search !!!
The error seem to me in trying to download rules per-user, I do not
think you can ever attain decent performances this way, and it will
always be fraught with errors. For example if sssd goes off line right
after the first step, then userB will always miss rules. This is not
expected, very difficult to explain to admins, and, frankly probably
unacceptable as it is very non-deterministic.
What you really want to do is to enumerate sudo rules that apply to the
machine. You effectively want to cache them all if possible and have a
reasonable tunable expiration time (5/10/15 minutes ...).
The reason is quite simple, sudo rules change very rarely, but you want
them all even when you are offline. You do not want to have a normal
user not have the sudo rules for his laptop handy only because he rarely
needs them.
Assume the case where sudo is used to run some VPN software only when
user X is out of the office. Now user X never used the VPN before and
therefore never needed to run sudo. Because you never downloaded rules
he goes home, opens the machine, runs the command to bring up the VPN
and ... it doesn't work.
I think the nature of sudo requires you to download all the rules that
apply to a specific machine. If we have a concern about the number of
rules, the admin can easily cut down the rules by properly restricting
the set of rule used on specific machines.
But in general I do not think it will be a big deal, I do not expect to
see cases where multiple thousands of rules that apply to the same host
are used. It would be unmanageable anyways.
So I think that the problem in the end is the approach where you decided
to download rules per-user instead of per-machine, it leads you into all
sort of corner cases.
The in-memory cache at this point is just a red-herring but I am glad I
dug in here as it uncovered a bigger design issue I didn't notice
before.
The in-memory cache keeps being an additional layer that should simply
be avoided, and becomes completely useless if you download rules
per-machine instead of per-user.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
8:57 a.m.
The error seem to me in trying to download rules per-user, I do not
think you can ever attain decent performances this way, and it will
always be fraught with errors. For example if sssd goes off line right
after the first step, then userB will always miss rules. This is not
expected, very difficult to explain to admins, and, frankly probably
unacceptable as it is very non-deterministic.
What you really want to do is to enumerate sudo rules that apply to the
machine.
Yes, of course we want it. The problem is that sudoHost attribute may
contain a *regular expression*. If we want to support this feature, we
have to download the rules per user.
On 04/05/2012 01:52 AM, Simo Sorce wrote:
On Wed, 2012-04-04 at 19:52 +0200, Pavel Březina wrote:
> First of all I have never thought of the in-memory cache as a
> performance issue (we are only sorting them), but as a nice way to
> handle rules expiration.
>
> I will try to explain why the sysdb is not enough in this case.
>
> Because it is a security issue,
Ok Pavel, stop right there.
Explain exactly what would be the security issue and why sysdb implies
it while a second in-memory cache wouldn't.
The security issue is that we do not want to allow the user to run any
command that he is not permitted to (when the rule is removed from LDAP
but still stored in a cache).
I'm not saying that the in-memory cache does not imply it - I did not
mention in-memory cache till the end of the message. I'm saying that we
would like to do it (go every time to the LDAP) but it is not
acceptable (with neighter sysdb or in-memory cache).
> when user runs SUDO, we want to always
> go to an LDAP server and download current rules - not all of them, just
> the ones that apply to him to minimize the traffic.
No we do not always want to, that's not necessarily the case, I can well
see admins decide that updating every X minutes is good enough, it's a
balance between performance and freshness of rules.
> But running SUDO multiple times in a short period of time is not an
> exception, therefore we cannot afford this approach because it would be
> too. It would make SUDO unusable.
Correct, a period of time within which sudo rules are considered valid
is quite ok.
> Therefore we need to implement expiration mechanism so we can reuse the
> rules in a short period (currently set to 180 seconds).
Why do we need a new expiration mechanism ? In sysdb we already store
"expiration time" for the entries normally. At least we do that for
users/groups. If it is not done for sudo rules it is an error, as you
wouldn't be able to asses rules validity on sssd process restart.
> The question is *how*?
>
> 1. *Download every time all rules instead of per user*
> This would be probably too costly to do, similar to users and groups
> enumeration. There is already implemented an option that enables
> periodical update of all rules.
>
> I admit I have never seen an enterprise sudoers. I assume that it
> can get very big with hundreds of rules. If this is not a valid
> assumption, just tell me that I am stupid and we can do it this
> way :-)
sudo rules can get very large indeed, whether it is ok to enumerate them
or not is a different thing. Enumerating a few hudreds of rules is not a
big deal.
For users it is different because we expect up to hundreds of thousands
users/groups.
Perhaps we should have a settable threshold under which we do enumerate
frequently, over which we automatically stop doing that unless forced
to.
> 2. *Store the rules in sysdb per user*
> There would be many data duplication and the database may get very
> huge.
Why per-user ? Doesn't make sense to me.
> 3. Store the expiration time with each rule
> *update only those rules that are expired*
I totally expect this to be done already, if it is not done I consider
it a bug.
> We wouldn't be able to decide whether a new rule was created without
> downloading all rules that apply for the user.
This assumption and logic is incorrect.
The assumption is incorrect because we already do smart updates for
users/groups enumeration by downloading only rules that were changed
since the last time they were enumerated. So you can easily download
only newer rules by using the changetime of the newest rule you have in
sysdb.
OK, I did not know about entryUSN.
But here the logic seem also incorrect. The main issue I see is not
much
in finding if new rules have been added. But in invalidating rules that
have been removed. However this could also be done in a smart way. First
refresh only for new rules. Then apply rule matching. For the handful of
rules that match the sudo request you check how old they are. For each
rule older than X minutes you do active validation by trying to fetch
them individually. This way you refresh only the matching rules.
If any rules is changed/missing you update/remove it from the evaluation
set.
I think I see the problem now. We have a different opinion on how it
should behave.
a) My primary goal is to avoid any LDAP query during the expiration
period. In my opinion, the last thing you want to do when using sudo is
to wait for an LDAP query to finish. When I was testing it on my
machine it was driving me crazy.
b) I also want to return the *same* set of rules to the user for the
expiration period.
Knowing about entryUSN I agree that the updating process should be done
in this "smart ways". However if you will agree with me on a) and (or)
b), it still requires additional information that is not suited to be
stored in the sysdb because it is not required to be persistent.
>> (And we might be delivering to SUDO half of the updated rules and
>> half of the old rules.)
>
> I do not see how this would happen.
>
>> 4. Store the expiration time with each rule
>> *if one rule has different timestamp than others, perform an update*
>
> I do not think this makes much sense, you want to be able to update only
> individual rules, always fetching all rules that apply to a user seem a
> waste of bandwidth and time spent waiting by the user.
>
>> Here we may end up in a not complete set of rules and a race
>> condition when the cache will not be used at all. Details are below
>> (*).
>>
>> 5. *Storing set of rules per user in in-memory cache* (current approach)
>>
>> Once the rules that apply for the user are downloaded, they are
>> stored in the sysdb for the offline times but they are also stored
>> in a hash table. We set a tevent timer that will remove them from
>> the hash table after the expiration time exceeds.
>
> What's the point here ?
> You can achieve this exaclty same result by setting an expiration date
> on each sysdb entry.
>
>> We will refresh the rules in case of in-memory cache miss.
>
> You can do the same with sysdb rules, just consider any expired one as
> missing.
>
>> There is a possible duplication of the rules in the memory, but only
>> for a short time and for a small amount of users (I don't expect
>> many administrators to be using sudo at one moment).
>
> I wouldn't count on such assumptions, however I do not see how a
> duplicated in-memory cache is helping here.
> You can do exactly the same operations with exactly the same effects
> using sysdb entries, so why is it not done that way ?
>
>> 6. If there is some approach I have not thought of, please tell me.
>
> See above.
>
>> =======================================================================
>> (*)
>>
>> **Rules in LDAP**
>>
>> cn=rule1
>> sudoUser: A
>>
>> cn=rule2
>> sudoUser: B
>>
>> cn=rule3
>> sudoUser: A
>> sudoUser: B
>>
>> **Timeline**
>>
>> *Timestamp: 0*
>> user A does: sudo ls
>>
>> Downloaded rules: rule1 and rule3
>> Sysdb contains:
>>
>> cn=rule1
>> expire: 180
>>
>> cn=rule3
>> expire: 180
>>
>> *Timestamp: 10*
>> user B does: sudo ls
>>
>> rule1 and rule3 are found in sysdb
>> expiration time equals
>> return them
>>
>> !!! rule2 is missing !!!
>>
>> *Timestamp: 190*
>> user B does: sudo ls
>>
>> rule3 in sysdb is expired
>> perform an LDAP search for user B
>> sysdb contains:
>>
>> cn=rule1
>> expire: 180
>>
>> cn=rule2
>> expire: 370
>>
>> cn=rule3
>> expire: 370
>>
>> *Timestamp: 200*
>> user A does: sudo ls
>>
>> rule1 and rule3 have different expiration time
>> perform an LDAP search for user A
>> sysdb contains:
>>
>> cn=rule1
>> expire: 380
>>
>> cn=rule2
>> expire: 370
>>
>> cn=rule3
>> expire: 380
>>
>> *Timestamp: 210*
>> user B does: sudo ls
>>
>> rule2 and rule3 have different expiration time
>> perform an LDAP search for user B
>> sysdb contains:
>>
>> cn=rule1
>> expire: 380
>>
>> cn=rule2
>> expire: 390
>>
>> cn=rule3
>> expire: 390
>>
>> !!! always performing an LDAP search !!!
>
The error seem to me in trying to download rules per-user, I do not
think you can ever attain decent performances this way, and it will
always be fraught with errors. For example if sssd goes off line right
after the first step, then userB will always miss rules. This is not
expected, very difficult to explain to admins, and, frankly probably
unacceptable as it is very non-deterministic.
What you really want to do is to enumerate sudo rules that apply to the
machine.
Yes, of course we want it. The problem is that sudoHost attribute may
contain a *regular expression*. If we want to support this feature, we
have to download the rules by user.
You effectively want to cache them all if possible and have a
reasonable tunable expiration time (5/10/15 minutes ...).
The reason is quite simple, sudo rules change very rarely, but you want
them all even when you are offline. You do not want to have a normal
user not have the sudo rules for his laptop handy only because he rarely
needs them.
Assume the case where sudo is used to run some VPN software only when
user X is out of the office. Now user X never used the VPN before and
therefore never needed to run sudo. Because you never downloaded rules
he goes home, opens the machine, runs the command to bring up the VPN
and ... it doesn't work.
I think the nature of sudo requires you to download all the rules that
apply to a specific machine. If we have a concern about the number of
rules, the admin can easily cut down the rules by properly restricting
the set of rule used on specific machines.
But in general I do not think it will be a big deal, I do not expect to
see cases where multiple thousands of rules that apply to the same host
are used. It would be unmanageable anyways.
So I think that the problem in the end is the approach where you decided
to download rules per-user instead of per-machine, it leads you into all
sort of corner cases.
The in-memory cache at this point is just a red-herring but I am glad I
dug in here as it uncovered a bigger design issue I didn't notice
before.
The in-memory cache keeps being an additional layer that should simply
be avoided, and becomes completely useless if you download rules
per-machine instead of per-user.
Simo.
11:19 a.m.
This is what simo and I made up in the last few days:
https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
I tried to go to the details so we can avoid further problems.
Please, review it and tell us what is not good or clear enough.
Thanks,
Pavel.
12:29 p.m.
On 04/17/2012 12:19 PM, Pavel Březina wrote:
This is what simo and I made up in the last few days:
https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
I tried to go to the details so we can avoid further problems.
Please, review it and tell us what is not good or clear enough.
Thanks,
Pavel.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
Should we also do per user smart updates when the user runs /sudo/?
Might be costly but probably should be an option. May be when user logs in.
Should we create a tool to force full refresh of the rules immediately?
Yes, eventually, but this can be deferred.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
12:37 p.m.
On Tue, 2012-04-17 at 13:29 -0400, Dmitri Pal wrote:
On 04/17/2012 12:19 PM, Pavel Březina wrote:
> This is what simo and I made up in the last few days:
>
> https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
>
> I tried to go to the details so we can avoid further problems.
> Please, review it and tell us what is not good or clear enough.
>
> Thanks,
> Pavel.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
Should we also do per user smart updates when the user runs sudo?
I had that point in the flow, but we decided to pull it out and defer it
for the first go, and do a simpler implementation.
Might be costly but probably should be an option. May be when user
logs in.
The way to do it was planned to be with storing the last smart update
time in the user account, and if both this timestamp and the global last
update timestamp were older than a threshold (5 min?) then we would do a
smart update as step 1 in B.
Should we create a tool to force full refresh of the rules
immediately?
Yes, eventually, but this can be deferred.
For testing purposes a command to force a full refresh now is very
valuable as well as for admins, so we should have it as part of the
first implementation.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
1:20 p.m.
On Tue, 2012-04-17 at 13:37 -0400, Simo Sorce wrote:
On Tue, 2012-04-17 at 13:29 -0400, Dmitri Pal wrote:
> On 04/17/2012 12:19 PM, Pavel Březina wrote:
> > This is what simo and I made up in the last few days:
> >
> > https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
> >
> > I tried to go to the details so we can avoid further problems.
> > Please, review it and tell us what is not good or clear enough.
> >
> > Thanks,
> > Pavel.
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel(a)lists.fedorahosted.org
> > https://fedorahosted.org/mailman/listinfo/sssd-devel
>
> Should we also do per user smart updates when the user runs sudo?
I had that point in the flow, but we decided to pull it out and defer it
for the first go, and do a simpler implementation.
> Might be costly but probably should be an option. May be when user
> logs in.
The way to do it was planned to be with storing the last smart update
time in the user account, and if both this timestamp and the global last
update timestamp were older than a threshold (5 min?) then we would do a
smart update as step 1 in B.
This was the first version with the user smart refresh workflow btw:
https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules?version=1
> Should we create a tool to force full refresh of the rules
> immediately?
>
> Yes, eventually, but this can be deferred.
For testing purposes a command to force a full refresh now is very
valuable as well as for admins, so we should have it as part of the
first implementation.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
5:46 a.m.
On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/1239
>
> [PATCH 1/2]
> Finally removes EOK constant from sudo api header. It is not used in
> the SUDO code so it does not require their changes.
>
Looks good to me, it'w weird to use EOK in an external header (even
though it is correctly defined).
(Note: this patch shouldn't be pushed even though I don't have any
comments. It is an ABI break and we need to coordinate with Daniel K.)
> [PATCH 2/2]
> This does what is requested in the ticket. It seems to be very huge but
> in fact it is mainly changing the variable. Basically I tried to get
> rid of domain ctx where possible, leave it only in initgroups part and
> use command ctx elsewhere.
>
Still, it is hard to review the huge patch. Can you split it into
smaller ones? What about creating one that removes the duplication
between _get_sudorules and _get_defaults, one that converts to using
cmdctx and one that adds the uid support?
The patches are attached.
I want to get those patches acked because I need to deliver the updated
interface to Dan so he can update the sudo binary.
It does not implement the in-memory cache (yet? :-)) due to the
discussion with simo.
The patches expects "sudo api: check sss_status instead of errnop in
sss_sudo_send_recv_generic()" (already acked on the list and waiting to
be pushed).
3:03 a.m.
On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel Březina wrote:
On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
>On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:
>>https://fedorahosted.org/sssd/ticket/1239
>>
>>[PATCH 1/2]
>>Finally removes EOK constant from sudo api header. It is not used in
>>the SUDO code so it does not require their changes.
>>
>
>Looks good to me, it'w weird to use EOK in an external header (even
>though it is correctly defined).
>
>(Note: this patch shouldn't be pushed even though I don't have any
>comments. It is an ABI break and we need to coordinate with Daniel K.)
>
>>[PATCH 2/2]
>>This does what is requested in the ticket. It seems to be very huge but
>>in fact it is mainly changing the variable. Basically I tried to get
>>rid of domain ctx where possible, leave it only in initgroups part and
>>use command ctx elsewhere.
>>
>
>Still, it is hard to review the huge patch. Can you split it into
>smaller ones? What about creating one that removes the duplication
>between _get_sudorules and _get_defaults, one that converts to using
>cmdctx and one that adds the uid support?
The patches are attached.
I want to get those patches acked because I need to deliver the updated
interface to Dan so he can update the sudo binary.
It does not implement the in-memory cache (yet? :-)) due to the
discussion with simo.
The patches expects "sudo api: check sss_status instead of errnop in
sss_sudo_send_recv_generic()" (already acked on the list and waiting
to be pushed).
Patch 0001: sudo api: remove EOK
Ack
Patch 0002: sudo responder: remove code duplication in commands
I think it would be a good idea to also check if rawname[rawname_len] ==
'\0' when parsing username, this would ensure that all the string checks
work OK.
Now that several sudosrv_response_append_* functions were removed from
header, can you make them static? In particular
sudosrv_response_append_string(), sudosrv_response_append_uint32(),
sudosrv_response_append_rule(), sudosrv_response_append_attr().
Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info more configurable
Ack
Patch 0004: sudo api: send uid, username and domainname
Please get rid of the C++ comments in sudosrv_cmd(). If you need to
leave code in but disabled, please use #if 0/#endif The whole hunk that
changes sudosrv_cmd() seems like it belongs to patch #2.
There's another // comment in sudosrv_get_sudorules_from_cache.
The logic looks good to me, though.
Even when the patches are acked, they shouldn't be pushed right?
5:51 a.m.
On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel Březina wrote:
> On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
>> On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:
>>> https://fedorahosted.org/sssd/ticket/1239
>>>
>>> [PATCH 1/2]
>>> Finally removes EOK constant from sudo api header. It is not used in
>>> the SUDO code so it does not require their changes.
>>>
>>
>> Looks good to me, it'w weird to use EOK in an external header (even
>> though it is correctly defined).
>>
>> (Note: this patch shouldn't be pushed even though I don't have any
>> comments. It is an ABI break and we need to coordinate with Daniel K.)
>>
>>> [PATCH 2/2]
>>> This does what is requested in the ticket. It seems to be very huge but
>>> in fact it is mainly changing the variable. Basically I tried to get
>>> rid of domain ctx where possible, leave it only in initgroups part and
>>> use command ctx elsewhere.
>>>
>>
>> Still, it is hard to review the huge patch. Can you split it into
>> smaller ones? What about creating one that removes the duplication
>> between _get_sudorules and _get_defaults, one that converts to using
>> cmdctx and one that adds the uid support?
>
> The patches are attached.
>
> I want to get those patches acked because I need to deliver the updated
> interface to Dan so he can update the sudo binary.
>
> It does not implement the in-memory cache (yet? :-)) due to the
> discussion with simo.
>
> The patches expects "sudo api: check sss_status instead of errnop in
> sss_sudo_send_recv_generic()" (already acked on the list and waiting
> to be pushed).
Patch 0001: sudo api: remove EOK
Ack
Patch 0002: sudo responder: remove code duplication in commands
I think it would be a good idea to also check if rawname[rawname_len] ==
'\0' when parsing username, this would ensure that all the string checks
work OK.
Done.
Now that several sudosrv_response_append_* functions were removed from
header, can you make them static? In particular
sudosrv_response_append_string(), sudosrv_response_append_uint32(),
sudosrv_response_append_rule(), sudosrv_response_append_attr().
Done.
Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info more
configurable
Ack
Patch 0004: sudo api: send uid, username and domainname
Please get rid of the C++ comments in sudosrv_cmd(). If you need to
leave code in but disabled, please use #if 0/#endif
Thanks. I'm using keybord shortcut to comment out whole block with
these slashes and I fortgot that we have this rule :)
The whole hunk that
changes sudosrv_cmd() seems like it belongs to patch #2.
Not really. It requires the new protocol which is implemented in this
patch. It makes the two command more similar than they was before.
There's another // comment in sudosrv_get_sudorules_from_cache.
The logic looks good to me, though.
Thank you for the review.
I'm attaching a 6th patch that removes all code related to the
in-memory cache as it will not be used anymore.
Even when the patches are acked, they shouldn't be pushed right?
The patches work so they can be pushed IMHO. And I will use them as a
starting point for the new design. [1]
The only problem is that Daniel did not find the time yet to change the
sudo sources. So we have no consumer that would understand the new
protocol.
[1] https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
10:05 a.m.
The subdomains patches messed with one line which was causing troubles.
Here are the rebased patches.
On 04/23/2012 12:51 PM, Pavel Březina wrote:
On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
> On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel Březina wrote:
>> On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
>>> On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:
>>>> https://fedorahosted.org/sssd/ticket/1239
>>>>
>>>> [PATCH 1/2]
>>>> Finally removes EOK constant from sudo api header. It is not used in
>>>> the SUDO code so it does not require their changes.
>>>>
>>>
>>> Looks good to me, it'w weird to use EOK in an external header (even
>>> though it is correctly defined).
>>>
>>> (Note: this patch shouldn't be pushed even though I don't have any
>>> comments. It is an ABI break and we need to coordinate with Daniel K.)
>>>
>>>> [PATCH 2/2]
>>>> This does what is requested in the ticket. It seems to be very huge
>>>> but
>>>> in fact it is mainly changing the variable. Basically I tried to get
>>>> rid of domain ctx where possible, leave it only in initgroups part and
>>>> use command ctx elsewhere.
>>>>
>>>
>>> Still, it is hard to review the huge patch. Can you split it into
>>> smaller ones? What about creating one that removes the duplication
>>> between _get_sudorules and _get_defaults, one that converts to using
>>> cmdctx and one that adds the uid support?
>>
>> The patches are attached.
>>
>> I want to get those patches acked because I need to deliver the updated
>> interface to Dan so he can update the sudo binary.
>>
>> It does not implement the in-memory cache (yet? :-)) due to the
>> discussion with simo.
>>
>> The patches expects "sudo api: check sss_status instead of errnop in
>> sss_sudo_send_recv_generic()" (already acked on the list and waiting
>> to be pushed).
>
> Patch 0001: sudo api: remove EOK
> Ack
>
> Patch 0002: sudo responder: remove code duplication in commands
> I think it would be a good idea to also check if rawname[rawname_len] ==
> '\0' when parsing username, this would ensure that all the string checks
> work OK.
Done.
>
> Now that several sudosrv_response_append_* functions were removed from
> header, can you make them static? In particular
> sudosrv_response_append_string(), sudosrv_response_append_uint32(),
> sudosrv_response_append_rule(), sudosrv_response_append_attr().
Done.
> Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info more configurable
> Ack
>
> Patch 0004: sudo api: send uid, username and domainname
> Please get rid of the C++ comments in sudosrv_cmd(). If you need to
> leave code in but disabled, please use #if 0/#endif
Thanks. I'm using keybord shortcut to comment out whole block with
these slashes and I fortgot that we have this rule :)
The whole hunk that
> changes sudosrv_cmd() seems like it belongs to patch #2.
Not really. It requires the new protocol which is implemented in this
patch. It makes the two command more similar than they was before.
> There's another // comment in sudosrv_get_sudorules_from_cache.
>
> The logic looks good to me, though.
Thank you for the review.
I'm attaching a 6th patch that removes all code related to the
in-memory cache as it will not be used anymore.
>
> Even when the patches are acked, they shouldn't be pushed right?
The patches work so they can be pushed IMHO. And I will use them as a
starting point for the new design. [1]
The only problem is that Daniel did not find the time yet to change the
sudo sources. So we have no consumer that would understand the new
protocol.
[1] https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
6:21 a.m.
On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel Březina wrote:
The subdomains patches messed with one line which was causing
troubles.
Here are the rebased patches.
On 04/23/2012 12:51 PM, Pavel Březina wrote:
>On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
>>On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel Březina wrote:
>>>On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
>>>>On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:
>>>>>https://fedorahosted.org/sssd/ticket/1239
>>>>>
>>>>>[PATCH 1/2]
>>>>>Finally removes EOK constant from sudo api header. It is not used in
>>>>>the SUDO code so it does not require their changes.
>>>>>
>>>>
>>>>Looks good to me, it'w weird to use EOK in an external header (even
>>>>though it is correctly defined).
>>>>
>>>>(Note: this patch shouldn't be pushed even though I don't have
any
>>>>comments. It is an ABI break and we need to coordinate with Daniel K.)
>>>>
>>>>>[PATCH 2/2]
>>>>>This does what is requested in the ticket. It seems to be very huge
>>>>>but
>>>>>in fact it is mainly changing the variable. Basically I tried to get
>>>>>rid of domain ctx where possible, leave it only in initgroups part
and
>>>>>use command ctx elsewhere.
>>>>>
>>>>
>>>>Still, it is hard to review the huge patch. Can you split it into
>>>>smaller ones? What about creating one that removes the duplication
>>>>between _get_sudorules and _get_defaults, one that converts to using
>>>>cmdctx and one that adds the uid support?
>>>
>>>The patches are attached.
>>>
>>>I want to get those patches acked because I need to deliver the updated
>>>interface to Dan so he can update the sudo binary.
>>>
>>>It does not implement the in-memory cache (yet? :-)) due to the
>>>discussion with simo.
>>>
>>>The patches expects "sudo api: check sss_status instead of errnop in
>>>sss_sudo_send_recv_generic()" (already acked on the list and waiting
>>>to be pushed).
>>
>>Patch 0001: sudo api: remove EOK
>>Ack
>>
>>Patch 0002: sudo responder: remove code duplication in commands
>>I think it would be a good idea to also check if rawname[rawname_len] ==
>>'\0' when parsing username, this would ensure that all the string checks
>>work OK.
>
>Done.
>
>>
>>Now that several sudosrv_response_append_* functions were removed from
>>header, can you make them static? In particular
>>sudosrv_response_append_string(), sudosrv_response_append_uint32(),
>>sudosrv_response_append_rule(), sudosrv_response_append_attr().
>
>Done.
>
>>Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info more configurable
>>Ack
>>
>>Patch 0004: sudo api: send uid, username and domainname
>>Please get rid of the C++ comments in sudosrv_cmd(). If you need to
>>leave code in but disabled, please use #if 0/#endif
>
>Thanks. I'm using keybord shortcut to comment out whole block with
>these slashes and I fortgot that we have this rule :)
>
>The whole hunk that
>>changes sudosrv_cmd() seems like it belongs to patch #2.
>
>Not really. It requires the new protocol which is implemented in this
>patch. It makes the two command more similar than they was before.
>
>>There's another // comment in sudosrv_get_sudorules_from_cache.
>>
>>The logic looks good to me, though.
>
>Thank you for the review.
>
>I'm attaching a 6th patch that removes all code related to the
>in-memory cache as it will not be used anymore.
>
Patch 0001: Ack
Patch 0002: Ack
Patch 0003: Ack
Patch 0004: Ack
Patch 0005: Ack
Patch 0006: Nack, does to apply on master:
$ git reset --hard origin/master
HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
$ git am *.patch
Applying: sudo api: remove EOK
Applying: sudo responder: remove code duplication in commands
Applying: sudo responder: get rid of dctx where possible
Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
Applying: sudo api: send uid, username and domainname
Applying: sudo responder: discard in-memory cache
error: patch failed: src/responder/sudo/sudosrv_cache.c:1
>>
>>Even when the patches are acked, they shouldn't be pushed right?
>
>The patches work so they can be pushed IMHO. And I will use them as a
>starting point for the new design. [1]
>
>The only problem is that Daniel did not find the time yet to change the
>sudo sources. So we have no consumer that would understand the new
>protocol.
No, that would break anyone that wants to use git HEAD with sudo that is
currently available. I think we have two options:
1) when the modified sudo binary is available, simply state that whoever
wants to use git HEAD with sudo, must use version xy of sudo. We
could also import sudo into our nightly repo for the time being.
2) Modify the code so that it accepts *both* protocols.
6:48 a.m.
On Fri, Apr 27, 2012 at 01:21:30PM +0200, Jakub Hrozek wrote:
On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel Březina wrote:
> The subdomains patches messed with one line which was causing troubles.
> Here are the rebased patches.
>
> On 04/23/2012 12:51 PM, Pavel Březina wrote:
> >On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
> >>On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel Březina wrote:
> >>>On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
> >>>>On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:
> >>>>>https://fedorahosted.org/sssd/ticket/1239
> >>>>>
> >>>>>[PATCH 1/2]
> >>>>>Finally removes EOK constant from sudo api header. It is not
used in
> >>>>>the SUDO code so it does not require their changes.
> >>>>>
> >>>>
> >>>>Looks good to me, it'w weird to use EOK in an external header
(even
> >>>>though it is correctly defined).
> >>>>
> >>>>(Note: this patch shouldn't be pushed even though I don't
have any
> >>>>comments. It is an ABI break and we need to coordinate with Daniel
K.)
> >>>>
> >>>>>[PATCH 2/2]
> >>>>>This does what is requested in the ticket. It seems to be very
huge
> >>>>>but
> >>>>>in fact it is mainly changing the variable. Basically I tried to
get
> >>>>>rid of domain ctx where possible, leave it only in initgroups
part and
> >>>>>use command ctx elsewhere.
> >>>>>
> >>>>
> >>>>Still, it is hard to review the huge patch. Can you split it into
> >>>>smaller ones? What about creating one that removes the duplication
> >>>>between _get_sudorules and _get_defaults, one that converts to
using
> >>>>cmdctx and one that adds the uid support?
> >>>
> >>>The patches are attached.
> >>>
> >>>I want to get those patches acked because I need to deliver the updated
> >>>interface to Dan so he can update the sudo binary.
> >>>
> >>>It does not implement the in-memory cache (yet? :-)) due to the
> >>>discussion with simo.
> >>>
> >>>The patches expects "sudo api: check sss_status instead of errnop
in
> >>>sss_sudo_send_recv_generic()" (already acked on the list and
waiting
> >>>to be pushed).
> >>
> >>Patch 0001: sudo api: remove EOK
> >>Ack
> >>
> >>Patch 0002: sudo responder: remove code duplication in commands
> >>I think it would be a good idea to also check if rawname[rawname_len] ==
> >>'\0' when parsing username, this would ensure that all the string
checks
> >>work OK.
> >
> >Done.
> >
> >>
> >>Now that several sudosrv_response_append_* functions were removed from
> >>header, can you make them static? In particular
> >>sudosrv_response_append_string(), sudosrv_response_append_uint32(),
> >>sudosrv_response_append_rule(), sudosrv_response_append_attr().
> >
> >Done.
> >
> >>Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info more configurable
> >>Ack
> >>
> >>Patch 0004: sudo api: send uid, username and domainname
> >>Please get rid of the C++ comments in sudosrv_cmd(). If you need to
> >>leave code in but disabled, please use #if 0/#endif
> >
> >Thanks. I'm using keybord shortcut to comment out whole block with
> >these slashes and I fortgot that we have this rule :)
> >
> >The whole hunk that
> >>changes sudosrv_cmd() seems like it belongs to patch #2.
> >
> >Not really. It requires the new protocol which is implemented in this
> >patch. It makes the two command more similar than they was before.
> >
> >>There's another // comment in sudosrv_get_sudorules_from_cache.
> >>
> >>The logic looks good to me, though.
> >
> >Thank you for the review.
> >
> >I'm attaching a 6th patch that removes all code related to the
> >in-memory cache as it will not be used anymore.
> >
Patch 0001: Ack
Patch 0002: Ack
Patch 0003: Ack
Patch 0004: Ack
Patch 0005: Ack
Patch 0006: Nack, does to apply on master:
$ git reset --hard origin/master
HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
$ git am *.patch
Applying: sudo api: remove EOK
Applying: sudo responder: remove code duplication in commands
Applying: sudo responder: get rid of dctx where possible
Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
Applying: sudo api: send uid, username and domainname
Applying: sudo responder: discard in-memory cache
error: patch failed: src/responder/sudo/sudosrv_cache.c:1
> >>
> >>Even when the patches are acked, they shouldn't be pushed right?
> >
> >The patches work so they can be pushed IMHO. And I will use them as a
> >starting point for the new design. [1]
> >
> >The only problem is that Daniel did not find the time yet to change the
> >sudo sources. So we have no consumer that would understand the new
> >protocol.
No, that would break anyone that wants to use git HEAD with sudo that is
currently available. I think we have two options:
1) when the modified sudo binary is available, simply state that whoever
wants to use git HEAD with sudo, must use version xy of sudo. We
could also import sudo into our nightly repo for the time being.
2) Modify the code so that it accepts *both* protocols.
2) is the strongly preferred solution, btw
3:30 p.m.
On 3.5.2012 13:48, Jakub Hrozek wrote:
On Fri, Apr 27, 2012 at 01:21:30PM +0200, Jakub Hrozek wrote:
> On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel Březina wrote:
>> The subdomains patches messed with one line which was causing troubles.
>> Here are the rebased patches.
>>
>> On 04/23/2012 12:51 PM, Pavel Březina wrote:
>>> On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
>>>> On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel Březina wrote:
>>>>> On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
>>>>>> On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:
>>>>>>> https://fedorahosted.org/sssd/ticket/1239
>>>>>>>
>>>>>>> [PATCH 1/2]
>>>>>>> Finally removes EOK constant from sudo api header. It is not
used in
>>>>>>> the SUDO code so it does not require their changes.
>>>>>>>
>>>>>>
>>>>>> Looks good to me, it'w weird to use EOK in an external header
(even
>>>>>> though it is correctly defined).
>>>>>>
>>>>>> (Note: this patch shouldn't be pushed even though I don't
have any
>>>>>> comments. It is an ABI break and we need to coordinate with
Daniel K.)
>>>>>>
>>>>>>> [PATCH 2/2]
>>>>>>> This does what is requested in the ticket. It seems to be
very huge
>>>>>>> but
>>>>>>> in fact it is mainly changing the variable. Basically I tried
to get
>>>>>>> rid of domain ctx where possible, leave it only in initgroups
part and
>>>>>>> use command ctx elsewhere.
>>>>>>>
>>>>>>
>>>>>> Still, it is hard to review the huge patch. Can you split it
into
>>>>>> smaller ones? What about creating one that removes the
duplication
>>>>>> between _get_sudorules and _get_defaults, one that converts to
using
>>>>>> cmdctx and one that adds the uid support?
>>>>>
>>>>> The patches are attached.
>>>>>
>>>>> I want to get those patches acked because I need to deliver the
updated
>>>>> interface to Dan so he can update the sudo binary.
>>>>>
>>>>> It does not implement the in-memory cache (yet? :-)) due to the
>>>>> discussion with simo.
>>>>>
>>>>> The patches expects "sudo api: check sss_status instead of
errnop in
>>>>> sss_sudo_send_recv_generic()" (already acked on the list and
waiting
>>>>> to be pushed).
>>>>
>>>> Patch 0001: sudo api: remove EOK
>>>> Ack
>>>>
>>>> Patch 0002: sudo responder: remove code duplication in commands
>>>> I think it would be a good idea to also check if rawname[rawname_len] ==
>>>> '\0' when parsing username, this would ensure that all the string
checks
>>>> work OK.
>>>
>>> Done.
>>>
>>>>
>>>> Now that several sudosrv_response_append_* functions were removed from
>>>> header, can you make them static? In particular
>>>> sudosrv_response_append_string(), sudosrv_response_append_uint32(),
>>>> sudosrv_response_append_rule(), sudosrv_response_append_attr().
>>>
>>> Done.
>>>
>>>> Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info more configurable
>>>> Ack
>>>>
>>>> Patch 0004: sudo api: send uid, username and domainname
>>>> Please get rid of the C++ comments in sudosrv_cmd(). If you need to
>>>> leave code in but disabled, please use #if 0/#endif
>>>
>>> Thanks. I'm using keybord shortcut to comment out whole block with
>>> these slashes and I fortgot that we have this rule :)
>>>
>>> The whole hunk that
>>>> changes sudosrv_cmd() seems like it belongs to patch #2.
>>>
>>> Not really. It requires the new protocol which is implemented in this
>>> patch. It makes the two command more similar than they was before.
>>>
>>>> There's another // comment in sudosrv_get_sudorules_from_cache.
>>>>
>>>> The logic looks good to me, though.
>>>
>>> Thank you for the review.
>>>
>>> I'm attaching a 6th patch that removes all code related to the
>>> in-memory cache as it will not be used anymore.
>>>
>
> Patch 0001: Ack
> Patch 0002: Ack
> Patch 0003: Ack
> Patch 0004: Ack
> Patch 0005: Ack
> Patch 0006: Nack, does to apply on master:
>
> $ git reset --hard origin/master
> HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
> $ git am *.patch
> Applying: sudo api: remove EOK
> Applying: sudo responder: remove code duplication in commands
> Applying: sudo responder: get rid of dctx where possible
> Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
> Applying: sudo api: send uid, username and domainname
> Applying: sudo responder: discard in-memory cache
> error: patch failed: src/responder/sudo/sudosrv_cache.c:1
I'm sorry, but it works for me. Are you sure you have the correct
patches? What exactly fails to merge?
$ git reset --hard origin/master
HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
$ git am *.patch
Applying: sudo api: remove EOK
Applying: sudo responder: remove code duplication in commands
Applying: sudo responder: get rid of dctx where possible
Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
Applying: sudo api: send uid, username and domainname
Applying: sudo responder: discard in-memory cache
$
>>>> Even when the patches are acked, they shouldn't
be pushed right?
>>>
>>> The patches work so they can be pushed IMHO. And I will use them as a
>>> starting point for the new design. [1]
>>>
>>> The only problem is that Daniel did not find the time yet to change the
>>> sudo sources. So we have no consumer that would understand the new
>>> protocol.
>
> No, that would break anyone that wants to use git HEAD with sudo that is
> currently available. I think we have two options:
> 1) when the modified sudo binary is available, simply state that whoever
> wants to use git HEAD with sudo, must use version xy of sudo. We
> could also import sudo into our nightly repo for the time being.
> 2) Modify the code so that it accepts *both* protocols.
2) is the strongly preferred solution, btw
It would be possible to put the logic back. Though I wouldn't do that
in this case because this is not an extension or a fix of a simple bug.
The old protocol may mix the results from two different domains in a
multi-domain environment, thus it may be better to force the update of
the sudo.
Pavel.
12:17 p.m.
On 05/03/2012 10:30 PM, Pavel Březina wrote:
On 3.5.2012 13:48, Jakub Hrozek wrote:
> On Fri, Apr 27, 2012 at 01:21:30PM +0200, Jakub Hrozek wrote:
>> On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel Březina wrote:
>>> The subdomains patches messed with one line which was causing troubles.
>>> Here are the rebased patches.
>>>
>>> On 04/23/2012 12:51 PM, Pavel Březina wrote:
>>>> On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
>>>>> On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel Březina wrote:
>>>>>> On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
>>>>>>> On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina
wrote:
>>>>>>>> https://fedorahosted.org/sssd/ticket/1239
>>>>>>>>
>>>>>>>> [PATCH 1/2]
>>>>>>>> Finally removes EOK constant from sudo api header. It is
not
>>>>>>>> used in
>>>>>>>> the SUDO code so it does not require their changes.
>>>>>>>>
>>>>>>>
>>>>>>> Looks good to me, it'w weird to use EOK in an external
header (even
>>>>>>> though it is correctly defined).
>>>>>>>
>>>>>>> (Note: this patch shouldn't be pushed even though I
don't have any
>>>>>>> comments. It is an ABI break and we need to coordinate with
>>>>>>> Daniel K.)
>>>>>>>
>>>>>>>> [PATCH 2/2]
>>>>>>>> This does what is requested in the ticket. It seems to be
very
>>>>>>>> huge
>>>>>>>> but
>>>>>>>> in fact it is mainly changing the variable. Basically I
tried
>>>>>>>> to get
>>>>>>>> rid of domain ctx where possible, leave it only in
initgroups
>>>>>>>> part and
>>>>>>>> use command ctx elsewhere.
>>>>>>>>
>>>>>>>
>>>>>>> Still, it is hard to review the huge patch. Can you split it
into
>>>>>>> smaller ones? What about creating one that removes the
duplication
>>>>>>> between _get_sudorules and _get_defaults, one that converts
to
>>>>>>> using
>>>>>>> cmdctx and one that adds the uid support?
>>>>>>
>>>>>> The patches are attached.
>>>>>>
>>>>>> I want to get those patches acked because I need to deliver the
>>>>>> updated
>>>>>> interface to Dan so he can update the sudo binary.
>>>>>>
>>>>>> It does not implement the in-memory cache (yet? :-)) due to the
>>>>>> discussion with simo.
>>>>>>
>>>>>> The patches expects "sudo api: check sss_status instead of
errnop in
>>>>>> sss_sudo_send_recv_generic()" (already acked on the list and
waiting
>>>>>> to be pushed).
>>>>>
>>>>> Patch 0001: sudo api: remove EOK
>>>>> Ack
>>>>>
>>>>> Patch 0002: sudo responder: remove code duplication in commands
>>>>> I think it would be a good idea to also check if
>>>>> rawname[rawname_len] ==
>>>>> '\0' when parsing username, this would ensure that all the
string
>>>>> checks
>>>>> work OK.
>>>>
>>>> Done.
>>>>
>>>>>
>>>>> Now that several sudosrv_response_append_* functions were removed
>>>>> from
>>>>> header, can you make them static? In particular
>>>>> sudosrv_response_append_string(), sudosrv_response_append_uint32(),
>>>>> sudosrv_response_append_rule(), sudosrv_response_append_attr().
>>>>
>>>> Done.
>>>>
>>>>> Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info more
>>>>> configurable
>>>>> Ack
>>>>>
>>>>> Patch 0004: sudo api: send uid, username and domainname
>>>>> Please get rid of the C++ comments in sudosrv_cmd(). If you need to
>>>>> leave code in but disabled, please use #if 0/#endif
>>>>
>>>> Thanks. I'm using keybord shortcut to comment out whole block with
>>>> these slashes and I fortgot that we have this rule :)
>>>>
>>>> The whole hunk that
>>>>> changes sudosrv_cmd() seems like it belongs to patch #2.
>>>>
>>>> Not really. It requires the new protocol which is implemented in this
>>>> patch. It makes the two command more similar than they was before.
>>>>
>>>>> There's another // comment in sudosrv_get_sudorules_from_cache.
>>>>>
>>>>> The logic looks good to me, though.
>>>>
>>>> Thank you for the review.
>>>>
>>>> I'm attaching a 6th patch that removes all code related to the
>>>> in-memory cache as it will not be used anymore.
>>>>
>>
>> Patch 0001: Ack
>> Patch 0002: Ack
>> Patch 0003: Ack
>> Patch 0004: Ack
>> Patch 0005: Ack
>> Patch 0006: Nack, does to apply on master:
>>
>> $ git reset --hard origin/master
>> HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
>> $ git am *.patch
>> Applying: sudo api: remove EOK
>> Applying: sudo responder: remove code duplication in commands
>> Applying: sudo responder: get rid of dctx where possible
>> Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
>> Applying: sudo api: send uid, username and domainname
>> Applying: sudo responder: discard in-memory cache
>> error: patch failed: src/responder/sudo/sudosrv_cache.c:1
I'm sorry, but it works for me. Are you sure you have the correct
patches? What exactly fails to merge?
$ git reset --hard origin/master
HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
$ git am *.patch
Applying: sudo api: remove EOK
Applying: sudo responder: remove code duplication in commands
Applying: sudo responder: get rid of dctx where possible
Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
Applying: sudo api: send uid, username and domainname
Applying: sudo responder: discard in-memory cache
$
I'm sending it again formatted via git format-patch -M -C --patience
--full-index and tarred.
5:23 a.m.
On 05/07/2012 07:17 PM, Pavel Březina wrote:
On 05/03/2012 10:30 PM, Pavel Březina wrote:
> On 3.5.2012 13:48, Jakub Hrozek wrote:
>> On Fri, Apr 27, 2012 at 01:21:30PM +0200, Jakub Hrozek wrote:
>>> On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel Březina wrote:
>>>> The subdomains patches messed with one line which was causing
>>>> troubles.
>>>> Here are the rebased patches.
>>>>
>>>> On 04/23/2012 12:51 PM, Pavel Březina wrote:
>>>>> On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
>>>>>> On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel Březina wrote:
>>>>>>> On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
>>>>>>>> On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina
wrote:
>>>>>>>>> https://fedorahosted.org/sssd/ticket/1239
>>>>>>>>>
>>>>>>>>> [PATCH 1/2]
>>>>>>>>> Finally removes EOK constant from sudo api header. It
is not
>>>>>>>>> used in
>>>>>>>>> the SUDO code so it does not require their changes.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Looks good to me, it'w weird to use EOK in an
external header
>>>>>>>> (even
>>>>>>>> though it is correctly defined).
>>>>>>>>
>>>>>>>> (Note: this patch shouldn't be pushed even though I
don't have any
>>>>>>>> comments. It is an ABI break and we need to coordinate
with
>>>>>>>> Daniel K.)
>>>>>>>>
>>>>>>>>> [PATCH 2/2]
>>>>>>>>> This does what is requested in the ticket. It seems
to be very
>>>>>>>>> huge
>>>>>>>>> but
>>>>>>>>> in fact it is mainly changing the variable. Basically
I tried
>>>>>>>>> to get
>>>>>>>>> rid of domain ctx where possible, leave it only in
initgroups
>>>>>>>>> part and
>>>>>>>>> use command ctx elsewhere.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Still, it is hard to review the huge patch. Can you split
it into
>>>>>>>> smaller ones? What about creating one that removes the
duplication
>>>>>>>> between _get_sudorules and _get_defaults, one that
converts to
>>>>>>>> using
>>>>>>>> cmdctx and one that adds the uid support?
>>>>>>>
>>>>>>> The patches are attached.
>>>>>>>
>>>>>>> I want to get those patches acked because I need to deliver
the
>>>>>>> updated
>>>>>>> interface to Dan so he can update the sudo binary.
>>>>>>>
>>>>>>> It does not implement the in-memory cache (yet? :-)) due to
the
>>>>>>> discussion with simo.
>>>>>>>
>>>>>>> The patches expects "sudo api: check sss_status instead
of
>>>>>>> errnop in
>>>>>>> sss_sudo_send_recv_generic()" (already acked on the list
and
>>>>>>> waiting
>>>>>>> to be pushed).
>>>>>>
>>>>>> Patch 0001: sudo api: remove EOK
>>>>>> Ack
>>>>>>
>>>>>> Patch 0002: sudo responder: remove code duplication in commands
>>>>>> I think it would be a good idea to also check if
>>>>>> rawname[rawname_len] ==
>>>>>> '\0' when parsing username, this would ensure that all
the string
>>>>>> checks
>>>>>> work OK.
>>>>>
>>>>> Done.
>>>>>
>>>>>>
>>>>>> Now that several sudosrv_response_append_* functions were
removed
>>>>>> from
>>>>>> header, can you make them static? In particular
>>>>>> sudosrv_response_append_string(),
sudosrv_response_append_uint32(),
>>>>>> sudosrv_response_append_rule(), sudosrv_response_append_attr().
>>>>>
>>>>> Done.
>>>>>
>>>>>> Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info more
>>>>>> configurable
>>>>>> Ack
>>>>>>
>>>>>> Patch 0004: sudo api: send uid, username and domainname
>>>>>> Please get rid of the C++ comments in sudosrv_cmd(). If you need
to
>>>>>> leave code in but disabled, please use #if 0/#endif
>>>>>
>>>>> Thanks. I'm using keybord shortcut to comment out whole block
with
>>>>> these slashes and I fortgot that we have this rule :)
>>>>>
>>>>> The whole hunk that
>>>>>> changes sudosrv_cmd() seems like it belongs to patch #2.
>>>>>
>>>>> Not really. It requires the new protocol which is implemented in
this
>>>>> patch. It makes the two command more similar than they was before.
>>>>>
>>>>>> There's another // comment in
sudosrv_get_sudorules_from_cache.
>>>>>>
>>>>>> The logic looks good to me, though.
>>>>>
>>>>> Thank you for the review.
>>>>>
>>>>> I'm attaching a 6th patch that removes all code related to the
>>>>> in-memory cache as it will not be used anymore.
>>>>>
>>>
>>> Patch 0001: Ack
>>> Patch 0002: Ack
>>> Patch 0003: Ack
>>> Patch 0004: Ack
>>> Patch 0005: Ack
>>> Patch 0006: Nack, does to apply on master:
>>>
>>> $ git reset --hard origin/master
>>> HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
>>> $ git am *.patch
>>> Applying: sudo api: remove EOK
>>> Applying: sudo responder: remove code duplication in commands
>>> Applying: sudo responder: get rid of dctx where possible
>>> Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
>>> Applying: sudo api: send uid, username and domainname
>>> Applying: sudo responder: discard in-memory cache
>>> error: patch failed: src/responder/sudo/sudosrv_cache.c:1
>
> I'm sorry, but it works for me. Are you sure you have the correct
> patches? What exactly fails to merge?
>
> $ git reset --hard origin/master
> HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
> $ git am *.patch
> Applying: sudo api: remove EOK
> Applying: sudo responder: remove code duplication in commands
> Applying: sudo responder: get rid of dctx where possible
> Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
> Applying: sudo api: send uid, username and domainname
> Applying: sudo responder: discard in-memory cache
> $
I'm sending it again formatted via git format-patch -M -C --patience
--full-index and tarred.
There was one big mistake in the fifth patch.
I was passing wrong uid information into query_cache function.
10:55 a.m.
On 11.5.2012 12:23, Pavel Březina wrote:
On 05/07/2012 07:17 PM, Pavel Březina wrote:
> On 05/03/2012 10:30 PM, Pavel Březina wrote:
>> On 3.5.2012 13:48, Jakub Hrozek wrote:
>>> On Fri, Apr 27, 2012 at 01:21:30PM +0200, Jakub Hrozek wrote:
>>>> On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel Březina wrote:
>>>>> The subdomains patches messed with one line which was causing
>>>>> troubles.
>>>>> Here are the rebased patches.
>>>>>
>>>>> On 04/23/2012 12:51 PM, Pavel Březina wrote:
>>>>>> On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
>>>>>>> On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel Březina
wrote:
>>>>>>>> On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
>>>>>>>>> On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel
Březina wrote:
>>>>>>>>>> https://fedorahosted.org/sssd/ticket/1239
>>>>>>>>>>
>>>>>>>>>> [PATCH 1/2]
>>>>>>>>>> Finally removes EOK constant from sudo api
header. It is not
>>>>>>>>>> used in
>>>>>>>>>> the SUDO code so it does not require their
changes.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Looks good to me, it'w weird to use EOK in an
external header
>>>>>>>>> (even
>>>>>>>>> though it is correctly defined).
>>>>>>>>>
>>>>>>>>> (Note: this patch shouldn't be pushed even though
I don't have
>>>>>>>>> any
>>>>>>>>> comments. It is an ABI break and we need to
coordinate with
>>>>>>>>> Daniel K.)
>>>>>>>>>
>>>>>>>>>> [PATCH 2/2]
>>>>>>>>>> This does what is requested in the ticket. It
seems to be very
>>>>>>>>>> huge
>>>>>>>>>> but
>>>>>>>>>> in fact it is mainly changing the variable.
Basically I tried
>>>>>>>>>> to get
>>>>>>>>>> rid of domain ctx where possible, leave it only
in initgroups
>>>>>>>>>> part and
>>>>>>>>>> use command ctx elsewhere.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Still, it is hard to review the huge patch. Can you
split it into
>>>>>>>>> smaller ones? What about creating one that removes
the
>>>>>>>>> duplication
>>>>>>>>> between _get_sudorules and _get_defaults, one that
converts to
>>>>>>>>> using
>>>>>>>>> cmdctx and one that adds the uid support?
>>>>>>>>
>>>>>>>> The patches are attached.
>>>>>>>>
>>>>>>>> I want to get those patches acked because I need to
deliver the
>>>>>>>> updated
>>>>>>>> interface to Dan so he can update the sudo binary.
>>>>>>>>
>>>>>>>> It does not implement the in-memory cache (yet? :-)) due
to the
>>>>>>>> discussion with simo.
>>>>>>>>
>>>>>>>> The patches expects "sudo api: check sss_status
instead of
>>>>>>>> errnop in
>>>>>>>> sss_sudo_send_recv_generic()" (already acked on the
list and
>>>>>>>> waiting
>>>>>>>> to be pushed).
>>>>>>>
>>>>>>> Patch 0001: sudo api: remove EOK
>>>>>>> Ack
>>>>>>>
>>>>>>> Patch 0002: sudo responder: remove code duplication in
commands
>>>>>>> I think it would be a good idea to also check if
>>>>>>> rawname[rawname_len] ==
>>>>>>> '\0' when parsing username, this would ensure that
all the string
>>>>>>> checks
>>>>>>> work OK.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>>
>>>>>>> Now that several sudosrv_response_append_* functions were
removed
>>>>>>> from
>>>>>>> header, can you make them static? In particular
>>>>>>> sudosrv_response_append_string(),
sudosrv_response_append_uint32(),
>>>>>>> sudosrv_response_append_rule(),
sudosrv_response_append_attr().
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>> Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info more
>>>>>>> configurable
>>>>>>> Ack
>>>>>>>
>>>>>>> Patch 0004: sudo api: send uid, username and domainname
>>>>>>> Please get rid of the C++ comments in sudosrv_cmd(). If you
need to
>>>>>>> leave code in but disabled, please use #if 0/#endif
>>>>>>
>>>>>> Thanks. I'm using keybord shortcut to comment out whole block
with
>>>>>> these slashes and I fortgot that we have this rule :)
>>>>>>
>>>>>> The whole hunk that
>>>>>>> changes sudosrv_cmd() seems like it belongs to patch #2.
>>>>>>
>>>>>> Not really. It requires the new protocol which is implemented in
>>>>>> this
>>>>>> patch. It makes the two command more similar than they was
before.
>>>>>>
>>>>>>> There's another // comment in
sudosrv_get_sudorules_from_cache.
>>>>>>>
>>>>>>> The logic looks good to me, though.
>>>>>>
>>>>>> Thank you for the review.
>>>>>>
>>>>>> I'm attaching a 6th patch that removes all code related to
the
>>>>>> in-memory cache as it will not be used anymore.
>>>>>>
>>>>
>>>> Patch 0001: Ack
>>>> Patch 0002: Ack
>>>> Patch 0003: Ack
>>>> Patch 0004: Ack
>>>> Patch 0005: Ack
>>>> Patch 0006: Nack, does to apply on master:
>>>>
>>>> $ git reset --hard origin/master
>>>> HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
>>>> $ git am *.patch
>>>> Applying: sudo api: remove EOK
>>>> Applying: sudo responder: remove code duplication in commands
>>>> Applying: sudo responder: get rid of dctx where possible
>>>> Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
>>>> Applying: sudo api: send uid, username and domainname
>>>> Applying: sudo responder: discard in-memory cache
>>>> error: patch failed: src/responder/sudo/sudosrv_cache.c:1
>>
>> I'm sorry, but it works for me. Are you sure you have the correct
>> patches? What exactly fails to merge?
>>
>> $ git reset --hard origin/master
>> HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
>> $ git am *.patch
>> Applying: sudo api: remove EOK
>> Applying: sudo responder: remove code duplication in commands
>> Applying: sudo responder: get rid of dctx where possible
>> Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
>> Applying: sudo api: send uid, username and domainname
>> Applying: sudo responder: discard in-memory cache
>> $
>
> I'm sending it again formatted via git format-patch -M -C --patience
> --full-index and tarred.
There was one big mistake in the fifth patch.
I was passing wrong uid information into query_cache function.
I have added a patch that increases protocol number. If client uses old
protocol, responder will return error immediately.
I know the sudo part is not yet ready. But I would like to get these
patches pushed - I don't want to rebase them again :) It does not break
sudo until sss is present in nsswitch.conf.
11:37 a.m.
On Mon, May 14, 2012 at 05:55:09PM +0200, Pavel Březina wrote:
On 11.5.2012 12:23, Pavel Březina wrote:
>On 05/07/2012 07:17 PM, Pavel Březina wrote:
>>On 05/03/2012 10:30 PM, Pavel Březina wrote:
>>>On 3.5.2012 13:48, Jakub Hrozek wrote:
>>>>On Fri, Apr 27, 2012 at 01:21:30PM +0200, Jakub Hrozek wrote:
>>>>>On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel Březina wrote:
>>>>>>The subdomains patches messed with one line which was causing
>>>>>>troubles.
>>>>>>Here are the rebased patches.
>>>>>>
>>>>>>On 04/23/2012 12:51 PM, Pavel Březina wrote:
>>>>>>>On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
>>>>>>>>On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel Březina
wrote:
>>>>>>>>>On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
>>>>>>>>>>On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel
Březina wrote:
>>>>>>>>>>>https://fedorahosted.org/sssd/ticket/1239
>>>>>>>>>>>
>>>>>>>>>>>[PATCH 1/2]
>>>>>>>>>>>Finally removes EOK constant from sudo api
header. It is not
>>>>>>>>>>>used in
>>>>>>>>>>>the SUDO code so it does not require their
changes.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>Looks good to me, it'w weird to use EOK in an
external header
>>>>>>>>>>(even
>>>>>>>>>>though it is correctly defined).
>>>>>>>>>>
>>>>>>>>>>(Note: this patch shouldn't be pushed even
though I don't have
>>>>>>>>>>any
>>>>>>>>>>comments. It is an ABI break and we need to
coordinate with
>>>>>>>>>>Daniel K.)
>>>>>>>>>>
>>>>>>>>>>>[PATCH 2/2]
>>>>>>>>>>>This does what is requested in the ticket. It
seems to be very
>>>>>>>>>>>huge
>>>>>>>>>>>but
>>>>>>>>>>>in fact it is mainly changing the variable.
Basically I tried
>>>>>>>>>>>to get
>>>>>>>>>>>rid of domain ctx where possible, leave it
only in initgroups
>>>>>>>>>>>part and
>>>>>>>>>>>use command ctx elsewhere.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>Still, it is hard to review the huge patch. Can
you split it into
>>>>>>>>>>smaller ones? What about creating one that
removes the
>>>>>>>>>>duplication
>>>>>>>>>>between _get_sudorules and _get_defaults, one
that converts to
>>>>>>>>>>using
>>>>>>>>>>cmdctx and one that adds the uid support?
>>>>>>>>>
>>>>>>>>>The patches are attached.
>>>>>>>>>
>>>>>>>>>I want to get those patches acked because I need to
deliver the
>>>>>>>>>updated
>>>>>>>>>interface to Dan so he can update the sudo binary.
>>>>>>>>>
>>>>>>>>>It does not implement the in-memory cache (yet? :-))
due to the
>>>>>>>>>discussion with simo.
>>>>>>>>>
>>>>>>>>>The patches expects "sudo api: check sss_status
instead of
>>>>>>>>>errnop in
>>>>>>>>>sss_sudo_send_recv_generic()" (already acked on
the list and
>>>>>>>>>waiting
>>>>>>>>>to be pushed).
>>>>>>>>
>>>>>>>>Patch 0001: sudo api: remove EOK
>>>>>>>>Ack
>>>>>>>>
>>>>>>>>Patch 0002: sudo responder: remove code duplication in
commands
>>>>>>>>I think it would be a good idea to also check if
>>>>>>>>rawname[rawname_len] ==
>>>>>>>>'\0' when parsing username, this would ensure
that all the string
>>>>>>>>checks
>>>>>>>>work OK.
>>>>>>>
>>>>>>>Done.
>>>>>>>
>>>>>>>>
>>>>>>>>Now that several sudosrv_response_append_* functions were
removed
>>>>>>>>from
>>>>>>>>header, can you make them static? In particular
>>>>>>>>sudosrv_response_append_string(),
sudosrv_response_append_uint32(),
>>>>>>>>sudosrv_response_append_rule(),
sudosrv_response_append_attr().
>>>>>>>
>>>>>>>Done.
>>>>>>>
>>>>>>>>Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info
more
>>>>>>>>configurable
>>>>>>>>Ack
>>>>>>>>
>>>>>>>>Patch 0004: sudo api: send uid, username and domainname
>>>>>>>>Please get rid of the C++ comments in sudosrv_cmd(). If
you need to
>>>>>>>>leave code in but disabled, please use #if 0/#endif
>>>>>>>
>>>>>>>Thanks. I'm using keybord shortcut to comment out whole
block with
>>>>>>>these slashes and I fortgot that we have this rule :)
>>>>>>>
>>>>>>>The whole hunk that
>>>>>>>>changes sudosrv_cmd() seems like it belongs to patch #2.
>>>>>>>
>>>>>>>Not really. It requires the new protocol which is implemented
in
>>>>>>>this
>>>>>>>patch. It makes the two command more similar than they was
before.
>>>>>>>
>>>>>>>>There's another // comment in
sudosrv_get_sudorules_from_cache.
>>>>>>>>
>>>>>>>>The logic looks good to me, though.
>>>>>>>
>>>>>>>Thank you for the review.
>>>>>>>
>>>>>>>I'm attaching a 6th patch that removes all code related
to the
>>>>>>>in-memory cache as it will not be used anymore.
>>>>>>>
>>>>>
>>>>>Patch 0001: Ack
>>>>>Patch 0002: Ack
>>>>>Patch 0003: Ack
>>>>>Patch 0004: Ack
>>>>>Patch 0005: Ack
>>>>>Patch 0006: Nack, does to apply on master:
>>>>>
>>>>>$ git reset --hard origin/master
>>>>>HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
>>>>>$ git am *.patch
>>>>>Applying: sudo api: remove EOK
>>>>>Applying: sudo responder: remove code duplication in commands
>>>>>Applying: sudo responder: get rid of dctx where possible
>>>>>Applying: sudo sysdb: make sysdb_get_sudo_user_info more
configurable
>>>>>Applying: sudo api: send uid, username and domainname
>>>>>Applying: sudo responder: discard in-memory cache
>>>>>error: patch failed: src/responder/sudo/sudosrv_cache.c:1
>>>
>>>I'm sorry, but it works for me. Are you sure you have the correct
>>>patches? What exactly fails to merge?
>>>
>>>$ git reset --hard origin/master
>>>HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
>>>$ git am *.patch
>>>Applying: sudo api: remove EOK
>>>Applying: sudo responder: remove code duplication in commands
>>>Applying: sudo responder: get rid of dctx where possible
>>>Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
>>>Applying: sudo api: send uid, username and domainname
>>>Applying: sudo responder: discard in-memory cache
>>>$
>>
>>I'm sending it again formatted via git format-patch -M -C --patience
>>--full-index and tarred.
>
Thank you, I'm not sure which one of the changes did it, but I'm able to
apply the patches now.
>There was one big mistake in the fifth patch.
>
>I was passing wrong uid information into query_cache function.
I have added a patch that increases protocol number. If client uses
old protocol, responder will return error immediately.
Sorry, one last change..can you change --version to --version-info and
bump the "current" number in the version string to 1?
I know the sudo part is not yet ready. But I would like to get these
patches pushed - I don't want to rebase them again :) It does not
break sudo until sss is present in nsswitch.conf.
FWIW, I don't like breaking running systems. Yes, there is probably very
few people who use sudo and sssd and would be affected by this, but
still..
I don't have any issues with the code, I'll ack it when the version is
bumped.
6:03 a.m.
On 14.5.2012 18:37, Jakub Hrozek wrote:
On Mon, May 14, 2012 at 05:55:09PM +0200, Pavel Březina wrote:
> On 11.5.2012 12:23, Pavel Březina wrote:
>> On 05/07/2012 07:17 PM, Pavel Březina wrote:
>>> On 05/03/2012 10:30 PM, Pavel Březina wrote:
>>>> On 3.5.2012 13:48, Jakub Hrozek wrote:
>>>>> On Fri, Apr 27, 2012 at 01:21:30PM +0200, Jakub Hrozek wrote:
>>>>>> On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel Březina wrote:
>>>>>>> The subdomains patches messed with one line which was
causing
>>>>>>> troubles.
>>>>>>> Here are the rebased patches.
>>>>>>>
>>>>>>> On 04/23/2012 12:51 PM, Pavel Březina wrote:
>>>>>>>> On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
>>>>>>>>> On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel
Březina wrote:
>>>>>>>>>> On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
>>>>>>>>>>> On Mon, Mar 19, 2012 at 02:46:02PM +0100,
Pavel Březina wrote:
>>>>>>>>>>>>
https://fedorahosted.org/sssd/ticket/1239
>>>>>>>>>>>>
>>>>>>>>>>>> [PATCH 1/2]
>>>>>>>>>>>> Finally removes EOK constant from sudo
api header. It is not
>>>>>>>>>>>> used in
>>>>>>>>>>>> the SUDO code so it does not require
their changes.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Looks good to me, it'w weird to use EOK
in an external header
>>>>>>>>>>> (even
>>>>>>>>>>> though it is correctly defined).
>>>>>>>>>>>
>>>>>>>>>>> (Note: this patch shouldn't be pushed
even though I don't have
>>>>>>>>>>> any
>>>>>>>>>>> comments. It is an ABI break and we need to
coordinate with
>>>>>>>>>>> Daniel K.)
>>>>>>>>>>>
>>>>>>>>>>>> [PATCH 2/2]
>>>>>>>>>>>> This does what is requested in the
ticket. It seems to be very
>>>>>>>>>>>> huge
>>>>>>>>>>>> but
>>>>>>>>>>>> in fact it is mainly changing the
variable. Basically I tried
>>>>>>>>>>>> to get
>>>>>>>>>>>> rid of domain ctx where possible, leave
it only in initgroups
>>>>>>>>>>>> part and
>>>>>>>>>>>> use command ctx elsewhere.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Still, it is hard to review the huge patch.
Can you split it into
>>>>>>>>>>> smaller ones? What about creating one that
removes the
>>>>>>>>>>> duplication
>>>>>>>>>>> between _get_sudorules and _get_defaults, one
that converts to
>>>>>>>>>>> using
>>>>>>>>>>> cmdctx and one that adds the uid support?
>>>>>>>>>>
>>>>>>>>>> The patches are attached.
>>>>>>>>>>
>>>>>>>>>> I want to get those patches acked because I need
to deliver the
>>>>>>>>>> updated
>>>>>>>>>> interface to Dan so he can update the sudo
binary.
>>>>>>>>>>
>>>>>>>>>> It does not implement the in-memory cache (yet?
:-)) due to the
>>>>>>>>>> discussion with simo.
>>>>>>>>>>
>>>>>>>>>> The patches expects "sudo api: check
sss_status instead of
>>>>>>>>>> errnop in
>>>>>>>>>> sss_sudo_send_recv_generic()" (already acked
on the list and
>>>>>>>>>> waiting
>>>>>>>>>> to be pushed).
>>>>>>>>>
>>>>>>>>> Patch 0001: sudo api: remove EOK
>>>>>>>>> Ack
>>>>>>>>>
>>>>>>>>> Patch 0002: sudo responder: remove code duplication
in commands
>>>>>>>>> I think it would be a good idea to also check if
>>>>>>>>> rawname[rawname_len] ==
>>>>>>>>> '\0' when parsing username, this would ensure
that all the string
>>>>>>>>> checks
>>>>>>>>> work OK.
>>>>>>>>
>>>>>>>> Done.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Now that several sudosrv_response_append_* functions
were removed
>>>>>>>>> from
>>>>>>>>> header, can you make them static? In particular
>>>>>>>>> sudosrv_response_append_string(),
sudosrv_response_append_uint32(),
>>>>>>>>> sudosrv_response_append_rule(),
sudosrv_response_append_attr().
>>>>>>>>
>>>>>>>> Done.
>>>>>>>>
>>>>>>>>> Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info
more
>>>>>>>>> configurable
>>>>>>>>> Ack
>>>>>>>>>
>>>>>>>>> Patch 0004: sudo api: send uid, username and
domainname
>>>>>>>>> Please get rid of the C++ comments in sudosrv_cmd().
If you need to
>>>>>>>>> leave code in but disabled, please use #if 0/#endif
>>>>>>>>
>>>>>>>> Thanks. I'm using keybord shortcut to comment out
whole block with
>>>>>>>> these slashes and I fortgot that we have this rule :)
>>>>>>>>
>>>>>>>> The whole hunk that
>>>>>>>>> changes sudosrv_cmd() seems like it belongs to patch
#2.
>>>>>>>>
>>>>>>>> Not really. It requires the new protocol which is
implemented in
>>>>>>>> this
>>>>>>>> patch. It makes the two command more similar than they
was before.
>>>>>>>>
>>>>>>>>> There's another // comment in
sudosrv_get_sudorules_from_cache.
>>>>>>>>>
>>>>>>>>> The logic looks good to me, though.
>>>>>>>>
>>>>>>>> Thank you for the review.
>>>>>>>>
>>>>>>>> I'm attaching a 6th patch that removes all code
related to the
>>>>>>>> in-memory cache as it will not be used anymore.
>>>>>>>>
>>>>>>
>>>>>> Patch 0001: Ack
>>>>>> Patch 0002: Ack
>>>>>> Patch 0003: Ack
>>>>>> Patch 0004: Ack
>>>>>> Patch 0005: Ack
>>>>>> Patch 0006: Nack, does to apply on master:
>>>>>>
>>>>>> $ git reset --hard origin/master
>>>>>> HEAD is now at 24ba5b8 NSS: Only return data from initgroups
once
>>>>>> $ git am *.patch
>>>>>> Applying: sudo api: remove EOK
>>>>>> Applying: sudo responder: remove code duplication in commands
>>>>>> Applying: sudo responder: get rid of dctx where possible
>>>>>> Applying: sudo sysdb: make sysdb_get_sudo_user_info more
configurable
>>>>>> Applying: sudo api: send uid, username and domainname
>>>>>> Applying: sudo responder: discard in-memory cache
>>>>>> error: patch failed: src/responder/sudo/sudosrv_cache.c:1
>>>>
>>>> I'm sorry, but it works for me. Are you sure you have the correct
>>>> patches? What exactly fails to merge?
>>>>
>>>> $ git reset --hard origin/master
>>>> HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
>>>> $ git am *.patch
>>>> Applying: sudo api: remove EOK
>>>> Applying: sudo responder: remove code duplication in commands
>>>> Applying: sudo responder: get rid of dctx where possible
>>>> Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
>>>> Applying: sudo api: send uid, username and domainname
>>>> Applying: sudo responder: discard in-memory cache
>>>> $
>>>
>>> I'm sending it again formatted via git format-patch -M -C --patience
>>> --full-index and tarred.
>>
Thank you, I'm not sure which one of the changes did it, but I'm able to
apply the patches now.
>> There was one big mistake in the fifth patch.
>>
>> I was passing wrong uid information into query_cache function.
>
> I have added a patch that increases protocol number. If client uses
> old protocol, responder will return error immediately.
>
Sorry, one last change..can you change --version to --version-info and
bump the "current" number in the version string to 1?
Patches are attached.
> I know the sudo part is not yet ready. But I would like to get
these
> patches pushed - I don't want to rebase them again :) It does not
> break sudo until sss is present in nsswitch.conf.
FWIW, I don't like breaking running systems. Yes, there is probably very
few people who use sudo and sssd and would be affected by this, but
still..
I don't have any issues with the code, I'll ack it when the version is
bumped.
10:29 a.m.
On 17.5.2012 13:03, Pavel Březina wrote:
On 14.5.2012 18:37, Jakub Hrozek wrote:
> On Mon, May 14, 2012 at 05:55:09PM +0200, Pavel Březina wrote:
>> On 11.5.2012 12:23, Pavel Březina wrote:
>>> On 05/07/2012 07:17 PM, Pavel Březina wrote:
>>>> On 05/03/2012 10:30 PM, Pavel Březina wrote:
>>>>> On 3.5.2012 13:48, Jakub Hrozek wrote:
>>>>>> On Fri, Apr 27, 2012 at 01:21:30PM +0200, Jakub Hrozek wrote:
>>>>>>> On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel Březina
wrote:
>>>>>>>> The subdomains patches messed with one line which was
causing
>>>>>>>> troubles.
>>>>>>>> Here are the rebased patches.
>>>>>>>>
>>>>>>>> On 04/23/2012 12:51 PM, Pavel Březina wrote:
>>>>>>>>> On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
>>>>>>>>>> On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel
Březina wrote:
>>>>>>>>>>> On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
>>>>>>>>>>>> On Mon, Mar 19, 2012 at 02:46:02PM +0100,
Pavel Březina wrote:
>>>>>>>>>>>>>
https://fedorahosted.org/sssd/ticket/1239
>>>>>>>>>>>>>
>>>>>>>>>>>>> [PATCH 1/2]
>>>>>>>>>>>>> Finally removes EOK constant from
sudo api header. It is not
>>>>>>>>>>>>> used in
>>>>>>>>>>>>> the SUDO code so it does not require
their changes.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Looks good to me, it'w weird to use
EOK in an external header
>>>>>>>>>>>> (even
>>>>>>>>>>>> though it is correctly defined).
>>>>>>>>>>>>
>>>>>>>>>>>> (Note: this patch shouldn't be pushed
even though I don't have
>>>>>>>>>>>> any
>>>>>>>>>>>> comments. It is an ABI break and we need
to coordinate with
>>>>>>>>>>>> Daniel K.)
>>>>>>>>>>>>
>>>>>>>>>>>>> [PATCH 2/2]
>>>>>>>>>>>>> This does what is requested in the
ticket. It seems to be
>>>>>>>>>>>>> very
>>>>>>>>>>>>> huge
>>>>>>>>>>>>> but
>>>>>>>>>>>>> in fact it is mainly changing the
variable. Basically I tried
>>>>>>>>>>>>> to get
>>>>>>>>>>>>> rid of domain ctx where possible,
leave it only in initgroups
>>>>>>>>>>>>> part and
>>>>>>>>>>>>> use command ctx elsewhere.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Still, it is hard to review the huge
patch. Can you split
>>>>>>>>>>>> it into
>>>>>>>>>>>> smaller ones? What about creating one
that removes the
>>>>>>>>>>>> duplication
>>>>>>>>>>>> between _get_sudorules and _get_defaults,
one that converts to
>>>>>>>>>>>> using
>>>>>>>>>>>> cmdctx and one that adds the uid
support?
>>>>>>>>>>>
>>>>>>>>>>> The patches are attached.
>>>>>>>>>>>
>>>>>>>>>>> I want to get those patches acked because I
need to deliver the
>>>>>>>>>>> updated
>>>>>>>>>>> interface to Dan so he can update the sudo
binary.
>>>>>>>>>>>
>>>>>>>>>>> It does not implement the in-memory cache
(yet? :-)) due to the
>>>>>>>>>>> discussion with simo.
>>>>>>>>>>>
>>>>>>>>>>> The patches expects "sudo api: check
sss_status instead of
>>>>>>>>>>> errnop in
>>>>>>>>>>> sss_sudo_send_recv_generic()" (already
acked on the list and
>>>>>>>>>>> waiting
>>>>>>>>>>> to be pushed).
>>>>>>>>>>
>>>>>>>>>> Patch 0001: sudo api: remove EOK
>>>>>>>>>> Ack
>>>>>>>>>>
>>>>>>>>>> Patch 0002: sudo responder: remove code
duplication in commands
>>>>>>>>>> I think it would be a good idea to also check if
>>>>>>>>>> rawname[rawname_len] ==
>>>>>>>>>> '\0' when parsing username, this would
ensure that all the
>>>>>>>>>> string
>>>>>>>>>> checks
>>>>>>>>>> work OK.
>>>>>>>>>
>>>>>>>>> Done.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Now that several sudosrv_response_append_*
functions were
>>>>>>>>>> removed
>>>>>>>>>> from
>>>>>>>>>> header, can you make them static? In particular
>>>>>>>>>> sudosrv_response_append_string(),
>>>>>>>>>> sudosrv_response_append_uint32(),
>>>>>>>>>> sudosrv_response_append_rule(),
sudosrv_response_append_attr().
>>>>>>>>>
>>>>>>>>> Done.
>>>>>>>>>
>>>>>>>>>> Patch 0003: sudo sysdb: make
sysdb_get_sudo_user_info more
>>>>>>>>>> configurable
>>>>>>>>>> Ack
>>>>>>>>>>
>>>>>>>>>> Patch 0004: sudo api: send uid, username and
domainname
>>>>>>>>>> Please get rid of the C++ comments in
sudosrv_cmd(). If you
>>>>>>>>>> need to
>>>>>>>>>> leave code in but disabled, please use #if
0/#endif
>>>>>>>>>
>>>>>>>>> Thanks. I'm using keybord shortcut to comment out
whole block
>>>>>>>>> with
>>>>>>>>> these slashes and I fortgot that we have this rule
:)
>>>>>>>>>
>>>>>>>>> The whole hunk that
>>>>>>>>>> changes sudosrv_cmd() seems like it belongs to
patch #2.
>>>>>>>>>
>>>>>>>>> Not really. It requires the new protocol which is
implemented in
>>>>>>>>> this
>>>>>>>>> patch. It makes the two command more similar than
they was
>>>>>>>>> before.
>>>>>>>>>
>>>>>>>>>> There's another // comment in
sudosrv_get_sudorules_from_cache.
>>>>>>>>>>
>>>>>>>>>> The logic looks good to me, though.
>>>>>>>>>
>>>>>>>>> Thank you for the review.
>>>>>>>>>
>>>>>>>>> I'm attaching a 6th patch that removes all code
related to the
>>>>>>>>> in-memory cache as it will not be used anymore.
>>>>>>>>>
>>>>>>>
>>>>>>> Patch 0001: Ack
>>>>>>> Patch 0002: Ack
>>>>>>> Patch 0003: Ack
>>>>>>> Patch 0004: Ack
>>>>>>> Patch 0005: Ack
>>>>>>> Patch 0006: Nack, does to apply on master:
>>>>>>>
>>>>>>> $ git reset --hard origin/master
>>>>>>> HEAD is now at 24ba5b8 NSS: Only return data from initgroups
once
>>>>>>> $ git am *.patch
>>>>>>> Applying: sudo api: remove EOK
>>>>>>> Applying: sudo responder: remove code duplication in
commands
>>>>>>> Applying: sudo responder: get rid of dctx where possible
>>>>>>> Applying: sudo sysdb: make sysdb_get_sudo_user_info more
>>>>>>> configurable
>>>>>>> Applying: sudo api: send uid, username and domainname
>>>>>>> Applying: sudo responder: discard in-memory cache
>>>>>>> error: patch failed: src/responder/sudo/sudosrv_cache.c:1
>>>>>
>>>>> I'm sorry, but it works for me. Are you sure you have the
correct
>>>>> patches? What exactly fails to merge?
>>>>>
>>>>> $ git reset --hard origin/master
>>>>> HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
>>>>> $ git am *.patch
>>>>> Applying: sudo api: remove EOK
>>>>> Applying: sudo responder: remove code duplication in commands
>>>>> Applying: sudo responder: get rid of dctx where possible
>>>>> Applying: sudo sysdb: make sysdb_get_sudo_user_info more
configurable
>>>>> Applying: sudo api: send uid, username and domainname
>>>>> Applying: sudo responder: discard in-memory cache
>>>>> $
>>>>
>>>> I'm sending it again formatted via git format-patch -M -C --patience
>>>> --full-index and tarred.
>>>
>
> Thank you, I'm not sure which one of the changes did it, but I'm able to
> apply the patches now.
>
>>> There was one big mistake in the fifth patch.
>>>
>>> I was passing wrong uid information into query_cache function.
>>
>> I have added a patch that increases protocol number. If client uses
>> old protocol, responder will return error immediately.
>>
>
> Sorry, one last change..can you change --version to --version-info and
> bump the "current" number in the version string to 1?
Patches are attached.
>> I know the sudo part is not yet ready. But I would like to get these
>> patches pushed - I don't want to rebase them again :) It does not
>> break sudo until sss is present in nsswitch.conf.
>
> FWIW, I don't like breaking running systems. Yes, there is probably very
> few people who use sudo and sssd and would be affected by this, but
> still..
>
> I don't have any issues with the code, I'll ack it when the version is
> bumped.
Rebased on the current master.
The sudo client is finally ready:
http://koji.fedoraproject.org/koji/rpminfo?rpmID=3115173
I will test it tomorrow.
3:02 a.m.
On 06/21/2012 05:29 PM, Pavel Březina wrote:
On 17.5.2012 13:03, Pavel Březina wrote:
> On 14.5.2012 18:37, Jakub Hrozek wrote:
>> On Mon, May 14, 2012 at 05:55:09PM +0200, Pavel Březina wrote:
>>> On 11.5.2012 12:23, Pavel Březina wrote:
>>>> On 05/07/2012 07:17 PM, Pavel Březina wrote:
>>>>> On 05/03/2012 10:30 PM, Pavel Březina wrote:
>>>>>> On 3.5.2012 13:48, Jakub Hrozek wrote:
>>>>>>> On Fri, Apr 27, 2012 at 01:21:30PM +0200, Jakub Hrozek
wrote:
>>>>>>>> On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel Březina
wrote:
>>>>>>>>> The subdomains patches messed with one line which was
causing
>>>>>>>>> troubles.
>>>>>>>>> Here are the rebased patches.
>>>>>>>>>
>>>>>>>>> On 04/23/2012 12:51 PM, Pavel Březina wrote:
>>>>>>>>>> On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
>>>>>>>>>>> On Fri, Apr 13, 2012 at 12:46:01PM +0200,
Pavel Březina wrote:
>>>>>>>>>>>> On 03/26/2012 09:47 PM, Jakub Hrozek
wrote:
>>>>>>>>>>>>> On Mon, Mar 19, 2012 at 02:46:02PM
+0100, Pavel Březina
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
https://fedorahosted.org/sssd/ticket/1239
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [PATCH 1/2]
>>>>>>>>>>>>>> Finally removes EOK constant from
sudo api header. It is not
>>>>>>>>>>>>>> used in
>>>>>>>>>>>>>> the SUDO code so it does not
require their changes.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Looks good to me, it'w weird to
use EOK in an external header
>>>>>>>>>>>>> (even
>>>>>>>>>>>>> though it is correctly defined).
>>>>>>>>>>>>>
>>>>>>>>>>>>> (Note: this patch shouldn't be
pushed even though I don't
>>>>>>>>>>>>> have
>>>>>>>>>>>>> any
>>>>>>>>>>>>> comments. It is an ABI break and we
need to coordinate with
>>>>>>>>>>>>> Daniel K.)
>>>>>>>>>>>>>
>>>>>>>>>>>>>> [PATCH 2/2]
>>>>>>>>>>>>>> This does what is requested in
the ticket. It seems to be
>>>>>>>>>>>>>> very
>>>>>>>>>>>>>> huge
>>>>>>>>>>>>>> but
>>>>>>>>>>>>>> in fact it is mainly changing the
variable. Basically I
>>>>>>>>>>>>>> tried
>>>>>>>>>>>>>> to get
>>>>>>>>>>>>>> rid of domain ctx where possible,
leave it only in
>>>>>>>>>>>>>> initgroups
>>>>>>>>>>>>>> part and
>>>>>>>>>>>>>> use command ctx elsewhere.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Still, it is hard to review the huge
patch. Can you split
>>>>>>>>>>>>> it into
>>>>>>>>>>>>> smaller ones? What about creating one
that removes the
>>>>>>>>>>>>> duplication
>>>>>>>>>>>>> between _get_sudorules and
_get_defaults, one that
>>>>>>>>>>>>> converts to
>>>>>>>>>>>>> using
>>>>>>>>>>>>> cmdctx and one that adds the uid
support?
>>>>>>>>>>>>
>>>>>>>>>>>> The patches are attached.
>>>>>>>>>>>>
>>>>>>>>>>>> I want to get those patches acked because
I need to deliver
>>>>>>>>>>>> the
>>>>>>>>>>>> updated
>>>>>>>>>>>> interface to Dan so he can update the
sudo binary.
>>>>>>>>>>>>
>>>>>>>>>>>> It does not implement the in-memory cache
(yet? :-)) due to
>>>>>>>>>>>> the
>>>>>>>>>>>> discussion with simo.
>>>>>>>>>>>>
>>>>>>>>>>>> The patches expects "sudo api: check
sss_status instead of
>>>>>>>>>>>> errnop in
>>>>>>>>>>>> sss_sudo_send_recv_generic()"
(already acked on the list and
>>>>>>>>>>>> waiting
>>>>>>>>>>>> to be pushed).
>>>>>>>>>>>
>>>>>>>>>>> Patch 0001: sudo api: remove EOK
>>>>>>>>>>> Ack
>>>>>>>>>>>
>>>>>>>>>>> Patch 0002: sudo responder: remove code
duplication in commands
>>>>>>>>>>> I think it would be a good idea to also check
if
>>>>>>>>>>> rawname[rawname_len] ==
>>>>>>>>>>> '\0' when parsing username, this
would ensure that all the
>>>>>>>>>>> string
>>>>>>>>>>> checks
>>>>>>>>>>> work OK.
>>>>>>>>>>
>>>>>>>>>> Done.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Now that several sudosrv_response_append_*
functions were
>>>>>>>>>>> removed
>>>>>>>>>>> from
>>>>>>>>>>> header, can you make them static? In
particular
>>>>>>>>>>> sudosrv_response_append_string(),
>>>>>>>>>>> sudosrv_response_append_uint32(),
>>>>>>>>>>> sudosrv_response_append_rule(),
sudosrv_response_append_attr().
>>>>>>>>>>
>>>>>>>>>> Done.
>>>>>>>>>>
>>>>>>>>>>> Patch 0003: sudo sysdb: make
sysdb_get_sudo_user_info more
>>>>>>>>>>> configurable
>>>>>>>>>>> Ack
>>>>>>>>>>>
>>>>>>>>>>> Patch 0004: sudo api: send uid, username and
domainname
>>>>>>>>>>> Please get rid of the C++ comments in
sudosrv_cmd(). If you
>>>>>>>>>>> need to
>>>>>>>>>>> leave code in but disabled, please use #if
0/#endif
>>>>>>>>>>
>>>>>>>>>> Thanks. I'm using keybord shortcut to comment
out whole block
>>>>>>>>>> with
>>>>>>>>>> these slashes and I fortgot that we have this
rule :)
>>>>>>>>>>
>>>>>>>>>> The whole hunk that
>>>>>>>>>>> changes sudosrv_cmd() seems like it belongs
to patch #2.
>>>>>>>>>>
>>>>>>>>>> Not really. It requires the new protocol which is
implemented in
>>>>>>>>>> this
>>>>>>>>>> patch. It makes the two command more similar than
they was
>>>>>>>>>> before.
>>>>>>>>>>
>>>>>>>>>>> There's another // comment in
sudosrv_get_sudorules_from_cache.
>>>>>>>>>>>
>>>>>>>>>>> The logic looks good to me, though.
>>>>>>>>>>
>>>>>>>>>> Thank you for the review.
>>>>>>>>>>
>>>>>>>>>> I'm attaching a 6th patch that removes all
code related to the
>>>>>>>>>> in-memory cache as it will not be used anymore.
>>>>>>>>>>
>>>>>>>>
>>>>>>>> Patch 0001: Ack
>>>>>>>> Patch 0002: Ack
>>>>>>>> Patch 0003: Ack
>>>>>>>> Patch 0004: Ack
>>>>>>>> Patch 0005: Ack
>>>>>>>> Patch 0006: Nack, does to apply on master:
>>>>>>>>
>>>>>>>> $ git reset --hard origin/master
>>>>>>>> HEAD is now at 24ba5b8 NSS: Only return data from
initgroups once
>>>>>>>> $ git am *.patch
>>>>>>>> Applying: sudo api: remove EOK
>>>>>>>> Applying: sudo responder: remove code duplication in
commands
>>>>>>>> Applying: sudo responder: get rid of dctx where possible
>>>>>>>> Applying: sudo sysdb: make sysdb_get_sudo_user_info more
>>>>>>>> configurable
>>>>>>>> Applying: sudo api: send uid, username and domainname
>>>>>>>> Applying: sudo responder: discard in-memory cache
>>>>>>>> error: patch failed:
src/responder/sudo/sudosrv_cache.c:1
>>>>>>
>>>>>> I'm sorry, but it works for me. Are you sure you have the
correct
>>>>>> patches? What exactly fails to merge?
>>>>>>
>>>>>> $ git reset --hard origin/master
>>>>>> HEAD is now at 24ba5b8 NSS: Only return data from initgroups
once
>>>>>> $ git am *.patch
>>>>>> Applying: sudo api: remove EOK
>>>>>> Applying: sudo responder: remove code duplication in commands
>>>>>> Applying: sudo responder: get rid of dctx where possible
>>>>>> Applying: sudo sysdb: make sysdb_get_sudo_user_info more
>>>>>> configurable
>>>>>> Applying: sudo api: send uid, username and domainname
>>>>>> Applying: sudo responder: discard in-memory cache
>>>>>> $
>>>>>
>>>>> I'm sending it again formatted via git format-patch -M -C
--patience
>>>>> --full-index and tarred.
>>>>
>>
>> Thank you, I'm not sure which one of the changes did it, but I'm able to
>> apply the patches now.
>>
>>>> There was one big mistake in the fifth patch.
>>>>
>>>> I was passing wrong uid information into query_cache function.
>>>
>>> I have added a patch that increases protocol number. If client uses
>>> old protocol, responder will return error immediately.
>>>
>>
>> Sorry, one last change..can you change --version to --version-info and
>> bump the "current" number in the version string to 1?
>
> Patches are attached.
>
>>> I know the sudo part is not yet ready. But I would like to get these
>>> patches pushed - I don't want to rebase them again :) It does not
>>> break sudo until sss is present in nsswitch.conf.
>>
>> FWIW, I don't like breaking running systems. Yes, there is probably very
>> few people who use sudo and sssd and would be affected by this, but
>> still..
>>
>> I don't have any issues with the code, I'll ack it when the version is
>> bumped.
Rebased on the current master.
The sudo client is finally ready:
http://koji.fedoraproject.org/koji/rpminfo?rpmID=3115173
I will test it tomorrow.
The sudo client has some problems with undefined symbols while
dlopen(sudoers.so). Daniel should look at it today.
I'm sending rebased patches one more time.
4:11 a.m.
On Tue, Jun 26, 2012 at 10:02:32AM +0200, Pavel Březina wrote:
On 06/21/2012 05:29 PM, Pavel Březina wrote:
>On 17.5.2012 13:03, Pavel Březina wrote:
>>On 14.5.2012 18:37, Jakub Hrozek wrote:
>>>On Mon, May 14, 2012 at 05:55:09PM +0200, Pavel Březina wrote:
>>>>On 11.5.2012 12:23, Pavel Březina wrote:
>>>>>On 05/07/2012 07:17 PM, Pavel Březina wrote:
>>>>>>On 05/03/2012 10:30 PM, Pavel Březina wrote:
>>>>>>>On 3.5.2012 13:48, Jakub Hrozek wrote:
>>>>>>>>On Fri, Apr 27, 2012 at 01:21:30PM +0200, Jakub Hrozek
wrote:
>>>>>>>>>On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel
Březina wrote:
>>>>>>>>>>The subdomains patches messed with one line which
was causing
>>>>>>>>>>troubles.
>>>>>>>>>>Here are the rebased patches.
>>>>>>>>>>
>>>>>>>>>>On 04/23/2012 12:51 PM, Pavel Březina wrote:
>>>>>>>>>>>On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
>>>>>>>>>>>>On Fri, Apr 13, 2012 at 12:46:01PM +0200,
Pavel Březina wrote:
>>>>>>>>>>>>>On 03/26/2012 09:47 PM, Jakub Hrozek
wrote:
>>>>>>>>>>>>>>On Mon, Mar 19, 2012 at
02:46:02PM +0100, Pavel Březina
>>>>>>>>>>>>>>wrote:
>>>>>>>>>>>>>>>https://fedorahosted.org/sssd/ticket/1239
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>[PATCH 1/2]
>>>>>>>>>>>>>>>Finally removes EOK constant
from sudo api header. It is not
>>>>>>>>>>>>>>>used in
>>>>>>>>>>>>>>>the SUDO code so it does not
require their changes.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>Looks good to me, it'w weird
to use EOK in an external header
>>>>>>>>>>>>>>(even
>>>>>>>>>>>>>>though it is correctly defined).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>(Note: this patch shouldn't
be pushed even though I don't
>>>>>>>>>>>>>>have
>>>>>>>>>>>>>>any
>>>>>>>>>>>>>>comments. It is an ABI break and
we need to coordinate with
>>>>>>>>>>>>>>Daniel K.)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>[PATCH 2/2]
>>>>>>>>>>>>>>>This does what is requested
in the ticket. It seems to be
>>>>>>>>>>>>>>>very
>>>>>>>>>>>>>>>huge
>>>>>>>>>>>>>>>but
>>>>>>>>>>>>>>>in fact it is mainly changing
the variable. Basically I
>>>>>>>>>>>>>>>tried
>>>>>>>>>>>>>>>to get
>>>>>>>>>>>>>>>rid of domain ctx where
possible, leave it only in
>>>>>>>>>>>>>>>initgroups
>>>>>>>>>>>>>>>part and
>>>>>>>>>>>>>>>use command ctx elsewhere.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>Still, it is hard to review the
huge patch. Can you split
>>>>>>>>>>>>>>it into
>>>>>>>>>>>>>>smaller ones? What about creating
one that removes the
>>>>>>>>>>>>>>duplication
>>>>>>>>>>>>>>between _get_sudorules and
_get_defaults, one that
>>>>>>>>>>>>>>converts to
>>>>>>>>>>>>>>using
>>>>>>>>>>>>>>cmdctx and one that adds the uid
support?
>>>>>>>>>>>>>
>>>>>>>>>>>>>The patches are attached.
>>>>>>>>>>>>>
>>>>>>>>>>>>>I want to get those patches acked
because I need to deliver
>>>>>>>>>>>>>the
>>>>>>>>>>>>>updated
>>>>>>>>>>>>>interface to Dan so he can update the
sudo binary.
>>>>>>>>>>>>>
>>>>>>>>>>>>>It does not implement the in-memory
cache (yet? :-)) due to
>>>>>>>>>>>>>the
>>>>>>>>>>>>>discussion with simo.
>>>>>>>>>>>>>
>>>>>>>>>>>>>The patches expects "sudo api:
check sss_status instead of
>>>>>>>>>>>>>errnop in
>>>>>>>>>>>>>sss_sudo_send_recv_generic()"
(already acked on the list and
>>>>>>>>>>>>>waiting
>>>>>>>>>>>>>to be pushed).
>>>>>>>>>>>>
>>>>>>>>>>>>Patch 0001: sudo api: remove EOK
>>>>>>>>>>>>Ack
>>>>>>>>>>>>
>>>>>>>>>>>>Patch 0002: sudo responder: remove code
duplication in commands
>>>>>>>>>>>>I think it would be a good idea to also
check if
>>>>>>>>>>>>rawname[rawname_len] ==
>>>>>>>>>>>>'\0' when parsing username, this
would ensure that all the
>>>>>>>>>>>>string
>>>>>>>>>>>>checks
>>>>>>>>>>>>work OK.
>>>>>>>>>>>
>>>>>>>>>>>Done.
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>Now that several
sudosrv_response_append_* functions were
>>>>>>>>>>>>removed
>>>>>>>>>>>>from
>>>>>>>>>>>>header, can you make them static? In
particular
>>>>>>>>>>>>sudosrv_response_append_string(),
>>>>>>>>>>>>sudosrv_response_append_uint32(),
>>>>>>>>>>>>sudosrv_response_append_rule(),
sudosrv_response_append_attr().
>>>>>>>>>>>
>>>>>>>>>>>Done.
>>>>>>>>>>>
>>>>>>>>>>>>Patch 0003: sudo sysdb: make
sysdb_get_sudo_user_info more
>>>>>>>>>>>>configurable
>>>>>>>>>>>>Ack
>>>>>>>>>>>>
>>>>>>>>>>>>Patch 0004: sudo api: send uid, username
and domainname
>>>>>>>>>>>>Please get rid of the C++ comments in
sudosrv_cmd(). If you
>>>>>>>>>>>>need to
>>>>>>>>>>>>leave code in but disabled, please use
#if 0/#endif
>>>>>>>>>>>
>>>>>>>>>>>Thanks. I'm using keybord shortcut to
comment out whole block
>>>>>>>>>>>with
>>>>>>>>>>>these slashes and I fortgot that we have this
rule :)
>>>>>>>>>>>
>>>>>>>>>>>The whole hunk that
>>>>>>>>>>>>changes sudosrv_cmd() seems like it
belongs to patch #2.
>>>>>>>>>>>
>>>>>>>>>>>Not really. It requires the new protocol
which is implemented in
>>>>>>>>>>>this
>>>>>>>>>>>patch. It makes the two command more similar
than they was
>>>>>>>>>>>before.
>>>>>>>>>>>
>>>>>>>>>>>>There's another // comment in
sudosrv_get_sudorules_from_cache.
>>>>>>>>>>>>
>>>>>>>>>>>>The logic looks good to me, though.
>>>>>>>>>>>
>>>>>>>>>>>Thank you for the review.
>>>>>>>>>>>
>>>>>>>>>>>I'm attaching a 6th patch that removes
all code related to the
>>>>>>>>>>>in-memory cache as it will not be used
anymore.
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>Patch 0001: Ack
>>>>>>>>>Patch 0002: Ack
>>>>>>>>>Patch 0003: Ack
>>>>>>>>>Patch 0004: Ack
>>>>>>>>>Patch 0005: Ack
>>>>>>>>>Patch 0006: Nack, does to apply on master:
>>>>>>>>>
>>>>>>>>>$ git reset --hard origin/master
>>>>>>>>>HEAD is now at 24ba5b8 NSS: Only return data from
initgroups once
>>>>>>>>>$ git am *.patch
>>>>>>>>>Applying: sudo api: remove EOK
>>>>>>>>>Applying: sudo responder: remove code duplication in
commands
>>>>>>>>>Applying: sudo responder: get rid of dctx where
possible
>>>>>>>>>Applying: sudo sysdb: make sysdb_get_sudo_user_info
more
>>>>>>>>>configurable
>>>>>>>>>Applying: sudo api: send uid, username and
domainname
>>>>>>>>>Applying: sudo responder: discard in-memory cache
>>>>>>>>>error: patch failed:
src/responder/sudo/sudosrv_cache.c:1
>>>>>>>
>>>>>>>I'm sorry, but it works for me. Are you sure you have the
correct
>>>>>>>patches? What exactly fails to merge?
>>>>>>>
>>>>>>>$ git reset --hard origin/master
>>>>>>>HEAD is now at 24ba5b8 NSS: Only return data from initgroups
once
>>>>>>>$ git am *.patch
>>>>>>>Applying: sudo api: remove EOK
>>>>>>>Applying: sudo responder: remove code duplication in
commands
>>>>>>>Applying: sudo responder: get rid of dctx where possible
>>>>>>>Applying: sudo sysdb: make sysdb_get_sudo_user_info more
>>>>>>>configurable
>>>>>>>Applying: sudo api: send uid, username and domainname
>>>>>>>Applying: sudo responder: discard in-memory cache
>>>>>>>$
>>>>>>
>>>>>>I'm sending it again formatted via git format-patch -M -C
--patience
>>>>>>--full-index and tarred.
>>>>>
>>>
>>>Thank you, I'm not sure which one of the changes did it, but I'm able
to
>>>apply the patches now.
>>>
>>>>>There was one big mistake in the fifth patch.
>>>>>
>>>>>I was passing wrong uid information into query_cache function.
>>>>
>>>>I have added a patch that increases protocol number. If client uses
>>>>old protocol, responder will return error immediately.
>>>>
>>>
>>>Sorry, one last change..can you change --version to --version-info and
>>>bump the "current" number in the version string to 1?
>>
>>Patches are attached.
>>
>>>>I know the sudo part is not yet ready. But I would like to get these
>>>>patches pushed - I don't want to rebase them again :) It does not
>>>>break sudo until sss is present in nsswitch.conf.
>>>
>>>FWIW, I don't like breaking running systems. Yes, there is probably very
>>>few people who use sudo and sssd and would be affected by this, but
>>>still..
>>>
>>>I don't have any issues with the code, I'll ack it when the version
is
>>>bumped.
>
>Rebased on the current master.
>
>The sudo client is finally ready:
>http://koji.fedoraproject.org/koji/rpminfo?rpmID=3115173
>
>I will test it tomorrow.
The sudo client has some problems with undefined symbols while
dlopen(sudoers.so). Daniel should look at it today.
I'm sending rebased patches one more time.
For the record, these patches were included in another thread, which
superseded this one.
4287
days inactive
4414
days old
sssd-devel@lists.fedorahosted.org
32 comments
4 participants
participants (4)
-
Dmitri Pal -
Jakub Hrozek -
Pavel Březina -
Simo Sorce