[SSSD] [PATCH] AD GPO: Fix incorrect sAMAccountName selection

Jakub Hrozek jhrozek at redhat.com
Mon Sep 29 11:37:31 UTC 2014


On Thu, Sep 25, 2014 at 03:41:22PM -0400, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 09/25/2014 03:40 PM, Stephen Gallagher wrote:
> > On 09/25/2014 04:56 AM, Jakub Hrozek wrote:
> >> On Wed, Sep 24, 2014 at 11:10:00AM -0400, Stephen Gallagher
> >> wrote: We were assuming that the ad_hostname value would match
> >> the sAMAccountName attribute, but in practice this was almost
> >> never the case on a properly-configured system.
> > 
> >> Microsoft's convention is that the sAMAccountName is always the 
> >> portion of the FQDN before the first dot, so this patch makes
> >> that same assumption.
> > 
> >>> From c179806c27ce6d25137306ba7bb37ecfae573c3b Mon Sep 17
> >>> 00:00:00 2001 From: Stephen Gallagher <sgallagh at redhat.com>
> >>> Date: Tue, 23 Sep 2014 17:44:41 -0400 Subject: [PATCH] AD GPO:
> >>> Fix incorrect sAMAccountName selection
> >>> 
> >>> --- src/providers/ad/ad_gpo.c | 20 +++++++++++++++++++- 1 file 
> >>> changed, 19 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/src/providers/ad/ad_gpo.c 
> >>> b/src/providers/ad/ad_gpo.c index 
> >>> de4d44166b85ccd85ed36bcb11f0596e0020af11..745af8b2786a5d6c71a2a3eb6c1448a61c151019
> >>>
> >>> 
> 100644 --- a/src/providers/ad/ad_gpo.c +++
> >>> b/src/providers/ad/ad_gpo.c @@ -1479,6 +1479,8 @@ 
> >>> ad_gpo_connect_done(struct tevent_req *subreq) struct
> >>> tevent_req *req; struct ad_gpo_access_state *state; char
> >>> *filter; +    char *hostname; +    char *shortname; char
> >>> *sam_account_name; char *domain_dn; int dp_error; @@ -1519,7
> >>> +1521,21 @@ ad_gpo_connect_done(struct tevent_req *subreq) } }
> >>> 
> >>> -    sam_account_name = talloc_asprintf(state, "%s$", 
> >>> state->ad_hostname); +    hostname = talloc_strdup(state, 
> >>> state->ad_hostname); +    if (hostname == NULL) { +        ret
> >>> = ENOMEM; +        goto done; +    } +    shortname = 
> >>> strtok(hostname, "."); +    if (shortname == NULL) { +
> >>> /* This should never fail; if there's no dot, +         * it
> >>> should return the full string. +         */ +        ret = EIO;
> >>> + goto done; +    } +    sam_account_name =
> >>> talloc_asprintf(state, "%s$", hostname); +
> >>> talloc_zfree(hostname); if (sam_account_name == NULL) { ret =
> >>> ENOMEM; goto done;
> > 
> >> The fix works, but we already have code that does pretty much
> >> the same for principal selection -- check out get_primary() in 
> >> sss_krb5.c. Would it be better to split out lines 38 to 53 from 
> >> get_primary() into a separate function and use it in ad_gpo.c,
> >> too, to save code duplication?
> > 
> > 
> > I didn't know about get_primary(). It's actually a perfect
> > solution for this (and accomplishes everything I set out to do
> > here, including appending the $ to the shortname).
> > 
> > See two new patches. The first renames get_primary() to 
> > sss_krb5_get_primary() and makes it public, the second consumes it
> > to generate an appropriate sAMAccountName value.
> > 
> > I tested this with my AD setup and it works correctly.
> 
> And this time with the patches attached...

ACK. I checked out the code in gdb:

(gdb) n
1489        req = tevent_req_callback_data(subreq, struct tevent_req);
(gdb) 
1490        state = tevent_req_data(req, struct ad_gpo_access_state);
(gdb) 
1492        ret = sdap_id_op_connect_recv(subreq, &dp_error);
(gdb) 
1493        talloc_zfree(subreq);
(gdb) 
1495        if (ret != EOK) {
(gdb) 
1522        sam_account_name = sss_krb5_get_primary(state, "%S$", state->ad_hostname);
(gdb) 
1523        if (sam_account_name == NULL) {
(gdb) p sam_account_name 
$1 = 0x1845a90 "ADCLIENT$



More information about the sssd-devel mailing list