[SSSD] Re: Config file merging in SSSD
On (22/03/16 15:43), Simo Sorce wrote: >On Tue, 2016-03-22 at 15:15 +0100, Michal Židek wrote: >> On 03/22/2016 02:46 PM, Lukas Slebodnik wrote: >> > On (22/03/16 14:30), Michal Židek wrote: >> >> On 03/22/2016 12:29 PM, Michal Židek wrote: >> >>> Hi, >> >>> >> >>> I would like to write a patch that will >> >>> allow SSSD to use the config file merging >> >>> feature from libini. But first I would like >> >>> to ask developers for their opinions on how >> >>> this should be implemented. >> >>> >> >>> My idea was that it could work like this. >> >>> >> >>> The current /etc/sssd/sssd.conf would work >> >>> as usual. >> >>> >> >>> We would add new directory /etc/sssd/conf.d/ >> >>> and its content would be following: >> >>> - README file that informs what the direcotory >> >>> is for. >> >>> - any number of files ending with .conf extension >> >>>that would contain additional configuration for >> >>>SSSD. These files would have higher prioriry >> >>>than the /etc/sssd/sssd.conf , meaning if >> >>>the same option is present in these files >> >>>it will override the /etc/sssd/sssd.conf >> >>>value. > >What's the rational behind 'snippets vs main conf' makes snippet a >winner ? > We already have a policy "the last one win" even in current version of sssd.conf. QE use it very often in testing. So it would not ne a huge change. We can change it if there is a use case. >> >>> SSSD would automatically pick up files ending >> >>> in .conf from that direcory and use them. In >> >>> order to disable the config file, the admin will >> >>> have to rename the file ending (for example >> >>> .conf.disabled). This way, we do not need to >> >>> inspect the snippets for any special options >> >>> like 'enable_this_snippet = true' which would >> >>> just complicate the processing. >> >>> >> > Another, way how to ignore snippet is to ignore >> > any file which start with dot ".". >> > "hiddent files". It would avoid adding suffix to every file. >> > >> > BTW logrotate and crond do the same >> > /etc/logrotate.d/ >> > /etc/cron.d/ > >logrotate also adds .old in var/log, in general you should not >have .conf files in conf.d if you do not want them used. > >Ignoring .something files should always be true as it makes it hard to >understand what is going on if you just do ls and can't see a snippet. > >> >>> In order for SSSD to load a configuration, all >> >>> the config snippets in /etc/sssd/conf.d/ and >> >>> /etc/sssd/sssd.conf must in combination >> >>> result in valid configuration. If there is an >> >>> error in processing one of the config files, >> >>> the whole configuration loading will be >> >>> unsuccessful and there will be no way to >> >>> skip problematic snippets (later we may add >> >>> a fallback config, but that is different issue). >> >>> >> > I do not remember the lib_iniconfig API. >> > But it might be already solved there. > >It is, question though: do you want to allow to override anything in a >snippet ? >At some point I had the idea the snippet would only allow to >define/override a single domain and the snippet name would have to be >domain_NAME.conf, but that may be overly restrictive given today we are >adding stuff like secrets which has a different way to deal with section >names. > Adding domains as a separate snippets is "kind of" a different use case. Because you also amend a "domains" option in the "[sssd]" section. It's solvable. Just add option "domain_enabled" into domain secition. Do we want to do it or do we want to keep it simple (current state)? >> >>> Of course sssd will have an cli option to >> >>> use alternative directory for snippets (similar >> >>> to what we use for config file now). >> >>> >> >>> Could it be implemented this way? >> >>> Any comments are welcome. >> >>> >> >>> Michal >> >> >> >> I forgot to notice one more thing. >> >> >> >> There can be conflicts in configuration >> >> among the snippets in /etc/sssd/conf.d directory. >> >> IMO the best thing to do in this situation is >> >> to end with an error. Defining priorities among >> >> the snippets could cause situations where people >> >> will write difficult to debug configurations. >> >> >> > In most, project you use numbers in the begining of files. >> > It guarantee the order and last value win. It's current behaviour >> > in sssd.conf. >> >> So should we rely on alphabetical order? I personally >> think it will add a little chaos to the configuration >> but maybe not. >> >> If we decide to rely on alphabetical order it may >> be nice to have a tool that will print the actual >> configuration in stdout skipping all the overriden options. >> This may be good for troubleshooting SSSD. > >We need an order so that admins can understand why a conflict happened, >alphabetical is as good as any. > +1 BTW logrotate was not ideal example because they do not care about order. >> > >> > We can also provide something like "systemctl cat name.service" >> > or we can even save parsed/merged.. config
[SSSD] Re: Design document - sssctl
On 03/22/2016 12:42 PM, Pavel Reichl wrote: Hello, Pavel Březina and I have prepared the 1st draft of design document. We mostly focused on summing up its future functionality and its interface. Please comment if you miss some essential functionality or if you would prefer some different interface. Thanks! https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl Hi, I would like to give a litle comment to: """ Uses cases Removing cache - Removing SSSD cache seems to be often misused act done by administrators as there are few real needs for that. Nevertheless, if administrator decides to remove the cache it would be better to do this using the tool instead of crude removing directories that might contain other useful data and could lead to serious problems. Q: Is this what was requested as 'force reload'? Q: Should this rather be part of sss_cache tool? """ There are two slightly related tickets: [1] sss_cache: invalidate sudo rules [2] Trigger full refresh of sudo rules on desire We have patch on list (tests remains to finish) for the first. The second is on my opinion candidate for sssctl. Reason is that is not only action of sss cache to obtain new updated data. Regards -- Petr^4 Čech [1] https://fedorahosted.org/sssd/ticket/2081 [2] https://fedorahosted.org/sssd/ticket/2884 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Design document - sssctl
On 03/22/2016 07:42 AM, Pavel Reichl wrote: > Hello, > > Pavel Březina and I have prepared the 1st draft of design document. We mostly > focused on summing up its future functionality and its interface. > > Please comment if you miss some essential functionality or if you would prefer > some different interface. > > Thanks! > > https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl "This tool will communicate with InfoPipe responder through its public D-Bus interface." Presumably this means that the sssctl CLI will be a thin presentation-layer shim over the D-BUS API (meaning that the CLI would contain no logic outside of argument processing)? This would be the ideal case, since it would also allow plugging in other front-ends for this (such as Cockpit). signature.asc Description: OpenPGP digital signature ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] GPO: log specific ini parse error messages
On (22/03/16 14:18), Michal Židek wrote:
>On 03/22/2016 10:45 AM, Michal Židek wrote:
>>On 03/21/2016 02:17 PM, Lukas Slebodnik wrote:
>>>On (16/03/16 16:50), Michal Židek wrote:
Hi,
sorry, I do not have working AD so I did
not test the patches. My testing
was only SSSD compilation :)
But I made a small ap that parses
ini files and treats the errors
the same way as in these patches
and it worked fine.
It prints the problematic line
with short error description
(like missing equal sign).
The patch is quite simple. It does not
solve any GPO issues, just logs found
parsing errors for easier debugging.
Michal
>>>
From 0bff25243b18406f88910a88705397365e2e37f1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?=
Date: Wed, 16 Mar 2016 16:38:34 +0100
Subject: [PATCH] GPO: log specific ini parse error messages
We should log error messages generated by
libini if there are problems with parsing
gpo files.
---
src/providers/ad/ad_gpo.c | 19 +++
src/providers/ad/ad_gpo_child.c | 19 +++
2 files changed, 38 insertions(+)
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 069196c..360aca5 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -1131,8 +1131,27 @@ ad_gpo_store_policy_settings(struct
sss_domain_info *domain,
ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0,
ini_config);
if (ret != 0) {
+int lret;
+char **errors;
+
DEBUG(SSSDBG_CRIT_FAILURE,
"ini_config_parse failed [%d][%s]\n", ret,
strerror(ret));
>>>
>>>Does it make sense to print also the filename?
>>>ini_config_get_filename()
>>>or is it logged somewhere else?
>>
>>It is not logged on the same level (and in gpo_child
>>it is not logged at all) so I think it makes sense to
>>add it to the debug message that informs about parse
>>failure. I updated the patch.
>>
>>>
>>>BTW you might not be able to do it in gpo_child
>>>because IIRC config file is parsed from memory (fmemopen)
>>>
>>
>>It is available in both cases. We call ini_config_file_open
>>just few lines above.
>>
>>Michal
>>
>>>LS
>>
>>
>
>Lukas noticed that there is missing newline in
>the debug message. I also shortened one of the
>messages because it contained uninteresting info.
>
>New patch attached.
>
>Michal
>
>From e252f76364d3db5446b34c50379dfda31cbb6aa9 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Michal=20=C5=BDidek?=
>Date: Wed, 16 Mar 2016 16:38:34 +0100
>Subject: [PATCH] GPO: log specific ini parse error messages
>
>We should log error messages generated by
>libini if there are problems with parsing
>gpo files.
>---
ACK
http://sssd-ci.duckdns.org/logs/job/39/74/summary.html
BTW I provided a test package with your patch
to user who had such issue. I hope it will help.
LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Design document - sssctl
This is great and will make our lives much easier in support! Currently we have autokeyed commands like 'service sssd stop; rm -f /var/lib/sss/db/*; service sssd stop' Once implemented I think we need update the sosreport plugin obtain sssctl output to be included in the sosreport. -Justin On 03/22/2016 08:47 AM, Pavel Reichl wrote: Hello, I'm adding some people, who might be interested in this tool, to CC list. (Please feel free to extend). On 03/22/2016 12:42 PM, Pavel Reichl wrote: Hello, Pavel Březina and I have prepared the 1st draft of design document. We mostly focused on summing up its future functionality and its interface. Please comment if you miss some essential functionality or if you would prefer some different interface. Thanks! https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Design document - sssctl
Thank you for the awesome work with it! On Tue, Mar 22, 2016 at 1:47 PM, Pavel Reichl wrote: > Hello, I'm adding some people, who might be interested in this tool, to CC > list. (Please feel free to extend). > > On 03/22/2016 12:42 PM, Pavel Reichl wrote: > >> Hello, >> >> Pavel Březina and I have prepared the 1st draft of design document. We >> mostly focused on summing up its future functionality and its interface. >> >> Please comment if you miss some essential functionality or if you would >> prefer some different interface. >> >> Thanks! >> >> https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl >> ___ >> sssd-devel mailing list >> sssd-devel@lists.fedorahosted.org >> >> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org >> > -- Thank you, George Agriogiannis Red Hat Support - EMEA ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Config file merging in SSSD
> On 22 Mar 2016, at 14:46, Lukas Slebodnik wrote: >>> >>> SSSD would automatically pick up files ending >>> in .conf from that direcory and use them. In >>> order to disable the config file, the admin will >>> have to rename the file ending (for example >>> .conf.disabled). This way, we do not need to >>> inspect the snippets for any special options >>> like 'enable_this_snippet = true' which would >>> just complicate the processing. >>> > Another, way how to ignore snippet is to ignore > any file which start with dot ".". > "hiddent files". It would avoid adding suffix to every file. > > BTW logrotate and crond do the same > /etc/logrotate.d/ > /etc/cron.d/ > +1 I would expect any decent software to ignore hidden files. The question is, should sssd ignore them or should libini_config? ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Config file merging in SSSD
> On 22 Mar 2016, at 20:43, Simo Sorce wrote: > > On Tue, 2016-03-22 at 15:15 +0100, Michal Židek wrote: >> On 03/22/2016 02:46 PM, Lukas Slebodnik wrote: >>> On (22/03/16 14:30), Michal Židek wrote: On 03/22/2016 12:29 PM, Michal Židek wrote: > Hi, > > I would like to write a patch that will > allow SSSD to use the config file merging > feature from libini. But first I would like > to ask developers for their opinions on how > this should be implemented. > > My idea was that it could work like this. > > The current /etc/sssd/sssd.conf would work > as usual. > > We would add new directory /etc/sssd/conf.d/ > and its content would be following: > - README file that informs what the direcotory > is for. > - any number of files ending with .conf extension > that would contain additional configuration for > SSSD. These files would have higher prioriry > than the /etc/sssd/sssd.conf , meaning if > the same option is present in these files > it will override the /etc/sssd/sssd.conf > value. > > What's the rational behind 'snippets vs main conf' makes snippet a > winner ? > The rationale is that the majority of systems tend to stick to a default, but there is one odd server that needs to connect to a different server instead of using SRV discovery by default. In that case, the admin would drop a snippet to the include directory with the help of Puppet or Ansible or similar and change the resulting config file on that single system w/o changing the authconfig/realmd/ipa-client-install that is usually set in the kickstart. (This is how I understand the use-case, feel free to come back with a counter-example..) ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Config file merging in SSSD
> On 22 Mar 2016, at 12:29, Michal Židek wrote: > > Hi, > > I would like to write a patch that will > allow SSSD to use the config file merging > feature from libini. But first I would like > to ask developers for their opinions on how > this should be implemented. > > My idea was that it could work like this. > > The current /etc/sssd/sssd.conf would work > as usual. > > We would add new directory /etc/sssd/conf.d/ > and its content would be following: > - README file that informs what the direcotory > is for. > - any number of files ending with .conf extension > that would contain additional configuration for > SSSD. These files would have higher prioriry > than the /etc/sssd/sssd.conf , meaning if > the same option is present in these files > it will override the /etc/sssd/sssd.conf > value. > > SSSD would automatically pick up files ending > in .conf from that direcory and use them. In > order to disable the config file, the admin will > have to rename the file ending (for example > .conf.disabled). This way, we do not need to > inspect the snippets for any special options > like 'enable_this_snippet = true' which would > just complicate the processing. > > In order for SSSD to load a configuration, all > the config snippets in /etc/sssd/conf.d/ and > /etc/sssd/sssd.conf must in combination > result in valid configuration. If there is an > error in processing one of the config files, > the whole configuration loading will be > unsuccessful and there will be no way to > skip problematic snippets (later we may add > a fallback config, but that is different issue). > I guess for now this is acceptable, because the snippet is similar to editing the conf file (as Sumit noted) but it would be nice to at least note in the design page of the fallback config and this merging feature that they are interconnected.. > Of course sssd will have an cli option to > use alternative directory for snippets (similar > to what we use for config file now). > Does anyone use the -c option actually (maybe except tests?) If not, do we need to add a new option? I'm cautious here, because any new option needs to be supported (almost) forever.. > Could it be implemented this way? > Any comments are welcome. > Would the admin be able to discover from debug message where an option comes from (conf file or snippet) ? Does libini provide that info? ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: NSS responder should negatively cache local users for a longer time
> On 22 Mar 2016, at 20:35, Simo Sorce wrote: > > On Sun, 2016-03-20 at 21:28 +0100, Jakub Hrozek wrote: >>> On 16 Mar 2016, at 13:45, Petr Cech wrote: >>> >>> Hi, >>> >>> I will work on $subject [1] and I have discussed this topic with >> Jakub a week ago. There are some open questions, so I will be glad if >> you say your opinion. >>> >>> There could be heavy traffic between SSSD client and server coused >> by local users. We need longer timeout in negative cache for local >> users. >>> >>> Questions are: >>> >>> a) Is better hack negative_cache or responder? >>> >> >> I would say that this solution should be reusable by other responders >> like ifp as well. Therefore I would say either negcache (but there I >> would say a new function, not extend the generic one) or a reusable >> function in responder/common. >> >>> b) Is better set timeout = 0 (it means permanently in negative >> cache) or set something really big like 12 hours? >>> * We couldn't remove local users from permanent negative cache (only >> by restart). >>> * Is timeout = 12 hours means some kind of network peak? >>> >> >> I guess some long timeout is slightly more flexible for cases where >> the admin would add the local user to LDAP groups. A couple of hours >> should be enough, as long as the negative entries are cached across >> all clients, then if a single client queries the server once a couple >> of hours, that should not bring the server down.. > > 12 hours is a lot, if you made a mistake and want to correct it (eg some > software install created a local user by mistake that the admin removes > because they want it in LDAP) you do not want to wait for hours. > > The main issue with the initgroups calls is not the load o the server, > but the slowdown of the call which ends up contacting a network (slower) > store for a local user. > > I would use a smaller timeout here, like 10 minutes, but potentially add > a midway cache check, like we do for positive results. so after 5 min, The admins actually complained about a load on the servers from users like postfix IIRC. That's the reason I suggested a long timeout. But I guess even a short timeout would work, but I would vote for defaulting to the entry_cache_timeout then (with a midway check perhaps..). I guess the biggest gain for admins would be to be able to specify a timeout for all passwd users at once and avoid putting them one-by-one into filter_users/filter_groups. > if a request comes in we do an asynchronous positive check to see if we > still need to extend the negative cache for another 10 minutes. If the > local user disappeared we drop the negative cache. > This is a good idea. >> btw do you think this feature should be enabled or disabled by >> default? > > Good q. how often does this problem happen ? > >>> c) Is it enough to do it only for initgroups? >> >> Hmm, not sure, by convention initgroups is the most frequent example >> (maybe there will be some users of the new libc merge feature), but at >> the same time special-casing initgroups doesn't gain much.. >> >> I guess I would personally do this for all lookups that the NSS >> interface can do (by name, by id) but I'm not 100% for or against >> either.. > > I agree, keep it generic. > > Simo. > > -- > Simo Sorce * Red Hat, Inc * New York Thank you for taking the time to review the design! ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] URI in HBAC rules - patch - request for feedback
Hi, FreeIPA and SSSD communities! I am working on adding URI to HBAC as my thesis [1]. The goal is to control access not only based on (user, host, service), but on (user, host, service, resource's URI). I created a patch for FreeIPA [2] so it is capable of storing URI as part of HBAC rule. I created a patch for SSSD [3] so it is able to get this URI from FreeIPA and use it in HBAC evaluation. I still need to develop a part of SSSD receiving URI-aware requests. It will either be an enhancement of Infopipe or I will use PAM responder (any suggestions?). I wanted to kindly ask you for review and your opinions on the patches and generally on my approach. This would be my first contribution to FreeIPA and SSSD so there might be bugs. What do you think? Btw, is there some better place to share patches than a pasting tool? Maybe some form of pull request? Thanks for your opinions! [1] https://diplomky.redhat.com/topic/show/326/store-and-manage-access-to-uris-in-freeipa [2] http://pastebin.com/rsHzXeAR [3] http://pastebin.com/atcZMuP1 -- Lukas Hellebrandt ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Config file merging in SSSD
On Tue, 2016-03-22 at 16:19 +0100, Michal Židek wrote: > On 03/22/2016 03:29 PM, Sumit Bose wrote: > > On Tue, Mar 22, 2016 at 12:29:39PM +0100, Michal Židek wrote: > >> Hi, > >> > >> I would like to write a patch that will > >> allow SSSD to use the config file merging > >> feature from libini. But first I would like > >> to ask developers for their opinions on how > >> this should be implemented. > >> > >> My idea was that it could work like this. > >> > >> The current /etc/sssd/sssd.conf would work > >> as usual. > >> > >> We would add new directory /etc/sssd/conf.d/ > >> and its content would be following: > >> - README file that informs what the direcotory > >> is for. > >> - any number of files ending with .conf extension > >>that would contain additional configuration for > >>SSSD. These files would have higher prioriry > >>than the /etc/sssd/sssd.conf , meaning if > >>the same option is present in these files > >>it will override the /etc/sssd/sssd.conf > >>value. > >> > >> SSSD would automatically pick up files ending > >> in .conf from that direcory and use them. In > >> order to disable the config file, the admin will > >> have to rename the file ending (for example > >> .conf.disabled). This way, we do not need to > >> inspect the snippets for any special options > >> like 'enable_this_snippet = true' which would > >> just complicate the processing. > >> > >> In order for SSSD to load a configuration, all > >> the config snippets in /etc/sssd/conf.d/ and > >> /etc/sssd/sssd.conf must in combination > >> result in valid configuration. If there is an > >> error in processing one of the config files, > >> the whole configuration loading will be > >> unsuccessful and there will be no way to > >> skip problematic snippets (later we may add > >> a fallback config, but that is different issue). > >> > >> Of course sssd will have an cli option to > >> use alternative directory for snippets (similar > >> to what we use for config file now). > >> > >> Could it be implemented this way? > >> Any comments are welcome. > > > > How will this interact with the python configuration API? If some > > application will use the API to change the configuration the result will > > be written to sssd.conf. But if there is a config snippets which sets > > the same value the change via the config API is overridden which I guess > > is unexpected by the application using the config API. > > > > bye, > > Sumit > > > > Good question. I was not thinking about this. We > could change the config API to actually write to its > own snippet that will be always applied last. > > OTOH some admins may want to really override whatever > other applications may set up using python config API. > > If we decide to apply snippets in alphabetical > order as Lukas suggested, we can give the snippet to which > python API writes a name for example 990_pyapi.conf > and if the admin decides to create snippet with starting > number lower than 990 it will have lower pririty than > python config API. > > We should document such behavior in the README file > that will be placed in the /etc/sssd/conf.d directory. If you make sssd.conf always win over snippets then you do not need to change anything, though. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Config file merging in SSSD
On Tue, 2016-03-22 at 15:15 +0100, Michal Židek wrote: > On 03/22/2016 02:46 PM, Lukas Slebodnik wrote: > > On (22/03/16 14:30), Michal Židek wrote: > >> On 03/22/2016 12:29 PM, Michal Židek wrote: > >>> Hi, > >>> > >>> I would like to write a patch that will > >>> allow SSSD to use the config file merging > >>> feature from libini. But first I would like > >>> to ask developers for their opinions on how > >>> this should be implemented. > >>> > >>> My idea was that it could work like this. > >>> > >>> The current /etc/sssd/sssd.conf would work > >>> as usual. > >>> > >>> We would add new directory /etc/sssd/conf.d/ > >>> and its content would be following: > >>> - README file that informs what the direcotory > >>> is for. > >>> - any number of files ending with .conf extension > >>>that would contain additional configuration for > >>>SSSD. These files would have higher prioriry > >>>than the /etc/sssd/sssd.conf , meaning if > >>>the same option is present in these files > >>>it will override the /etc/sssd/sssd.conf > >>>value. What's the rational behind 'snippets vs main conf' makes snippet a winner ? > >>> SSSD would automatically pick up files ending > >>> in .conf from that direcory and use them. In > >>> order to disable the config file, the admin will > >>> have to rename the file ending (for example > >>> .conf.disabled). This way, we do not need to > >>> inspect the snippets for any special options > >>> like 'enable_this_snippet = true' which would > >>> just complicate the processing. > >>> > > Another, way how to ignore snippet is to ignore > > any file which start with dot ".". > > "hiddent files". It would avoid adding suffix to every file. > > > > BTW logrotate and crond do the same > > /etc/logrotate.d/ > > /etc/cron.d/ logrotate also adds .old in var/log, in general you should not have .conf files in conf.d if you do not want them used. Ignoring .something files should always be true as it makes it hard to understand what is going on if you just do ls and can't see a snippet. > >>> In order for SSSD to load a configuration, all > >>> the config snippets in /etc/sssd/conf.d/ and > >>> /etc/sssd/sssd.conf must in combination > >>> result in valid configuration. If there is an > >>> error in processing one of the config files, > >>> the whole configuration loading will be > >>> unsuccessful and there will be no way to > >>> skip problematic snippets (later we may add > >>> a fallback config, but that is different issue). > >>> > > I do not remember the lib_iniconfig API. > > But it might be already solved there. It is, question though: do you want to allow to override anything in a snippet ? At some point I had the idea the snippet would only allow to define/override a single domain and the snippet name would have to be domain_NAME.conf, but that may be overly restrictive given today we are adding stuff like secrets which has a different way to deal with section names. > >>> Of course sssd will have an cli option to > >>> use alternative directory for snippets (similar > >>> to what we use for config file now). > >>> > >>> Could it be implemented this way? > >>> Any comments are welcome. > >>> > >>> Michal > >> > >> I forgot to notice one more thing. > >> > >> There can be conflicts in configuration > >> among the snippets in /etc/sssd/conf.d directory. > >> IMO the best thing to do in this situation is > >> to end with an error. Defining priorities among > >> the snippets could cause situations where people > >> will write difficult to debug configurations. > >> > > In most, project you use numbers in the begining of files. > > It guarantee the order and last value win. It's current behaviour > > in sssd.conf. > > So should we rely on alphabetical order? I personally > think it will add a little chaos to the configuration > but maybe not. > > If we decide to rely on alphabetical order it may > be nice to have a tool that will print the actual > configuration in stdout skipping all the overriden options. > This may be good for troubleshooting SSSD. We need an order so that admins can understand why a conflict happened, alphabetical is as good as any. > > > > We can also provide something like "systemctl cat name.service" > > or we can even save parsed/merged.. configuration with > > ini_config_save_as. > > > > What do you think? > > It sounds good to me. The condition for a conf file could be: > (ends with '.conf') and (does not start with '.') > everything else will be ignored. > > > > > BTW you might also look into gssproxy how they implemented it. > > They already uses this feature from lib_iniconfig IIRC. We do, indeed. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: NSS responder should negatively cache local users for a longer time
On Sun, 2016-03-20 at 21:28 +0100, Jakub Hrozek wrote: > > On 16 Mar 2016, at 13:45, Petr Cech wrote: > > > > Hi, > > > > I will work on $subject [1] and I have discussed this topic with > Jakub a week ago. There are some open questions, so I will be glad if > you say your opinion. > > > > There could be heavy traffic between SSSD client and server coused > by local users. We need longer timeout in negative cache for local > users. > > > > Questions are: > > > > a) Is better hack negative_cache or responder? > > > > I would say that this solution should be reusable by other responders > like ifp as well. Therefore I would say either negcache (but there I > would say a new function, not extend the generic one) or a reusable > function in responder/common. > > > b) Is better set timeout = 0 (it means permanently in negative > cache) or set something really big like 12 hours? > > * We couldn't remove local users from permanent negative cache (only > by restart). > > * Is timeout = 12 hours means some kind of network peak? > > > > I guess some long timeout is slightly more flexible for cases where > the admin would add the local user to LDAP groups. A couple of hours > should be enough, as long as the negative entries are cached across > all clients, then if a single client queries the server once a couple > of hours, that should not bring the server down.. 12 hours is a lot, if you made a mistake and want to correct it (eg some software install created a local user by mistake that the admin removes because they want it in LDAP) you do not want to wait for hours. The main issue with the initgroups calls is not the load o the server, but the slowdown of the call which ends up contacting a network (slower) store for a local user. I would use a smaller timeout here, like 10 minutes, but potentially add a midway cache check, like we do for positive results. so after 5 min, if a request comes in we do an asynchronous positive check to see if we still need to extend the negative cache for another 10 minutes. If the local user disappeared we drop the negative cache. > btw do you think this feature should be enabled or disabled by > default? Good q. how often does this problem happen ? > > c) Is it enough to do it only for initgroups? > > Hmm, not sure, by convention initgroups is the most frequent example > (maybe there will be some users of the new libc merge feature), but at > the same time special-casing initgroups doesn't gain much.. > > I guess I would personally do this for all lookups that the NSS > interface can do (by name, by id) but I'm not 100% for or against > either.. I agree, keep it generic. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Config file merging in SSSD
On 03/22/2016 03:29 PM, Sumit Bose wrote: On Tue, Mar 22, 2016 at 12:29:39PM +0100, Michal Židek wrote: Hi, I would like to write a patch that will allow SSSD to use the config file merging feature from libini. But first I would like to ask developers for their opinions on how this should be implemented. My idea was that it could work like this. The current /etc/sssd/sssd.conf would work as usual. We would add new directory /etc/sssd/conf.d/ and its content would be following: - README file that informs what the direcotory is for. - any number of files ending with .conf extension that would contain additional configuration for SSSD. These files would have higher prioriry than the /etc/sssd/sssd.conf , meaning if the same option is present in these files it will override the /etc/sssd/sssd.conf value. SSSD would automatically pick up files ending in .conf from that direcory and use them. In order to disable the config file, the admin will have to rename the file ending (for example .conf.disabled). This way, we do not need to inspect the snippets for any special options like 'enable_this_snippet = true' which would just complicate the processing. In order for SSSD to load a configuration, all the config snippets in /etc/sssd/conf.d/ and /etc/sssd/sssd.conf must in combination result in valid configuration. If there is an error in processing one of the config files, the whole configuration loading will be unsuccessful and there will be no way to skip problematic snippets (later we may add a fallback config, but that is different issue). Of course sssd will have an cli option to use alternative directory for snippets (similar to what we use for config file now). Could it be implemented this way? Any comments are welcome. How will this interact with the python configuration API? If some application will use the API to change the configuration the result will be written to sssd.conf. But if there is a config snippets which sets the same value the change via the config API is overridden which I guess is unexpected by the application using the config API. bye, Sumit Good question. I was not thinking about this. We could change the config API to actually write to its own snippet that will be always applied last. OTOH some admins may want to really override whatever other applications may set up using python config API. If we decide to apply snippets in alphabetical order as Lukas suggested, we can give the snippet to which python API writes a name for example 990_pyapi.conf and if the admin decides to create snippet with starting number lower than 990 it will have lower pririty than python config API. We should document such behavior in the README file that will be placed in the /etc/sssd/conf.d directory. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Config file merging in SSSD
On Tue, Mar 22, 2016 at 12:29:39PM +0100, Michal Židek wrote: > Hi, > > I would like to write a patch that will > allow SSSD to use the config file merging > feature from libini. But first I would like > to ask developers for their opinions on how > this should be implemented. > > My idea was that it could work like this. > > The current /etc/sssd/sssd.conf would work > as usual. > > We would add new directory /etc/sssd/conf.d/ > and its content would be following: > - README file that informs what the direcotory > is for. > - any number of files ending with .conf extension > that would contain additional configuration for > SSSD. These files would have higher prioriry > than the /etc/sssd/sssd.conf , meaning if > the same option is present in these files > it will override the /etc/sssd/sssd.conf > value. > > SSSD would automatically pick up files ending > in .conf from that direcory and use them. In > order to disable the config file, the admin will > have to rename the file ending (for example > .conf.disabled). This way, we do not need to > inspect the snippets for any special options > like 'enable_this_snippet = true' which would > just complicate the processing. > > In order for SSSD to load a configuration, all > the config snippets in /etc/sssd/conf.d/ and > /etc/sssd/sssd.conf must in combination > result in valid configuration. If there is an > error in processing one of the config files, > the whole configuration loading will be > unsuccessful and there will be no way to > skip problematic snippets (later we may add > a fallback config, but that is different issue). > > Of course sssd will have an cli option to > use alternative directory for snippets (similar > to what we use for config file now). > > Could it be implemented this way? > Any comments are welcome. How will this interact with the python configuration API? If some application will use the API to change the configuration the result will be written to sssd.conf. But if there is a config snippets which sets the same value the change via the config API is overridden which I guess is unexpected by the application using the config API. bye, Sumit > > Michal > ___ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Config file merging in SSSD
On 03/22/2016 02:46 PM, Lukas Slebodnik wrote: On (22/03/16 14:30), Michal Židek wrote: On 03/22/2016 12:29 PM, Michal Židek wrote: Hi, I would like to write a patch that will allow SSSD to use the config file merging feature from libini. But first I would like to ask developers for their opinions on how this should be implemented. My idea was that it could work like this. The current /etc/sssd/sssd.conf would work as usual. We would add new directory /etc/sssd/conf.d/ and its content would be following: - README file that informs what the direcotory is for. - any number of files ending with .conf extension that would contain additional configuration for SSSD. These files would have higher prioriry than the /etc/sssd/sssd.conf , meaning if the same option is present in these files it will override the /etc/sssd/sssd.conf value. SSSD would automatically pick up files ending in .conf from that direcory and use them. In order to disable the config file, the admin will have to rename the file ending (for example .conf.disabled). This way, we do not need to inspect the snippets for any special options like 'enable_this_snippet = true' which would just complicate the processing. Another, way how to ignore snippet is to ignore any file which start with dot ".". "hiddent files". It would avoid adding suffix to every file. BTW logrotate and crond do the same /etc/logrotate.d/ /etc/cron.d/ In order for SSSD to load a configuration, all the config snippets in /etc/sssd/conf.d/ and /etc/sssd/sssd.conf must in combination result in valid configuration. If there is an error in processing one of the config files, the whole configuration loading will be unsuccessful and there will be no way to skip problematic snippets (later we may add a fallback config, but that is different issue). I do not remember the lib_iniconfig API. But it might be already solved there. Of course sssd will have an cli option to use alternative directory for snippets (similar to what we use for config file now). Could it be implemented this way? Any comments are welcome. Michal I forgot to notice one more thing. There can be conflicts in configuration among the snippets in /etc/sssd/conf.d directory. IMO the best thing to do in this situation is to end with an error. Defining priorities among the snippets could cause situations where people will write difficult to debug configurations. In most, project you use numbers in the begining of files. It guarantee the order and last value win. It's current behaviour in sssd.conf. So should we rely on alphabetical order? I personally think it will add a little chaos to the configuration but maybe not. If we decide to rely on alphabetical order it may be nice to have a tool that will print the actual configuration in stdout skipping all the overriden options. This may be good for troubleshooting SSSD. We can also provide something like "systemctl cat name.service" or we can even save parsed/merged.. configuration with ini_config_save_as. What do you think? It sounds good to me. The condition for a conf file could be: (ends with '.conf') and (does not start with '.') everything else will be ignored. BTW you might also look into gssproxy how they implemented it. They already uses this feature from lib_iniconfig IIRC. LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Config file merging in SSSD
On (22/03/16 14:30), Michal Židek wrote: >On 03/22/2016 12:29 PM, Michal Židek wrote: >>Hi, >> >>I would like to write a patch that will >>allow SSSD to use the config file merging >>feature from libini. But first I would like >>to ask developers for their opinions on how >>this should be implemented. >> >>My idea was that it could work like this. >> >>The current /etc/sssd/sssd.conf would work >>as usual. >> >>We would add new directory /etc/sssd/conf.d/ >>and its content would be following: >>- README file that informs what the direcotory >>is for. >>- any number of files ending with .conf extension >> that would contain additional configuration for >> SSSD. These files would have higher prioriry >> than the /etc/sssd/sssd.conf , meaning if >> the same option is present in these files >> it will override the /etc/sssd/sssd.conf >> value. >> >>SSSD would automatically pick up files ending >>in .conf from that direcory and use them. In >>order to disable the config file, the admin will >>have to rename the file ending (for example >>.conf.disabled). This way, we do not need to >>inspect the snippets for any special options >>like 'enable_this_snippet = true' which would >>just complicate the processing. >> Another, way how to ignore snippet is to ignore any file which start with dot ".". "hiddent files". It would avoid adding suffix to every file. BTW logrotate and crond do the same /etc/logrotate.d/ /etc/cron.d/ >>In order for SSSD to load a configuration, all >>the config snippets in /etc/sssd/conf.d/ and >>/etc/sssd/sssd.conf must in combination >>result in valid configuration. If there is an >>error in processing one of the config files, >>the whole configuration loading will be >>unsuccessful and there will be no way to >>skip problematic snippets (later we may add >>a fallback config, but that is different issue). >> I do not remember the lib_iniconfig API. But it might be already solved there. >>Of course sssd will have an cli option to >>use alternative directory for snippets (similar >>to what we use for config file now). >> >>Could it be implemented this way? >>Any comments are welcome. >> >>Michal > >I forgot to notice one more thing. > >There can be conflicts in configuration >among the snippets in /etc/sssd/conf.d directory. >IMO the best thing to do in this situation is >to end with an error. Defining priorities among >the snippets could cause situations where people >will write difficult to debug configurations. > In most, project you use numbers in the begining of files. It guarantee the order and last value win. It's current behaviour in sssd.conf. We can also provide something like "systemctl cat name.service" or we can even save parsed/merged.. configuration with ini_config_save_as. What do you think? BTW you might also look into gssproxy how they implemented it. They already uses this feature from lib_iniconfig IIRC. LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Config file merging in SSSD
On 03/22/2016 12:29 PM, Michal Židek wrote: Hi, I would like to write a patch that will allow SSSD to use the config file merging feature from libini. But first I would like to ask developers for their opinions on how this should be implemented. My idea was that it could work like this. The current /etc/sssd/sssd.conf would work as usual. We would add new directory /etc/sssd/conf.d/ and its content would be following: - README file that informs what the direcotory is for. - any number of files ending with .conf extension that would contain additional configuration for SSSD. These files would have higher prioriry than the /etc/sssd/sssd.conf , meaning if the same option is present in these files it will override the /etc/sssd/sssd.conf value. SSSD would automatically pick up files ending in .conf from that direcory and use them. In order to disable the config file, the admin will have to rename the file ending (for example .conf.disabled). This way, we do not need to inspect the snippets for any special options like 'enable_this_snippet = true' which would just complicate the processing. In order for SSSD to load a configuration, all the config snippets in /etc/sssd/conf.d/ and /etc/sssd/sssd.conf must in combination result in valid configuration. If there is an error in processing one of the config files, the whole configuration loading will be unsuccessful and there will be no way to skip problematic snippets (later we may add a fallback config, but that is different issue). Of course sssd will have an cli option to use alternative directory for snippets (similar to what we use for config file now). Could it be implemented this way? Any comments are welcome. Michal I forgot to notice one more thing. There can be conflicts in configuration among the snippets in /etc/sssd/conf.d directory. IMO the best thing to do in this situation is to end with an error. Defining priorities among the snippets could cause situations where people will write difficult to debug configurations. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] GPO: log specific ini parse error messages
On 03/22/2016 10:45 AM, Michal Židek wrote:
On 03/21/2016 02:17 PM, Lukas Slebodnik wrote:
On (16/03/16 16:50), Michal Židek wrote:
Hi,
sorry, I do not have working AD so I did
not test the patches. My testing
was only SSSD compilation :)
But I made a small ap that parses
ini files and treats the errors
the same way as in these patches
and it worked fine.
It prints the problematic line
with short error description
(like missing equal sign).
The patch is quite simple. It does not
solve any GPO issues, just logs found
parsing errors for easier debugging.
Michal
From 0bff25243b18406f88910a88705397365e2e37f1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?=
Date: Wed, 16 Mar 2016 16:38:34 +0100
Subject: [PATCH] GPO: log specific ini parse error messages
We should log error messages generated by
libini if there are problems with parsing
gpo files.
---
src/providers/ad/ad_gpo.c | 19 +++
src/providers/ad/ad_gpo_child.c | 19 +++
2 files changed, 38 insertions(+)
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 069196c..360aca5 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -1131,8 +1131,27 @@ ad_gpo_store_policy_settings(struct
sss_domain_info *domain,
ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0,
ini_config);
if (ret != 0) {
+int lret;
+char **errors;
+
DEBUG(SSSDBG_CRIT_FAILURE,
"ini_config_parse failed [%d][%s]\n", ret,
strerror(ret));
Does it make sense to print also the filename?
ini_config_get_filename()
or is it logged somewhere else?
It is not logged on the same level (and in gpo_child
it is not logged at all) so I think it makes sense to
add it to the debug message that informs about parse
failure. I updated the patch.
BTW you might not be able to do it in gpo_child
because IIRC config file is parsed from memory (fmemopen)
It is available in both cases. We call ini_config_file_open
just few lines above.
Michal
LS
Lukas noticed that there is missing newline in
the debug message. I also shortened one of the
messages because it contained uninteresting info.
New patch attached.
Michal
>From e252f76364d3db5446b34c50379dfda31cbb6aa9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?=
Date: Wed, 16 Mar 2016 16:38:34 +0100
Subject: [PATCH] GPO: log specific ini parse error messages
We should log error messages generated by
libini if there are problems with parsing
gpo files.
---
src/providers/ad/ad_gpo.c | 21 -
src/providers/ad/ad_gpo_child.c | 21 -
2 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 78ff723..3bd9ab0 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -1138,8 +1138,27 @@ ad_gpo_store_policy_settings(struct sss_domain_info *domain,
ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0, ini_config);
if (ret != 0) {
+int lret;
+char **errors;
+
DEBUG(SSSDBG_CRIT_FAILURE,
- "ini_config_parse failed [%d][%s]\n", ret, strerror(ret));
+ "[%s]: ini_config_parse failed [%d][%s]\n",
+ filename, ret, strerror(ret));
+
+/* Now get specific errors if there are any */
+lret = ini_config_get_errors(ini_config, &errors);
+if (lret != 0) {
+DEBUG(SSSDBG_CRIT_FAILURE,
+ "Failed to get specific parse error [%d][%s]\n", lret,
+ strerror(lret));
+goto done;
+}
+
+for (int a = 0; errors[a]; a++) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "%s\n", errors[a]);
+}
+ini_config_free_errors(errors);
+
goto done;
}
diff --git a/src/providers/ad/ad_gpo_child.c b/src/providers/ad/ad_gpo_child.c
index 1b59971..668af05 100644
--- a/src/providers/ad/ad_gpo_child.c
+++ b/src/providers/ad/ad_gpo_child.c
@@ -463,8 +463,27 @@ ad_gpo_parse_ini_file(const char *smb_path,
ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0, ini_config);
if (ret != 0) {
+int lret;
+char **errors;
+
DEBUG(SSSDBG_CRIT_FAILURE,
- "ini_config_parse failed [%d][%s]\n", ret, strerror(ret));
+ "[%s]: ini_config_parse failed [%d][%s]\n",
+ ini_filename, ret, strerror(ret));
+
+/* Now get specific errors if there are any */
+lret = ini_config_get_errors(ini_config, &errors);
+if (lret != 0) {
+DEBUG(SSSDBG_CRIT_FAILURE,
+ "Failed to get specific parse error [%d][%s]\n", lret,
+ strerror(lret));
+goto done;
+}
+
+for (int i = 0; errors[i]; i++) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "%s\n", errors[i]);
+}
+ini_config_free_errors(errors);
+
goto done;
}
--
2.5.0
___
[SSSD] Re: Design document - sssctl
Hello, I'm adding some people, who might be interested in this tool, to CC list. (Please feel free to extend). On 03/22/2016 12:42 PM, Pavel Reichl wrote: Hello, Pavel Březina and I have prepared the 1st draft of design document. We mostly focused on summing up its future functionality and its interface. Please comment if you miss some essential functionality or if you would prefer some different interface. Thanks! https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Design document - sssctl
Hello, Pavel Březina and I have prepared the 1st draft of design document. We mostly focused on summing up its future functionality and its interface. Please comment if you miss some essential functionality or if you would prefer some different interface. Thanks! https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Config file merging in SSSD
Hi, I would like to write a patch that will allow SSSD to use the config file merging feature from libini. But first I would like to ask developers for their opinions on how this should be implemented. My idea was that it could work like this. The current /etc/sssd/sssd.conf would work as usual. We would add new directory /etc/sssd/conf.d/ and its content would be following: - README file that informs what the direcotory is for. - any number of files ending with .conf extension that would contain additional configuration for SSSD. These files would have higher prioriry than the /etc/sssd/sssd.conf , meaning if the same option is present in these files it will override the /etc/sssd/sssd.conf value. SSSD would automatically pick up files ending in .conf from that direcory and use them. In order to disable the config file, the admin will have to rename the file ending (for example .conf.disabled). This way, we do not need to inspect the snippets for any special options like 'enable_this_snippet = true' which would just complicate the processing. In order for SSSD to load a configuration, all the config snippets in /etc/sssd/conf.d/ and /etc/sssd/sssd.conf must in combination result in valid configuration. If there is an error in processing one of the config files, the whole configuration loading will be unsuccessful and there will be no way to skip problematic snippets (later we may add a fallback config, but that is different issue). Of course sssd will have an cli option to use alternative directory for snippets (similar to what we use for config file now). Could it be implemented this way? Any comments are welcome. Michal ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH SET] TOOL: Invalidation of sudo rules at sss_cache
On (22/03/16 11:20), Pavel Březina wrote:
>On 03/22/2016 11:05 AM, Lukas Slebodnik wrote:
>>On (22/03/16 10:37), Pavel Březina wrote:
>>>On 03/21/2016 02:06 PM, Lukas Slebodnik wrote:
On (21/03/16 13:51), Pavel Březina wrote:
>On 03/21/2016 01:10 PM, Lukas Slebodnik wrote:
>>On (21/03/16 11:02), Petr Cech wrote:
>>>On 03/16/2016 12:37 PM, Pavel Březina wrote:
>>>
>>>[...]
>>>
*Patch 2: SYSDB: Add new funtions into sysdb_sudo*
>+errno_t
>+sysdb_set_sudo_rule_attr(struct sss_domain_info *domain,
>+ const char *name,
>+ struct sysdb_attrs *attrs,
>+ int mod_op)
>+{
>+errno_t ret;
>+struct ldb_dn *dn;
>+TALLOC_CTX *tmp_ctx;
>+
>+tmp_ctx = talloc_new(NULL);
>+if (tmp_ctx == NULL) {
>+return ENOMEM;
>+}
>+
>+dn = sysdb_sudo_rule_dn(tmp_ctx, domain, name);
>+NULL_CHECK(dn, ret, done);
>+
>+ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
>+
>+done:
>+talloc_free(tmp_ctx);
>+return ret;
>>>Temporary context is not needed here, you can use NULL instead and
>>>talloc_free(dn) in the end.
>>
>>I am not sure about it. I have tried it but it didn't work for me.
>>
What didn't work for you?
errno_t
sysdb_set_sudo_rule_attr(struct sss_domain_info *domain,
const char *name,
struct sysdb_attrs *attrs,
int mod_op)
{
struct ldb_dn *dn;
errno_t ret;
dn = sysdb_sudo_rule_dn(NULL, domain, name);
if (dn == NULL) {
return ENOMEM;
}
ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
talloc_free(dn);
return ret;
}
>>>
>>>Hi list,
>>>
>>>I think that I find bug in ldb-1.1.25:
>>>
>>>
>>>So, I have done little investigation.
>>>
>>>In short, backtrace in gdb says:
>>>
>>>#0 ldb_dn_new_fmt (mem_ctx=0x0, ldb=0x6578f0, new_fmt=0x779b3538
>>>"name=%s,cn=%s,cn=custom,cn=%s,cn=sysdb") at ../common/ldb_dn.c:174
>>>#1 0x7796727c in sysdb_custom_dn (mem_ctx=0x0, dom=0x658500,
>>>object_name=0x409f34 "test_rule3", subtree_name=0x40aecf "sudorules") at
>>>../src/db/sysdb.c:148
>>>#2 0x004073e8 in sysdb_sudo_rule_dn (mem_ctx=0x0,
>>>domain=0x658500,
>>>name=0x409f34 "test_rule3") at ../src/db/sysdb_sudo.c:962
>>>#3 0x00407416 in sysdb_set_sudo_rule_attr (domain=0x658500,
>>>name=0x409f34 "test_rule3", attrs=0x658390, mod_op=2) at
>>>../src/db/sysdb_sudo.c:974
>>>#4 0x0040458b in test_set_sudo_rule_attr_exist (state=0x614a90)
>>>at
>>>../src/tests/cmocka/test_sysdb_sudo.c:400
>>>#5 0x77bd6659 in cmocka_run_one_test_or_fixture
>>>(function_name=0x40a8e9 "test_set_sudo_rule_attr_exist",
>>>test_func=0x4044d7
>>>, setup_func=setup_func@entry=0x0,
>>>teardown_func=teardown_func@entry=0x0, state=state@entry=0x614a90,
>>>heap_check_point=heap_check_point@entry=0x0) at
>>>/usr/src/debug/cmocka-1.0.1/src/cmocka.c:2305
>>>#6 0x77bd6d8f in cmocka_run_one_tests (test_state=0x614a80) at
>>>/usr/src/debug/cmocka-1.0.1/src/cmocka.c:2413
>>>#7 _cmocka_run_group_tests (group_name=0x40a823 "tests", tests=>>out>, num_tests=15, group_setup=, group_teardown=0x0) at
>>>/usr/src/debug/cmocka-1.0.1/src/cmocka.c:2518
>>>#8 0x00404d7a in main (argc=1, argv=0x7fffe3f8) at
>>>../src/tests/cmocka/test_sysdb_sudo.c:645
>>>
>>>
>>>On the line
>>>/usr/src/debug/ldb-1.1.25/common/ldb_dn.c:174:
>>>
>>>167 struct ldb_dn *ldb_dn_new_fmt(TALLOC_CTX *mem_ctx,
>>>168 struct ldb_context *ldb,
>>>169 const char *new_fmt, ...)
>>>170 {
>>>171 char *strdn;
>>>172 va_list ap;
>>>173
>>>174 if ( (! mem_ctx) || (! ldb)) return NULL;
>>>175
>>>176 va_start(ap, new_fmt);
>>>177 strdn = talloc_vasprintf(mem_ctx, new_fmt, ap);
>>>178 va_end(ap);
>>>179
>>>180 if (strdn) {
>>>181 struct ldb_dn *dn = ldb_dn_new(mem_ctx, ldb, strdn);
>>>182 talloc_free(strdn);
>>>183 return dn;
>>>184 }
>>>185
>>>186 return NULL;
>>>187 }
>>>188
>>>
>>>How you can see, if we send NULL to sysdb_sudo_rule_dn, it will explode
>>>very
>>>badly :-)
>>>
>>It might be a bug. But we should try to avoid using NULL context
>>
>>We had some memory leaks caused by allocating something on NULL context.
>>
>>LS
>
>Except in this case this is the same thing
[SSSD] Re: [PATCH SET] TOOL: Invalidation of sudo rules at sss_cache
On 03/22/2016 11:05 AM, Lukas Slebodnik wrote:
On (22/03/16 10:37), Pavel Březina wrote:
On 03/21/2016 02:06 PM, Lukas Slebodnik wrote:
On (21/03/16 13:51), Pavel Březina wrote:
On 03/21/2016 01:10 PM, Lukas Slebodnik wrote:
On (21/03/16 11:02), Petr Cech wrote:
On 03/16/2016 12:37 PM, Pavel Březina wrote:
[...]
*Patch 2: SYSDB: Add new funtions into sysdb_sudo*
+errno_t
+sysdb_set_sudo_rule_attr(struct sss_domain_info *domain,
+ const char *name,
+ struct sysdb_attrs *attrs,
+ int mod_op)
+{
+errno_t ret;
+struct ldb_dn *dn;
+TALLOC_CTX *tmp_ctx;
+
+tmp_ctx = talloc_new(NULL);
+if (tmp_ctx == NULL) {
+return ENOMEM;
+}
+
+dn = sysdb_sudo_rule_dn(tmp_ctx, domain, name);
+NULL_CHECK(dn, ret, done);
+
+ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
+
+done:
+talloc_free(tmp_ctx);
+return ret;
Temporary context is not needed here, you can use NULL instead and
talloc_free(dn) in the end.
I am not sure about it. I have tried it but it didn't work for me.
What didn't work for you?
errno_t
sysdb_set_sudo_rule_attr(struct sss_domain_info *domain,
const char *name,
struct sysdb_attrs *attrs,
int mod_op)
{
struct ldb_dn *dn;
errno_t ret;
dn = sysdb_sudo_rule_dn(NULL, domain, name);
if (dn == NULL) {
return ENOMEM;
}
ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
talloc_free(dn);
return ret;
}
Hi list,
I think that I find bug in ldb-1.1.25:
So, I have done little investigation.
In short, backtrace in gdb says:
#0 ldb_dn_new_fmt (mem_ctx=0x0, ldb=0x6578f0, new_fmt=0x779b3538
"name=%s,cn=%s,cn=custom,cn=%s,cn=sysdb") at ../common/ldb_dn.c:174
#1 0x7796727c in sysdb_custom_dn (mem_ctx=0x0, dom=0x658500,
object_name=0x409f34 "test_rule3", subtree_name=0x40aecf "sudorules") at
../src/db/sysdb.c:148
#2 0x004073e8 in sysdb_sudo_rule_dn (mem_ctx=0x0, domain=0x658500,
name=0x409f34 "test_rule3") at ../src/db/sysdb_sudo.c:962
#3 0x00407416 in sysdb_set_sudo_rule_attr (domain=0x658500,
name=0x409f34 "test_rule3", attrs=0x658390, mod_op=2) at
../src/db/sysdb_sudo.c:974
#4 0x0040458b in test_set_sudo_rule_attr_exist (state=0x614a90) at
../src/tests/cmocka/test_sysdb_sudo.c:400
#5 0x77bd6659 in cmocka_run_one_test_or_fixture
(function_name=0x40a8e9 "test_set_sudo_rule_attr_exist", test_func=0x4044d7
, setup_func=setup_func@entry=0x0,
teardown_func=teardown_func@entry=0x0, state=state@entry=0x614a90,
heap_check_point=heap_check_point@entry=0x0) at
/usr/src/debug/cmocka-1.0.1/src/cmocka.c:2305
#6 0x77bd6d8f in cmocka_run_one_tests (test_state=0x614a80) at
/usr/src/debug/cmocka-1.0.1/src/cmocka.c:2413
#7 _cmocka_run_group_tests (group_name=0x40a823 "tests", tests=, num_tests=15, group_setup=, group_teardown=0x0) at
/usr/src/debug/cmocka-1.0.1/src/cmocka.c:2518
#8 0x00404d7a in main (argc=1, argv=0x7fffe3f8) at
../src/tests/cmocka/test_sysdb_sudo.c:645
On the line
/usr/src/debug/ldb-1.1.25/common/ldb_dn.c:174:
167 struct ldb_dn *ldb_dn_new_fmt(TALLOC_CTX *mem_ctx,
168 struct ldb_context *ldb,
169 const char *new_fmt, ...)
170 {
171 char *strdn;
172 va_list ap;
173
174 if ( (! mem_ctx) || (! ldb)) return NULL;
175
176 va_start(ap, new_fmt);
177 strdn = talloc_vasprintf(mem_ctx, new_fmt, ap);
178 va_end(ap);
179
180 if (strdn) {
181 struct ldb_dn *dn = ldb_dn_new(mem_ctx, ldb, strdn);
182 talloc_free(strdn);
183 return dn;
184 }
185
186 return NULL;
187 }
188
How you can see, if we send NULL to sysdb_sudo_rule_dn, it will explode very
badly :-)
It might be a bug. But we should try to avoid using NULL context
We had some memory leaks caused by allocating something on NULL context.
LS
Except in this case this is the same thing as using temporary context but
shorter.
If you want to avoid creating tmp_ctx you can still use "ldb"
as talloc context and you will not hit bug in ldb_dn_new_fmt.
I agree with you with this as a workaround to avoid creating new talloc
context here.
If you extend this function and possibly create memory leak
it will be easier to find it.
Actually no, finding memory leaked on NULL context is easier than finding it
on existing talloc context. Nothing but top level context is supposed to be
attached to NULL context so if there is anything else we have a leak, it's
simple as that. Using existing talloc context makes it harder to find and it
may not even show because you free it somewhere in the process.
It needn't be simpler.
Why?
some_ctx = NULL;
fun1(some_ctx, ) {
fun2(some_ctx, ...);
}
and you leak some memory in fun2.
How simple would be find such memory leak?
One thing is to find location of the leak, another thing is to detect
that a leak exist and what data are we leaking. I am not going to spend
more time on thi
[SSSD] Re: [PATCH SET] TOOL: Invalidation of sudo rules at sss_cache
On (22/03/16 10:37), Pavel Březina wrote:
>On 03/21/2016 02:06 PM, Lukas Slebodnik wrote:
>>On (21/03/16 13:51), Pavel Březina wrote:
>>>On 03/21/2016 01:10 PM, Lukas Slebodnik wrote:
On (21/03/16 11:02), Petr Cech wrote:
>On 03/16/2016 12:37 PM, Pavel Březina wrote:
>
>[...]
>
>>*Patch 2: SYSDB: Add new funtions into sysdb_sudo*
>>
>>>+errno_t
>>>+sysdb_set_sudo_rule_attr(struct sss_domain_info *domain,
>>>+ const char *name,
>>>+ struct sysdb_attrs *attrs,
>>>+ int mod_op)
>>>+{
>>>+errno_t ret;
>>>+struct ldb_dn *dn;
>>>+TALLOC_CTX *tmp_ctx;
>>>+
>>>+tmp_ctx = talloc_new(NULL);
>>>+if (tmp_ctx == NULL) {
>>>+return ENOMEM;
>>>+}
>>>+
>>>+dn = sysdb_sudo_rule_dn(tmp_ctx, domain, name);
>>>+NULL_CHECK(dn, ret, done);
>>>+
>>>+ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
>>>+
>>>+done:
>>>+talloc_free(tmp_ctx);
>>>+return ret;
>Temporary context is not needed here, you can use NULL instead and
>talloc_free(dn) in the end.
I am not sure about it. I have tried it but it didn't work for me.
>>
>>What didn't work for you?
>>
>>errno_t
>>sysdb_set_sudo_rule_attr(struct sss_domain_info *domain,
>> const char *name,
>> struct sysdb_attrs *attrs,
>> int mod_op)
>>{
>> struct ldb_dn *dn;
>> errno_t ret;
>>
>> dn = sysdb_sudo_rule_dn(NULL, domain, name);
>> if (dn == NULL) {
>> return ENOMEM;
>> }
>>
>> ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
>> talloc_free(dn);
>> return ret;
>>}
>
>Hi list,
>
>I think that I find bug in ldb-1.1.25:
>
>
>So, I have done little investigation.
>
>In short, backtrace in gdb says:
>
>#0 ldb_dn_new_fmt (mem_ctx=0x0, ldb=0x6578f0, new_fmt=0x779b3538
>"name=%s,cn=%s,cn=custom,cn=%s,cn=sysdb") at ../common/ldb_dn.c:174
>#1 0x7796727c in sysdb_custom_dn (mem_ctx=0x0, dom=0x658500,
>object_name=0x409f34 "test_rule3", subtree_name=0x40aecf "sudorules") at
>../src/db/sysdb.c:148
>#2 0x004073e8 in sysdb_sudo_rule_dn (mem_ctx=0x0, domain=0x658500,
>name=0x409f34 "test_rule3") at ../src/db/sysdb_sudo.c:962
>#3 0x00407416 in sysdb_set_sudo_rule_attr (domain=0x658500,
>name=0x409f34 "test_rule3", attrs=0x658390, mod_op=2) at
>../src/db/sysdb_sudo.c:974
>#4 0x0040458b in test_set_sudo_rule_attr_exist (state=0x614a90) at
>../src/tests/cmocka/test_sysdb_sudo.c:400
>#5 0x77bd6659 in cmocka_run_one_test_or_fixture
>(function_name=0x40a8e9 "test_set_sudo_rule_attr_exist", test_func=0x4044d7
>, setup_func=setup_func@entry=0x0,
>teardown_func=teardown_func@entry=0x0, state=state@entry=0x614a90,
>heap_check_point=heap_check_point@entry=0x0) at
>/usr/src/debug/cmocka-1.0.1/src/cmocka.c:2305
>#6 0x77bd6d8f in cmocka_run_one_tests (test_state=0x614a80) at
>/usr/src/debug/cmocka-1.0.1/src/cmocka.c:2413
>#7 _cmocka_run_group_tests (group_name=0x40a823 "tests", tests=out>, num_tests=15, group_setup=, group_teardown=0x0) at
>/usr/src/debug/cmocka-1.0.1/src/cmocka.c:2518
>#8 0x00404d7a in main (argc=1, argv=0x7fffe3f8) at
>../src/tests/cmocka/test_sysdb_sudo.c:645
>
>
>On the line
>/usr/src/debug/ldb-1.1.25/common/ldb_dn.c:174:
>
>167 struct ldb_dn *ldb_dn_new_fmt(TALLOC_CTX *mem_ctx,
>168 struct ldb_context *ldb,
>169 const char *new_fmt, ...)
>170 {
>171 char *strdn;
>172 va_list ap;
>173
>174 if ( (! mem_ctx) || (! ldb)) return NULL;
>175
>176 va_start(ap, new_fmt);
>177 strdn = talloc_vasprintf(mem_ctx, new_fmt, ap);
>178 va_end(ap);
>179
>180 if (strdn) {
>181 struct ldb_dn *dn = ldb_dn_new(mem_ctx, ldb, strdn);
>182 talloc_free(strdn);
>183 return dn;
>184 }
>185
>186 return NULL;
>187 }
>188
>
>How you can see, if we send NULL to sysdb_sudo_rule_dn, it will explode
>very
>badly :-)
>
It might be a bug. But we should try to avoid using NULL context
We had some memory leaks caused by allocating something on NULL context.
LS
>>>
>>>Except in this case this is the same thing as using temporary context but
>>>shorter.
>>If you want to avoid creating tmp_ctx you can still use "ldb"
>>as talloc context and you will not hit bug in ldb_dn_new_fmt.
>
>I agree with you with this as a workaround to avoid creating new talloc
>context here.
>
>>
>>If you extend this function and possibly create memory leak
>>it will be easier to find it.
>
>Actually no, findin
[SSSD] Re: [PATCH] GPO: log specific ini parse error messages
On 03/21/2016 02:17 PM, Lukas Slebodnik wrote:
On (16/03/16 16:50), Michal Židek wrote:
Hi,
sorry, I do not have working AD so I did
not test the patches. My testing
was only SSSD compilation :)
But I made a small ap that parses
ini files and treats the errors
the same way as in these patches
and it worked fine.
It prints the problematic line
with short error description
(like missing equal sign).
The patch is quite simple. It does not
solve any GPO issues, just logs found
parsing errors for easier debugging.
Michal
From 0bff25243b18406f88910a88705397365e2e37f1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?=
Date: Wed, 16 Mar 2016 16:38:34 +0100
Subject: [PATCH] GPO: log specific ini parse error messages
We should log error messages generated by
libini if there are problems with parsing
gpo files.
---
src/providers/ad/ad_gpo.c | 19 +++
src/providers/ad/ad_gpo_child.c | 19 +++
2 files changed, 38 insertions(+)
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 069196c..360aca5 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -1131,8 +1131,27 @@ ad_gpo_store_policy_settings(struct sss_domain_info
*domain,
ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0, ini_config);
if (ret != 0) {
+int lret;
+char **errors;
+
DEBUG(SSSDBG_CRIT_FAILURE,
"ini_config_parse failed [%d][%s]\n", ret, strerror(ret));
Does it make sense to print also the filename?
ini_config_get_filename()
or is it logged somewhere else?
It is not logged on the same level (and in gpo_child
it is not logged at all) so I think it makes sense to
add it to the debug message that informs about parse
failure. I updated the patch.
BTW you might not be able to do it in gpo_child
because IIRC config file is parsed from memory (fmemopen)
It is available in both cases. We call ini_config_file_open
just few lines above.
Michal
LS
>From a33229eed340069910d3ff15d98cd8fe07b9bc42 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?=
Date: Wed, 16 Mar 2016 16:38:34 +0100
Subject: [PATCH] GPO: log specific ini parse error messages
We should log error messages generated by
libini if there are problems with parsing
gpo files.
---
src/providers/ad/ad_gpo.c | 22 +-
src/providers/ad/ad_gpo_child.c | 22 +-
2 files changed, 42 insertions(+), 2 deletions(-)
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 78ff723..a9262d7 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -1138,8 +1138,28 @@ ad_gpo_store_policy_settings(struct sss_domain_info *domain,
ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0, ini_config);
if (ret != 0) {
+int lret;
+char **errors;
+
DEBUG(SSSDBG_CRIT_FAILURE,
- "ini_config_parse failed [%d][%s]\n", ret, strerror(ret));
+ "[%s]: ini_config_parse failed [%d][%s]\n",
+ filename, ret, strerror(ret));
+
+/* Now get specific errors if there are any */
+lret = ini_config_get_errors(ini_config, &errors);
+if (lret != 0) {
+DEBUG(SSSDBG_CRIT_FAILURE,
+ "Failed to get specific parse error [%d][%s]\n", lret,
+ strerror(lret));
+goto done;
+}
+
+for (int a = 0; errors[a]; a++) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "ini_config_parse error %d: %s", a + 1, errors[a]);
+}
+ini_config_free_errors(errors);
+
goto done;
}
diff --git a/src/providers/ad/ad_gpo_child.c b/src/providers/ad/ad_gpo_child.c
index 1b59971..843d343 100644
--- a/src/providers/ad/ad_gpo_child.c
+++ b/src/providers/ad/ad_gpo_child.c
@@ -463,8 +463,28 @@ ad_gpo_parse_ini_file(const char *smb_path,
ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0, ini_config);
if (ret != 0) {
+int lret;
+char **errors;
+
DEBUG(SSSDBG_CRIT_FAILURE,
- "ini_config_parse failed [%d][%s]\n", ret, strerror(ret));
+ "[%s]: ini_config_parse failed [%d][%s]\n",
+ ini_filename, ret, strerror(ret));
+
+/* Now get specific errors if there are any */
+lret = ini_config_get_errors(ini_config, &errors);
+if (lret != 0) {
+DEBUG(SSSDBG_CRIT_FAILURE,
+ "Failed to get specific parse error [%d][%s]\n", lret,
+ strerror(lret));
+goto done;
+}
+
+for (int i = 0; errors[i]; i++) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "ini_config_parse error %d: %s", i + 1, errors[i]);
+}
+ini_config_free_errors(errors);
+
goto done;
}
--
2.5.0
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd
[SSSD] Re: [PATCH SET] TOOL: Invalidation of sudo rules at sss_cache
On 03/21/2016 02:06 PM, Lukas Slebodnik wrote:
On (21/03/16 13:51), Pavel Březina wrote:
On 03/21/2016 01:10 PM, Lukas Slebodnik wrote:
On (21/03/16 11:02), Petr Cech wrote:
On 03/16/2016 12:37 PM, Pavel Březina wrote:
[...]
*Patch 2: SYSDB: Add new funtions into sysdb_sudo*
+errno_t
+sysdb_set_sudo_rule_attr(struct sss_domain_info *domain,
+ const char *name,
+ struct sysdb_attrs *attrs,
+ int mod_op)
+{
+errno_t ret;
+struct ldb_dn *dn;
+TALLOC_CTX *tmp_ctx;
+
+tmp_ctx = talloc_new(NULL);
+if (tmp_ctx == NULL) {
+return ENOMEM;
+}
+
+dn = sysdb_sudo_rule_dn(tmp_ctx, domain, name);
+NULL_CHECK(dn, ret, done);
+
+ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
+
+done:
+talloc_free(tmp_ctx);
+return ret;
Temporary context is not needed here, you can use NULL instead and
talloc_free(dn) in the end.
I am not sure about it. I have tried it but it didn't work for me.
What didn't work for you?
errno_t
sysdb_set_sudo_rule_attr(struct sss_domain_info *domain,
const char *name,
struct sysdb_attrs *attrs,
int mod_op)
{
struct ldb_dn *dn;
errno_t ret;
dn = sysdb_sudo_rule_dn(NULL, domain, name);
if (dn == NULL) {
return ENOMEM;
}
ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
talloc_free(dn);
return ret;
}
Hi list,
I think that I find bug in ldb-1.1.25:
So, I have done little investigation.
In short, backtrace in gdb says:
#0 ldb_dn_new_fmt (mem_ctx=0x0, ldb=0x6578f0, new_fmt=0x779b3538
"name=%s,cn=%s,cn=custom,cn=%s,cn=sysdb") at ../common/ldb_dn.c:174
#1 0x7796727c in sysdb_custom_dn (mem_ctx=0x0, dom=0x658500,
object_name=0x409f34 "test_rule3", subtree_name=0x40aecf "sudorules") at
../src/db/sysdb.c:148
#2 0x004073e8 in sysdb_sudo_rule_dn (mem_ctx=0x0, domain=0x658500,
name=0x409f34 "test_rule3") at ../src/db/sysdb_sudo.c:962
#3 0x00407416 in sysdb_set_sudo_rule_attr (domain=0x658500,
name=0x409f34 "test_rule3", attrs=0x658390, mod_op=2) at
../src/db/sysdb_sudo.c:974
#4 0x0040458b in test_set_sudo_rule_attr_exist (state=0x614a90) at
../src/tests/cmocka/test_sysdb_sudo.c:400
#5 0x77bd6659 in cmocka_run_one_test_or_fixture
(function_name=0x40a8e9 "test_set_sudo_rule_attr_exist", test_func=0x4044d7
, setup_func=setup_func@entry=0x0,
teardown_func=teardown_func@entry=0x0, state=state@entry=0x614a90,
heap_check_point=heap_check_point@entry=0x0) at
/usr/src/debug/cmocka-1.0.1/src/cmocka.c:2305
#6 0x77bd6d8f in cmocka_run_one_tests (test_state=0x614a80) at
/usr/src/debug/cmocka-1.0.1/src/cmocka.c:2413
#7 _cmocka_run_group_tests (group_name=0x40a823 "tests", tests=, num_tests=15, group_setup=, group_teardown=0x0) at
/usr/src/debug/cmocka-1.0.1/src/cmocka.c:2518
#8 0x00404d7a in main (argc=1, argv=0x7fffe3f8) at
../src/tests/cmocka/test_sysdb_sudo.c:645
On the line
/usr/src/debug/ldb-1.1.25/common/ldb_dn.c:174:
167 struct ldb_dn *ldb_dn_new_fmt(TALLOC_CTX *mem_ctx,
168 struct ldb_context *ldb,
169 const char *new_fmt, ...)
170 {
171 char *strdn;
172 va_list ap;
173
174 if ( (! mem_ctx) || (! ldb)) return NULL;
175
176 va_start(ap, new_fmt);
177 strdn = talloc_vasprintf(mem_ctx, new_fmt, ap);
178 va_end(ap);
179
180 if (strdn) {
181 struct ldb_dn *dn = ldb_dn_new(mem_ctx, ldb, strdn);
182 talloc_free(strdn);
183 return dn;
184 }
185
186 return NULL;
187 }
188
How you can see, if we send NULL to sysdb_sudo_rule_dn, it will explode very
badly :-)
It might be a bug. But we should try to avoid using NULL context
We had some memory leaks caused by allocating something on NULL context.
LS
Except in this case this is the same thing as using temporary context but
shorter.
If you want to avoid creating tmp_ctx you can still use "ldb"
as talloc context and you will not hit bug in ldb_dn_new_fmt.
I agree with you with this as a workaround to avoid creating new talloc
context here.
If you extend this function and possibly create memory leak
it will be easier to find it.
Actually no, finding memory leaked on NULL context is easier than
finding it on existing talloc context. Nothing but top level context is
supposed to be attached to NULL context so if there is anything else we
have a leak, it's simple as that. Using existing talloc context makes it
harder to find and it may not even show because you free it somewhere in
the process.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
