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.
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:
- 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?
- 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.
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:
- 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?
- 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.
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:
- 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?
- 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
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:
- 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?
- 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@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, Mar 27, 2012 at 10:05:04AM +0200, Pavel Březina wrote:
- 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.
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:
- 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.
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:
- 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?
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:
- 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
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:
- 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@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
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:
- 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).
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.
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*?
*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.
- *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.
- 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.
- 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 (*).
*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 ?
- 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.
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*?
*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.
- *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.
- 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.
- 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 (*).
*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 ?
- 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.
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.
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@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.
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@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.
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@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.
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).
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?
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
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@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
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.
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
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.
- 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.
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.
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.
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.
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.
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.
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.
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.
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.
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Jakub Hrozek -
Pavel Březina -
Simo Sorce