[SSSD] [PATCH] AD SRV plugin

Jakub Hrozek jhrozek at redhat.com
Mon Apr 29 22:37:33 UTC 2013


On Mon, Apr 29, 2013 at 06:58:41PM +0200, Pavel Březina wrote:
> On 04/17/2013 02:44 PM, Pavel Březina wrote:
> >https://fedorahosted.org/sssd/ticket/1032
> >
> >Here it is.
> >
> 
> This is new patch set that addresses few issues raised by Jakub via IRC.
> 
> The most important was that I should treat domain names as case
> insensitive. So I changed strcmp() to strcasecmp() in
> fo_discover_servers_primary_done() which was part of the the original
> patch set and also included a new patch that makes the change in dns srv
> plugin.

tl;dr - looking mostly good. Let's fix some nitpicks and push the work
to master.

I performed testing using the dssite.msc MMC -- without any specific site, I
got redirected to the Default-First-Site-Name site, defining the site
got me redirected to the proper site. 

[PATCH 1/7] add fo_discover_servers request
Mostly good, but I'd like to discuss returning backup servers as primary
servers. I haven't tested that particular case, but can you confirm that
if any records were added on the server that would resolve to primary
servers, the client would pick them up after the SRV query expires
without restarting the client completely?

Otherwise ack.

[PATCH 2/7] IPA SRV plugin: use fo_discover_servers request
Ack. I did a quick sanity test of the IPA DNS discovery and it still
works.

[PATCH 3/7] IPA SRV plugin: improve debugging
Ack

[PATCH 4/7] sdap: add sdap_connect_host request
I tested this patch by checking with netstat that there was only one
connection after the discovery used the request. 

I just wonder if using "struct be_resolv_ctx" would make more sense rather
than using both "struct resolv_ctx" and "enum restrict_family". It would also
make sense to cancel the resolv request after "dns_resolver_timeout" passes
(and read its value from "struct be_resolv_ctx"), although I would be OK
with filing a ticket for that. This functionality is not needed for the beta.

[PATCH 5/7] add sss_ldap_filter_binary_value
Have you looked at ldap_encode_ndr_uint32() from Samba? At the very least
I would prefer if the name made it clear that the function is converting
uint32_t to char. FTR the current implementation of ldap_encode_ndr_uint32()
looks like:

char *ldap_encode_ndr_uint32(TALLOC_CTX *mem_ctx, uint32_t value)
{
    uint8_t buf[4];
    struct ldb_val val;
    SIVAL(buf, 0, value);
    val.data = buf;
    val.length = 4;
    return ldb_binary_encode(mem_ctx, val);
}

So what about a very similar sss_ldap_encode_ndr_uint32() ?

[PATCH 6/7] DNS sites support - add AD SRV plugin
This patch makes samba4-devel a hard requirement of the AD provider.
Given that we're considering dropping support of RHEL5 upstream now, I
don't think this should be a blocker in accepting these patches, but I
would prefer to file a ticket to discover if we can #ifdef the support
out to keep the support of older releases upstream.

It might not be practical after all given the other AD provider features
coming up in 1.10, but still, I think we should track the samba4-devel
requirement and decide on implementing or dropping the ifdefs later during
1.10.0 cycle when all the features are in.

> --- a/src/man/sssd-ad.5.xml
> +++ b/src/man/sssd-ad.5.xml
> @@ -130,6 +130,26 @@ ldap_id_mapping = False
>                          </para>
>                      </listitem>
>                  </varlistentry>
> +                
> +                <varlistentry>
> +                    <term>ad_enable_dns_sites (boolean)</term>
> +                    <listitem>
> +                        <para>
> +                            Enables DNS sites - location based
> +                            service discovery.
> +                        </para>
> +                        <para>
> +                            If true and service discovery (see Service
> +                            Discovery paragraph at the bottom of the man page)
> +                            is enabled, then the SSSD will first attempt
> +                            location based discovery using an Active Directory
> +                            mechanism called Sites.

I was wondering for a while whether to explain a little about the AD
Sites, but the sssd-ad man page is probably not the right place..

Still, what about "the SSSD will first attempt to discover the Active
Directory server to connect to using the Active Directory Site
Discovery and fall back to the DNS SRV records if no AD site is found".

Defaulting to True is correct give the presence of the
Default-First-Site-Name site.

The ad_get_client_site_send() function could accept "struct
be_resolv_ctx".

Nitpick:
> +static void ad_get_client_site_done(struct tevent_req *subreq)
> +{
> +    struct ad_get_client_site_state *state = NULL;
> +    struct tevent_req *req = NULL;
> +    struct ldb_message_element *el = NULL;
> +    struct sysdb_attrs **reply = NULL;
> +    size_t reply_count;
> +    errno_t ret;
> +
> +    req = tevent_req_callback_data(subreq, struct tevent_req);
> +    state = tevent_req_data(req, struct ad_get_client_site_state);
> +
> +    ret = sdap_get_generic_recv(subreq, state, &reply_count, &reply);
> +    talloc_zfree(subreq);

Can you put a comment here stating that this is where the LDAP
connection that was used for the location discovery is closed?

> +    talloc_zfree(state->sh);
> +    if (ret != EOK) {

The rest looks good to me.

[PATCH 7/7] dns srv plugin: compare domain names case insensitive
Ack



More information about the sssd-devel mailing list