[PATCH] Failover to next server if authentication fails
by Pavel Březina
We can fail in sasl_bind_send() with ERR_AUTH_FAILED for basically
unspecified reason but we do not failover to next server. This patch
should fix it.
As said on the meeting, I didn't reproduce it and I'm not sure if it
will fix the customer issue unless they confirm it, but it seems to be a
valid patch anyway.
7 years, 7 months
Re: [PATCH SET] AD_PROVIDER: ad_enabled_domains
by Petr Cech
Sorry, I experienced some issue with mailing list.
So I send it again.
-------- Forwarded Message --------
Subject: Re: [SSSD] Re: [PATCH SET] AD_PROVIDER: ad_enabled_domains
Date: Tue, 9 Aug 2016 17:29:38 +0200
From: Petr Cech <pcech(a)redhat.com>
To: sssd-devel(a)lists.fedorahosted.org
On 08/09/2016 11:07 AM, Jakub Hrozek wrote:
> On Mon, Jul 25, 2016 at 06:18:28PM +0200, Petr Cech wrote:
>> > Hello,
>> >
>> > there is fixed patch set attached.
>> >
>> > Segmentation fault was caused by wrong pointer :-(, sorry.
>> >
>> > This new patch set has new debug message. I am open to dissccus the
>> > debug_level and content of message. Any improving idea?
>> >
>> > I hit one issue during testing -- sometimes if I am connected to subdomain
>> > and I enable only sibling subdomain (the master is added automaticaly) and
>> > forest root is not enabled -- I see only master and sibling not. But if I
>> > added sleep for cycle (for using dbg) to function ad_subdomains_init()
>> > everythink is OK.
>> > Any idea?
> Can you test that case with valgrind? This sounds like some uninitilized
> variable condition.
I didn't run valgrind but I have new information.
If you clear the cache and reset sssd, first attempt to obtain
information about user from sibling domain fails. The second and the
other attempts runs correctly.
I see that the sibling domain is enabled. But if I look more carefully
there is message in log (gamma.domain.bootes is sibling domain):
[sssd[be[beta.domain.bootes]]] [dp_req_new] (0x0020): Unknown domain:
gamma.domain.bootes
First attempt should works too but you should wait nearly exactly 6
seconds after restart sssd.
New patch set is attached.
>> > Anyway, I would like to ask you for testing.
>> >
>> > Regards
>> >
>> > --
>> > Petr^4 Čech
>> > From 7fd9ad8f42f274463c1d107b195d21290fd0b0f2 Mon Sep 17 00:00:00 2001
>> > From: Petr Cech <pcech(a)redhat.com>
>> > Date: Fri, 13 May 2016 05:21:07 -0400
>> > Subject: [PATCH 1/5] AD_PROVIDER: Add ad_enabled_domains option
> ACK
Thanks.
>> > From dfa6e063d3ef4850a7809e2f5a6d3826ea061b27 Mon Sep 17 00:00:00 2001
>> > From: Petr Cech <pcech(a)redhat.com>
>> > Date: Tue, 21 Jun 2016 08:34:15 +0200
>> > Subject: [PATCH 2/5] AD_PROVIDER: Initializing of ad_enabled_domains
>> >
>> > We add ad_enabled_domains into ad_subdomains_ctx.
>> >
>> > Resolves:
>> > https://fedorahosted.org/sssd/ticket/2828
>> > ---
>> > src/providers/ad/ad_subdomains.c | 82 ++++++++++++++++++++++++++++++++++++++++
>> > 1 file changed, 82 insertions(+)
>> >
>> > diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
>> > index e9da04e384e598927f9c8c203a751bcccd29e895..9c0afb19418e44a3e3daa661baf1c7a82439d60d 100644
>> > --- a/src/providers/ad/ad_subdomains.c
>> > +++ b/src/providers/ad/ad_subdomains.c
>> > @@ -57,6 +57,79 @@
>> > /* do not refresh more often than every 5 seconds for now */
>> > #define AD_SUBDOMAIN_REFRESH_LIMIT 5
>> >
>> > +static errno_t ad_get_enabled_domains(TALLOC_CTX *mem_ctx,
>> > + struct ad_id_ctx *ad_id_ctx,
>> > + const char *ad_domain,
>> > + const char ***_ad_enabled_domains)
>> > +{
>> > + int ret;
>> > + const char *str;
>> > + const char *option_name;
>> > + const char **domains = NULL;
>> > + int count;
>> > + bool is_ad_in_domains;
>> > + TALLOC_CTX *tmp_ctx = NULL;
>> > +
>> > + tmp_ctx = talloc_new(NULL);
>> > + if (tmp_ctx == NULL) {
>> > + return ENOMEM;
>> > + }
>> > +
>> > + str = dp_opt_get_cstring(ad_id_ctx->ad_options->basic, AD_ENABLED_DOMAINS);
>> > + if (str == NULL) {
>> > + _ad_enabled_domains = NULL;
> I think you wanted to dereference the pointer here? (it might also be a
> good idea to return EINVAL if the caller passed NULL as the output pointer)
Right, thanks, addressed.
>> > + ret = EOK;
>> > + goto done;
>> > + }
>> > +
>> > From 4cd5c77f09750f9eb20c91eadc4759c3e4166093 Mon Sep 17 00:00:00 2001
>> > From: Petr Cech <pcech(a)redhat.com>
>> > Date: Tue, 21 Jun 2016 09:48:52 +0200
>> > Subject: [PATCH 3/5] AD_PROVIDER: ad_enabled_domains - only master
>> >
>> > We can skip looking up other domains if option ad_enabled_domains
>> > contains only master domain.
>> >
>> > Resolves:
>> > https://fedorahosted.org/sssd/ticket/2828
>> > ---
>> > src/providers/ad/ad_subdomains.c | 16 ++++++++++++++++
>> > 1 file changed, 16 insertions(+)
>> >
>> > diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
>> > index 9c0afb19418e44a3e3daa661baf1c7a82439d60d..eaa85f802dbb4022dc632cc4c05d89685eccd901 100644
>> > --- a/src/providers/ad/ad_subdomains.c
>> > +++ b/src/providers/ad/ad_subdomains.c
>> > @@ -1163,6 +1163,7 @@ static void ad_subdomains_refresh_connect_done(struct tevent_req *subreq)
>> > return;
>> > }
>> >
>> > + /* connect to the DC we are a member of */
>> > subreq = ad_master_domain_send(state, state->ev, state->id_ctx->conn,
>> > state->sdap_op, state->sd_ctx->domain_name);
>> > if (subreq == NULL) {
>> > @@ -1211,6 +1212,21 @@ static void ad_subdomains_refresh_master_done(struct tevent_req *subreq)
>> > goto done;
>> > }
>> >
>> > + /*
>> > + * If ad_enabled_domains contains only master domain
>> > + * we shouldn't lookup other domains.
>> > + */
>> > + if (state->sd_ctx->ad_enabled_domains != NULL) {
>> > + if (talloc_array_length(state->sd_ctx->ad_enabled_domains) == 2) {
>> > + if (strcmp(state->sd_ctx->ad_enabled_domains[0],
>> > + state->be_ctx->domain->name) == 0) {
> I only notices this now, but IIRC the domains are case insensitive,
> shouldn't we use strcasecmp here?
I see strcasecmp in other places. So I fixed it.
>> > + DEBUG(SSSDBG_TRACE_FUNC,
>> > + "No other enabled domain than master.\n");
>> > + goto done;
>> > + }
>> > + }
>> > + }
>> > +
>> > subreq = ad_get_root_domain_send(state, state->ev, forest,
>> > sdap_id_op_handle(state->sdap_op),
>> > state->sd_ctx);
>> > --
>> > 2.7.4
>> >
>> > From 0d2b0c67875663d0e614b8361152ec7edc14c37e Mon Sep 17 00:00:00 2001
>> > From: Petr Cech <pcech(a)redhat.com>
>> > Date: Mon, 27 Jun 2016 11:51:30 +0200
>> > Subject: [PATCH 4/5] AD_PROVIDER: ad_enabled_domains - other then master
>> >
>> > We can skip looking up other domains if
>> > option ad_enabled_domains doesn't contain them.
>> >
>> > Resolves:
>> > https://fedorahosted.org/sssd/ticket/2828
>> > ---
>> > src/providers/ad/ad_subdomains.c | 49 ++++++++++++++++++++++++++++++++++++----
>> > 1 file changed, 45 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
>> > index eaa85f802dbb4022dc632cc4c05d89685eccd901..32f7d7b3b2c4917d15d516c5e8beac159ffe164f 100644
>> > --- a/src/providers/ad/ad_subdomains.c
>> > +++ b/src/providers/ad/ad_subdomains.c
>> > @@ -130,6 +130,23 @@ done:
>> > return ret;
>> > }
>> >
>> > +static bool is_domain_enabled (const char *domain,
>> > + const char **enabled_doms)
> ^
> We don't put a space between function name and the argument list.
Addressed.
>> > +{
>> > + bool ret;
>> > +
>> > + if (enabled_doms == NULL) {
>> > + ret = true;
>> > + goto done;
> Do we really need to use jump labels instead of just returning true?
Addressed.
>> > + }
>> > +
>> > + ret = string_in_list(domain, discard_const_p(char *, enabled_doms), true) ?
>> > + true : false;
> Can you also fix the indentation here? We could also just return the
> function return value and get rid of the 'ret' variable completely.
Addressed.
>> > +
>> > +done:
>> > + return ret;
>> > +}
>> > +
>> > static errno_t
>> > ad_subdom_ad_ctx_new(struct be_ctx *be_ctx,
>> > struct ad_id_ctx *id_ctx,
>> > @@ -485,6 +502,7 @@ done:
>> >
>> > From 92af52a3b4d8fa138282030a72122d6cbe9c5413 Mon Sep 17 00:00:00 2001
>> > From: Petr Cech <pcech(a)redhat.com>
>> > Date: Mon, 27 Jun 2016 11:53:19 +0200
>> > Subject: [PATCH 5/5] TESTS: Adding tests for ad_enabled_domains option
> ACK
Thanks.
Regards
--
Petr^4 Čech
7 years, 7 months
[PATCH] WIP: PROXY: Adding proxy_max_children option
by Petr Cech
Hello,
I am fighting with adding new option to sssd.conf.
I slowly running out of breath.
I know proxy could be id, auth or chpass provider. I don't know
where is the right place for my option. And the second issue is
it breaks test for SSSD config. :-(
Is there anyone who would like to join to the fight? Please,
see attached patch.
Regards
--
Petr^4 Čech
7 years, 7 months
