Bug 12696 - net ads join always moves the computer account OU if the account already existed.
Summary: net ads join always moves the computer account OU if the account already exis...
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
Depends on:
Reported: 2017-03-15 22:53 UTC by Jeremy Allison
Modified: 2017-03-31 06:22 UTC (History)
2 users (show)

See Also:

git-am fix for master. (1.41 KB, patch)
2017-03-15 22:53 UTC, Jeremy Allison
no flags Details
Updated commit message with bugID. (1.46 KB, patch)
2017-03-15 23:03 UTC, Jeremy Allison
jra: review? (gd)

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2017-03-15 22:53:18 UTC
> In source3/libnet/libnet_join.c we have:
>        /* Attempt to create the machine account and bail if this fails.
>            Assume that the admin wants exactly what they requested */
>         status = ads_create_machine_acct(r->in.ads,
>                                          r->in.machine_name,
>                                          r->in.account_ou,
>                                          r->in.desired_encryption_types);
>         if (ADS_ERR_OK(status)) {
>                 DEBUG(1,("machine account creation created\n"));
>                 return status;
>         } else  if ((status.error_type == ENUM_ADS_ERROR_LDAP) &&
>                     (status.err.rc == LDAP_ALREADY_EXISTS)) {
>                 status = ADS_SUCCESS;
>         }
>         if (!ADS_ERR_OK(status)) {
>                 DEBUG(1,("machine account creation failed\n"));
>                 return status;
>         }
>         status = ads_move_machine_acct(r->in.ads,
>                                        r->in.machine_name,
>                                        r->in.account_ou,
>                                        &moved);
>         if (!ADS_ERR_OK(status)) {
>                 DEBUG(1,("failure to locate/move pre-existing "
>                         "machine account\n"));
>                 return status;
>         }
>         DEBUG(1,("The machine account %s the specified OU.\n",
>                 moved ? "was moved into" : "already exists in"));
> As you can see it treats LDAP_ALREADY_EXISTS as "ok" and moves
> the account anyway. Should we move the account to the new
> OU if it already existed, or should we leave it where it
> was ?

Looking at this some more, at the top of this function we have:

static ADS_STATUS libnet_join_precreate_machine_acct(TALLOC_CTX *mem_ctx,
                                                     struct libnet_JoinCtx *r)
        ADS_STATUS status;
        LDAPMessage *res = NULL;
        const char *attrs[] = { "dn", NULL };
        bool moved = false;

        status = ads_check_ou_dn(mem_ctx, r->in.ads, &r->in.account_ou);
        if (!ADS_ERR_OK(status)) {
                return status;

If someone just ran 'net ads join' without using the 'createcomputer' parameter,
then on entry to this function r->in.account_ou == NULL.

The call to ads_check_ou_dn() fills this in to a valid value for the following
ads_create_machine_acct() call.

Could we make the ads_move_machine_acct() conditional on r->in.account_ou not
initially being NULL ?

That way if someone does use createcomputer=dn on the command line, then we move
it if it already existed, but if they don't, then we leave it where it was.

We already do something similar inside the calling function libnet_DomainJoin(),
which has:

                ads_status = libnet_join_precreate_machine_acct(mem_ctx, r);
                if (ADS_ERR_OK(ads_status)) {

                         * LDAP object create succeeded, now go to the rpc
                         * password set routines

                        r->in.join_flags &= ~WKSSVC_JOIN_FLAGS_ACCOUNT_CREATE;
                        goto rpc_join;

                if (initial_account_ou != NULL) {
                        libnet_join_set_error_string(mem_ctx, r,
                                "failed to precreate account in ou %s: %s",
                        return WERR_NERR_DEFAULTJOINREQUIRED;

                DEBUG(5, ("failed to precreate account in ou %s: %s",
                        r->in.account_ou, ads_errstr(ads_status)));

so there appears to be precedence for this.

Test patch to follow.
Comment 1 Jeremy Allison 2017-03-15 22:53:50 UTC
Created attachment 13070 [details]
git-am fix for master.

Possible patch if we agree this is something we need to change.
Comment 2 Jeremy Allison 2017-03-15 23:03:03 UTC
Created attachment 13072 [details]
Updated commit message with bugID.
Comment 3 Jeremy Allison 2017-03-31 00:24:32 UTC
Ping ! Hi Guenther, can you take a look at this ?