[SSSD] [PATCHES] Support one-way trusts with AD domains in IPA server mode

Jakub Hrozek jhrozek at redhat.com
Mon Jun 15 07:43:44 UTC 2015


On Mon, Jun 15, 2015 at 08:43:56AM +0200, Lukas Slebodnik wrote:
> On (12/06/15 17:59), Sumit Bose wrote:
> >On Fri, Jun 12, 2015 at 03:32:36PM +0200, Jakub Hrozek wrote:
> >> On Thu, Jun 11, 2015 at 09:01:46PM +0200, Sumit Bose wrote:
> >> > On Wed, Jun 10, 2015 at 09:30:43PM +0200, Jakub Hrozek wrote:
> >> > > On Mon, Jun 08, 2015 at 05:03:02PM +0200, Sumit Bose wrote:
> >> > > > On Mon, Jun 08, 2015 at 10:16:26AM +0200, Jakub Hrozek wrote:
> >> > > > > On Thu, Jun 04, 2015 at 01:15:55PM +0200, Jakub Hrozek wrote:
> >> > > > > > Hi,
> >> > > > > > 
> >> > > > > > the attached patches implement most of the one-way trust functionality.
> >> > > > > > The trust directions, fetching keytabs and using different keytabs and
> >> > > > > > different principals works well for me. I'm still working on changing
> >> > > > > > the ldap_child to either use ccache collection or using the environment
> >> > > > > > variables safer, also the failover changes are still missing. But the
> >> > > > > > feature is testable in my opinion.
> >> > > > > > 
> >> > > > > > There are two NOSUBMIT patches that are useful only for testing until
> >> > > > > > patches that allow the IPA server principal to read the direction are
> >> > > > > > available. I hope exposing my super-secret DM password like that is OK,
> >> > > > > > please change these patches in your testing..
> >> > > > > > 
> >> > > > > > Several patches are not strictly related to one-way trusts, but unify
> >> > > > > > the info we store for subdomains in IPA and AD or info we store for
> >> > > > > > subdomains that represent forest root versus member domains.
> >> > > > > > 
> >> > > > > > There are also patches that rename or refactor a bit functions in the
> >> > > > > > ad_common.c module. I hope this is acceptable, because I had a hard time
> >> > > > > > le-learning my way around the module. I still think we need to make the
> >> > > > > > code that selects the appropriate principal from keytab readable better,
> >> > > > > > currently the setting of "desired_primary" and "default_primary" is a
> >> > > > > > total mess.
> >> > > > > > 
> >> > > > > > Most of the code is also unit-tested, so several patches just change
> >> > > > > > tests.
> >> > > > > > 
> >> > > > > > Here are some points I'd like to get reviewed carefully as I'm not sure
> >> > > > > > about them myself:
> >> > > > > >     - do we need the SD_TRUST_DIRECTION_NOT_SET enum? I was going back
> >> > > > > >       and forth between having it and just using either a NULL pointer
> >> > > > > >       if the trust direction is uknown or a zero value.
> >> > > > > >     - is the reading of the direction OK? Do we fall back the way we
> >> > > > > >       should?
> >> > > > > >     - are the additional data stored with (sub)domains like forest
> >> > > > > >       stored for forest root subdomains and realm for master domains OK? In
> >> > > > > >       my opinion they make processing of domains easier as there's fewer
> >> > > > > >       special cases..
> >> > > > > >     - should I add a full-blown getter for the forest_root member of
> >> > > > > >       sss_domain_info a a first step towards making the structure
> >> > > > > >       opaque?
> >> > > > > > 
> >> > > > > > Also feel free to propose more tests, either scenarios that I should
> >> > > > > > test manually or something that should be unit tested.
> >> > > > > 
> >> > > > > Hi,
> >> > > > > 
> >> > > > > I fixed one casing issue that Sumit found and one similar issue that
> >> > > > > Alexander found (and patched, thanks a lot!) and pushed new patches
> >> > > > > here:
> >> > > > >     https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=oneway
> >> > > > 
> >> > > > Hi Jakub,
> >> > > > 
> >> > > > so far I mainly tested for regression but so far didn't found any. Please find
> >> > > > below my review of the first 19 patches. Maybe you can add the Reviewed-by tag
> >> > > > to the ones I already ACKed. This would help me (any maybe others) with the
> >> > > > second round.
> >> > > > 
> >> > > > bye,
> >> > > > Sumit
> >> > > > 
> >> > > > - SYSDB: Store trust direction for subdomains
> >> > > > The values for the trust direction are define in MS-ADTS 6.1.6.7.12
> >> > > > (currently https://msdn.microsoft.com/en-us/library/cc223768.aspx). I
> >> > > > would recommend to use the same values in the SSSD cache. This makes it
> >> > > > easier to compare them with setting on AD.
> >> > > 
> >> > > OK, initially I didn't want to spill the knowledge about AD-specific
> >> > > values into sss_domain_info, but I wanted to restrict the values
> >> > > to some known set, so I came up with the enum.
> >> > > 
> >> > > After some more discussion on IRC with Sumit, I converted the trust
> >> > > direction from enum to uint32_t. The domain user, not the domain would be
> >> > > responsible for storing the right values. This would allow us to simplify
> >> > > or rather remove the translation later in the IPA code as well.
> >> > > 
> >> > > > 
> >> > > > For SD_TRUST_DIRECTION_NOT_SET you should pick a suitable value, maybe 128?
> >> > > 
> >> > > NOT_SET is now completely removed and zero is used for cases where we
> >> > > don't care. This also allows us to not store the parameter at all to
> >> > > sysdb unless it's set, because we have:
> >> > >     if (trust_direction) td_flags = LDB_FLAG_MOD_ADD;
> >> > > in sysdb_subdomain_store().
> >> > > 
> >> > > > 
> >> > > > You use
> >> > > > ...
> >> > > > > +    int trust_direction;
> >> > > > >  
> >> > > > >      tmp_ctx = talloc_new(NULL);
> >> > > > >      if (tmp_ctx == NULL) {
> >> > > > > @@ -106,6 +108,10 @@ errno_t sysdb_update_subdomains(struct sss_domain_info *domain)
> >> > > > >          forest = ldb_msg_find_attr_as_string(res->msgs[i],
> >> > > > >                                               SYSDB_SUBDOMAIN_FOREST, NULL);
> >> > > > >  
> >> > > > > +        trust_direction = ldb_msg_find_attr_as_int(res->msgs[i],
> >> > > > > +                                             SYSDB_SUBDOMAIN_TRUST_DIRECTION,
> >> > > > > +                                             SD_TRUST_DIRECTION_NOT_SET);
> >> > > > > +
> >> > > > ...
> >> > > > and
> >> > > > ...
> >> > > > > +    enum trust_direction tmp_td;
> >> > > > >      int ret;
> >> > > > >  
> >> > > > >      tmp_ctx = talloc_new(NULL);
> >> > > > ...
> >> > > > > @@ -622,10 +641,18 @@ errno_t sysdb_subdomain_store(struct sysdb_ctx *sysdb,
> >> > > > >                  forest_flags = LDB_FLAG_MOD_REPLACE;
> >> > > > >              }
> >> > > > >          }
> >> > > > > +
> >> > > > > +        tmp_td = ldb_msg_find_attr_as_uint(res->msgs[0],
> >> > > > > +                                           SYSDB_SUBDOMAIN_TRUST_DIRECTION,
> >> > > > > +                                           SD_TRUST_DIRECTION_NOT_SET);
> >> > > > > +        if (tmp_td != trust_direction) {
> >> > > > > +            td_flags = LDB_FLAG_MOD_REPLACE;
> >> > > > ...
> >> > > > 
> >> > > > I think only one version should be used.
> >> > > 
> >> > > uint32_t is used everywhere now.
> >> > 
> >> > ACK 
> >> 
> >> RB: you
> >> 
> >> > 
> >> > > 
> >> > > > 
> >> > > > ============================================================================
> >> > > > - UTIL/SYSDB: Move new_subdomain() to sysdb_subdomains.c and make it private
> >> > > > 
> >> > > > ACK
> >> > > 
> >> > > Thanks, I added R-B:you even though I changed the parameter of
> >> > > new_subdomain from enum to uint32_t. That was the only change.
> >> > > 
> >> > > > 
> >> > > > ==========================================
> >> > > > - TESTS: Add a test for sysdb_subdomains.c
> >> > > > 
> >> > > > ACK
> >> > > 
> >> > > Same here. I added R-B: you and only changed the SD_TRUST_DIRECTION_*
> >> > > for integers.
> >> > > 
> >> > > > 
> >> > > > ================================================
> >> > > > SYSDB: Add realm to sysdb_master_domain_add_info
> >> > > > 
> >> > > > 
> >> > > > Why do you say "Missing realm or hostname." in the debug message if the realm
> >> > > > could not be found?
> >> > > 
> >> > > I don't know, I think it must be a copy-and-paste error. It's fixed now.
> >> > > 
> >> > > > 
> >> > > > > diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c
> >> > > > > index b82bda2..7587aab 100644
> >> > > > > --- a/src/providers/ipa/ipa_subdomains.c
> >> > > > > +++ b/src/providers/ipa/ipa_subdomains.c
> >> > > > > @@ -1416,6 +1416,7 @@ static void ipa_subdomains_handler_master_done(struct tevent_req *req)
> >> > > > >      if (reply_count) {
> >> > > > >          const char *flat = NULL;
> >> > > > >          const char *id = NULL;
> >> > > > > +        const char *realm;
> >> > > > >  
> >> > > > >          ret = sysdb_attrs_get_string(reply[0], IPA_FLATNAME, &flat);
> >> > > > >          if (ret != EOK) {
> >> > > > > @@ -1427,8 +1428,15 @@ static void ipa_subdomains_handler_master_done(struct tevent_req *req)
> >> > > > >              goto done;
> >> > > > >          }
> >> > > > >  
> >> > > > > +        realm = dp_opt_get_string(ctx->sd_ctx->id_ctx->ipa_options->basic,
> >> > > > > +                                  IPA_KRB5_REALM);
> >> > > > > +        if (realm == NULL) {
> >> > > > > +            DEBUG(SSSDBG_CRIT_FAILURE, "No Kerberos realm for IPA?\n");
> >> > > > > +            goto done;
> >> > > > 
> >> > > > ret is not set
> >> > > 
> >> > > Thanks, fixed.
> >> > > 
> >> > > > 
> >> > > > > +        }
> >> > > > > +
> >> > > > >          ret = sysdb_master_domain_add_info(ctx->sd_ctx->be_ctx->domain,
> >> > > > > -                                           flat, id, NULL);
> >> > > > > +                                           realm, flat, id, NULL);
> >> > > > >      } else {
> >> > > > >          ctx->search_base_iter++;
> >> > > > >          ret = ipa_subdomains_handler_get(ctx, IPA_SUBDOMAINS_MASTER);
> >> > > > 
> >> 
> >> Sumit acked this one on IRC.
> >> 
> >> RB: you
> >> 
> >> > > > =====================================================
> >> > > > SYSDB: Add a forest root attribute to sss_domain_info
> >> > > > 
> >> > > > In link_forest_roots() you use get_next_domain() which iirc skips disabled
> >> > > > domains. I wonder if it would be better here to really check all domains even
> >> > > > disabled ones?
> >> > > 
> >> > > probably yes, Lukas asked for something similar when he was reviewing
> >> > > the get_next_domain() unit tests, so it makes sense. But since it's a
> >> > > corner case, let's do it after 1.13 Alpha:
> >> > >     https://fedorahosted.org/sssd/ticket/2673
> >> > 
> >> > ok
> >> > 
> >> > > > 
> >> > > > About tests, if you agree with handling disabled domains as well I would
> >> > > > recommend to disable one or the other domain in the test, maybe even a forest
> >> > > > root.
> >> > > 
> >> > > Will do that together with #2673.
> >> > 
> >> > ok
> >> > 
> >> > > 
> >> > > > Additionally it might be good to add some additional domains which are
> >> > > > not related to the forest, i.e. additional domains configured in sssd.conf. In
> >> > > > test_sysdb_link_forest_root_ipa() you should check that the IPA domain as well.
> >> > > 
> >> > > OK, I amended the existing tests and added
> >> > > test_sysdb_link_ad_multidom(), do they look better now?
> >> > 
> >> > ACK
> >> 
> >> RB: you
> >> 
> >> > 
> >> > > 
> >> > > > 
> >> > > > =========================================================
> >> > > > IPA: Add ipa_subdomains_handler_get_{start,cont} wrappers
> >> > > > 
> >> > > > ACK
> >> > > 
> >> > > Added R-B, otherwise no changes
> >> > > 
> >> > > > 
> >> > > > ========================================================
> >> > > > IPA: Check master domain record before subdomain records
> >> > > > 
> >> > > > ACK
> >> > > 
> >> > > Added R-B, otherwise no changes
> >> > > 
> >> > > > 
> >> > > > =====================================================
> >> > > > IPA: Fold ipa_subdom_enumerates into ipa_subdom_store
> >> > > > 
> >> > > > ACK
> >> > > 
> >> > > Added R-B, otherwise no changes
> >> > > 
> >> > > > 
> >> > > > ===============================================================
> >> > > > IPA: Also update master domain when initializing subdom handler
> >> > > > 
> >> > > > ACK
> >> > > > 
> >> > > > ====================================================
> >> > > > IPA: Move server-mode functions to a separate module
> >> > > > 
> >> > > > I would have been easier for review if you have split the moving of the
> >> > > > functions into a new file and the code changes into separate patches.
> >> > > 
> >> > > That's a fair comment, sorry about that.
> >> > > 
> >> > > > 
> >> > > > In ipa_ad_subdom_remove() you removed the server mode check, are you sure it
> >> > > > will be only called in server mode? You removed the check in
> >> > > > ipa_ad_subdom_refresh() and ipa_ad_subdom_init() as well but added checks in
> >> > > > the caller. I'm not sure I like this change at all. If you prefer it this way I
> >> > > > would recommend to use a new prefix, e.g. ipasrv_*, to indicate that it is only
> >> > > > safe to call those functions in server mode.
> >> > > 
> >> > > I reverted to the previous style where the function checks for server
> >> > > mode internally. For this patch it's better, because then it's really
> >> > > mostly move -- only parameters of function changed to not require the
> >> > > subdomain context, but rather the structures they need. The diff should
> >> > > be easier to compare with no suprises anymore.
> >> > > 
> >> > > IIRC the reason I changed the order was that it seemed more
> >> > > efficient whent he subdomains handler is later asynchronous, ie
> >> > > when we call ipa_server_create_trusts_send() intead of synchronous
> >> > > ipa_ad_subdom_refresh(). Do you think it would also be more reasable to
> >> > > always call ipa_server_create_trusts_send() and continue in the tevent
> >> > > handler even in client mode?
> >> > > 
> >> > > I no longer think efficiency matters, this code is quite rare and
> >> > > doesn't have to be too optimal, but readability is important.
> >> > 
> >> > ACK
> >> 
> >> RB: you
> >> 
> >> > 
> >> > > 
> >> > > > 
> >> > > > ==================================
> >> > > > IPA: Split two functions to new module ipa_subdomains_utils.c
> >> > > > 
> >> > > > ACK, but you might want to fix the author of ipa_subdomains_utils.c (and of
> >> > > > some ofther new files in other patches as well :-)
> >> > > 
> >> > > I would honestly prefer to stop using the author tag in SSSD completely.
> >> > > 
> >> > > It serves no purpose in the age of git-blame and git-log and it's just
> >> > > difficult to use (when I move code, should I put myself or the person
> >> > > who wrote the code?)
> >> > 
> >> > I agree with the arguments but this should be discussed with all
> >> > authors, but please keep in mind that all the git magic is lost when you
> >> > release a tar ball. I'm by far not an expert here but I guess since we
> >> > do releases based on tar balls the content of the tar ball is all that
> >> > matters and not some additional information which might be available in
> >> > some git repo or a mailing list.
> >> 
> >> Well, consumers of tarballs typically don't care about VCS metadata..
> >> 
> >> But let's have this discussion sometimes later :-)
> >> 
> >> > 
> >> > > 
> >> > > > 
> >> > > > =========================================================================
> >> > > > IPA: Include ipaNTTrustDirection in the attribute set for trusted domains
> >> > > > 
> >> > > > Please see my comments about the sysdb trust direction flags above.
> >> > > > 
> >> > > > > +        switch (ipa_trust_direction) {
> >> > > > > +        case LSA_TRUST_DIRECTION_OUTBOUND:
> >> > > > > +            direction = SD_TRUST_DIRECTION_OUTBOUND;
> >> > > > > +            break;
> >> > > > > +        case (LSA_TRUST_DIRECTION_INBOUND | LSA_TRUST_DIRECTION_OUTBOUND):
> >> > > > > +            direction = SD_TRUST_DIRECTION_TWOWAY;
> >> > > > 
> >> > > > This looks odd, even if you want to set SD_TRUST_DIRECTION_TWOWAY explicitly
> >> > > > only for a two-way trust. In this case I would expect
> >> > > > (LSA_TRUST_DIRECTION_INBOUND & LSA_TRUST_DIRECTION_OUTBOUND). But in general
> >> > > > just LSA_TRUST_DIRECTION_INBOUND should be sufficient to cover the case where
> >> > > > the host keytab might be used.
> >> > > 
> >> > > I just removed this whole logic. Instead, the only logic is fallback to
> >> > > INBOUND for old servers where the ACIs don't allow us to read the
> >> > > direction. Otherwise, handling of the trust direction will be done when
> >> > > the corresponding AD object is instantiated.
> >> > 
> >> > ok, but I think ipa_trust_dir2str() should be modified to better explain
> >> > the values, i.e. only when both flags are set it should say two-way. If
> >> > only one flags is set it should say one-way (incoming/outgoing)
> >> > respectively.
> >> 
> >> Done.
> >> 
> >> I also filed:
> >>     https://fedorahosted.org/sssd/ticket/2677
> >> 
> >> To check again how we use the zero value of trust direction.
> >> 
> >> > 
> >> > > 
> >> > > > 
> >> > > > > +            break;
> >> > > > > +        default:
> >> > > > > +            return ERR_TRUST_NOT_SUPPORTED;
> >> > > > > +        }
> >> > > > > 
> >> > > > 
> >> > > > ======================================================
> >> > > > IPA: Read forest name for trusted forest roots as well
> >> > > > 
> >> > > > ACK, only a really minor comment, I would have used 'forest' in the debug
> >> > > > message and not 'name' but I don't mind either way, I trust talloc_strdup()
> >> > > > here.
> >> > > 
> >> > > You're right, that was a bug. A minor one, bug a bug nonetheless.
> >> > 
> >> > ACK
> >> > 
> >> > > 
> >> > > > 
> >> > > > ======================================================= 
> >> > > > IPA: Make constructing an IPA server mode context async
> >> > > > 
> >> > > > >      trust_ctx->dom = subdom;
> >> > > > >      trust_ctx->ad_id_ctx = ad_id_ctx;
> >> > > > >  
> >> > > > >      DLIST_ADD(id_ctx->server_mode->trusts, trust_ctx);
> >> > > > > +    ret = EOK;
> >> > > > > +    goto immediate;
> >> > > > 
> >> > > > This looks a bit odd and I would ask to add a comment here but since this whole
> >> > > > part will be replaced in a few patches later I think it is ok.
> >> > > 
> >> > > Good idea, comment added..
> >> > > 
> >> > > > 
> >> > > > > +
> >> > > > > +    return req;
> >> > > > > +
> >> > > > > +immediate:
> >> > > > > +    if (ret != EOK) {
> >> > > > > +        tevent_req_error(req, ret);
> >> > > > > +    } else {
> >> > > > > +        tevent_req_done(req);
> >> > > > > +    }
> >> > > > > +    tevent_req_post(req, ev);
> >> > > > > +    return req;
> >> > > > > +}
> >> > > > 
> >> > > > ACK
> >> > 
> >> > ACK again
> >> > 
> >> > > > 
> >> > > > =====================================================
> >> > > > TESTS: Split off keytab creation into a common module
> >> > > > 
> >> > > > ACK, but please fix Author and Copyright date.
> >> > > 
> >> > > Done. Because I only touched the comment section, I added R-B: you
> >> > > 
> >> > > > 
> >> > > > ========================================
> >> > > > TESTS: Add a common mock_be_ctx function
> >> > > > 
> >> > > > ACK, but please fix Author and Copyright date.
> >> > > 
> >> > > Done. Because I only touched the comment section, I added R-B: you
> >> > > 
> >> > > > 
> >> > > > ==================================================
> >> > > > TESTS: Add a common function to set up sdap_id_ctx
> >> > > > 
> >> > > > ACK
> >> > > 
> >> > > Added R-B: you
> >> > > 
> >> > > > 
> >> > > > ===============================================
> >> > > > TESTS: Move krb5_try_kdcip to nested group test
> >> > > > 
> >> > > > Since krb5_try_kdcip() is no specific to the nested group test, shouldn't it be
> >> > > > removed from common_mock_sdap.h as well?
> >> > > 
> >> > > Yes, it should. Done.
> >> > > 
> >> > > > 
> >> > > > As an alternative it might be possible to proper mock krb5_try_kdcip with
> >> > > > '-Wl,-wrap,krb5_try_kdcip' to make it save to link it with krb5_common.c?
> >> > > 
> >> > > I think we can move the function local to the test for now..
> >> > 
> >> > ACK
> >> 
> >> RB: you
> >> 
> >> > 
> >> > > 
> >> > > > 
> >> > > > ======================================================
> >> > > > TESTS: Add unit test for the subdomain_server.c module
> >> > > > 
> >> > > > ACK
> >> > > 
> >> > > Added R-B: you
> >> > > 
> >> > > Thank you very much for the review. So far I pushed the patches to my
> >> > > fedorapeople branch. I'll continue with the second review now.
> >> > 
> >> > 
> >> > > On Tue, Jun 09, 2015 at 01:22:20PM +0200, Sumit Bose wrote:
> >> > > > =================================
> >> > > > IPA: Fetch keytab for 1way trusts
> >> > > > 
> >> > > > Why do you have to wrap execl(), looks like only execle() is used?
> >> > > 
> >> > > You're right, I don't. I just used execl first, then converted to
> >> > > execle() because I need to set the right KRB5CCNAME, but forgot to
> >> > > remove the old wrapper after testing..
> >> > > 
> >> > > Thanks, removed.
> >> > > 
> >> > > > 
> >> > > > > +    errno_t ret;
> >> > > > > +    struct tevent_req *req = NULL;
> >> > > > > +    struct ipa_getkeytab_state *state;
> >> > > > > +    pid_t child_pid;
> >> > > > > +    struct timeval tv;
> >> > > > > +
> >> > > > > +    req = tevent_req_create(mem_ctx, &state, struct ipa_getkeytab_state);
> >> > > > > +    if (req == NULL) {
> >> > > > > +        return NULL;
> >> > > > > +    }
> >> > > > > +    state->child_status = 0;
> >> > > > 
> >> > > > Maybe it would be safer to set this to a generic error like EFAULT to make sure
> >> > > > 0/EOK is only returned if the request is successful?
> >> > > 
> >> > > Yes, good idea. As you noted, there is a bit of copy-paste in this
> >> > > area..
> >> > > 
> >> > > > 
> >> > > > > +
> >> > > > > +    if (server == NULL || principal == NULL || keytab == NULL) {
> >> > > > > +        ret = EINVAL;
> >> > > > > +        goto done;
> >> > > > > +    }
> >> > > > 
> >> > > > Just a general comment, maybe it would help to reduce code duplication if
> >> > > > child_handler_setup() would create the timeout handler as well? Shall we add a
> >> > > > ticket?
> >> > > 
> >> > > Yes: https://fedorahosted.org/sssd/ticket/2674
> >> > > 
> >> > > > 
> >> > > > > +    gkt_env[0] = talloc_asprintf(NULL, "KRB5CCNAME=%s", ccache);
> >> > > > > +    if (gkt_env[0] == NULL) {
> >> > > > > +        DEBUG(SSSDBG_CRIT_FAILURE, "OOM\n");
> >> > > > 
> >> > > > Since SSSDBG_CRIT_FAILURE messages are expected to be read by users/admins I
> >> > > > think it should be a bit more explicit.
> >> > > 
> >> > > Yes, sorry. I initially wrote this part of code as a quick for for ab
> >> > > and then just squashed into the patchset.
> >> > > 
> >> > > > 
> >> > > > > +        exit(1);
> >> > > > > +    }
> >> > > > > +
> >> > > > > +    errno = 0;
> >> > > > > +    execle(IPA_GETKEYTAB_PATH, IPA_GETKEYTAB_PATH,
> >> > > > > +           "-r", "-s", server, "-p", principal, "-k", keytab_path, NULL,
> >> > > > > +           gkt_env);
> >> > > > 
> >> > > > It's academic but I guess catching the return value of execle() would help to
> >> > > > avoid some complaints from static code-analyzers.
> >> > > 
> >> > > OK, to avoid static code-analyzers from complaining that the value of
> >> > > ret is read, but never used, I also added another DEBUG message.
> >> > > 
> >> > > > 
> >> > > > > +
> >> > > > > +    /* The child should never end up here */
> >> > > > > +    ret = errno;
> >> > > > > +    DEBUG(SSSDBG_CRIT_FAILURE,
> >> > > > > +          "execv failed [%d][%s].\n", ret, sss_strerror(ret));
> >> > > > > +    exit(1);
> >> > 
> >> > >     errno = 0;
> >> > > +    ret = execle(IPA_GETKEYTAB_PATH, IPA_GETKEYTAB_PATH,
> >> > > +                 "-r", "-s", server, "-p", principal, "-k",
> >> > > keytab_path, NULL,
> >> > > +                 gkt_env);
> >> > > +
> >> > > +    DEBUG(SSSDBG_CRIT_FAILURE,
> >> > > +          "execle returned %d, this shoudln't happen!\n", ret);
> >> > 
> >> >                                      typo ^^^^
> >> 
> >> Fixed
> >> 
> >> > 
> >> > > +
> >> > > +    /* The child should never end up here */
> >> > > +    ret = errno;
> >> > > +    DEBUG(SSSDBG_CRIT_FAILURE,
> >> > > +          "execv failed [%d][%s].\n", ret, sss_strerror(ret));
> >> > 
> >> >         typo  ^^^^^
> >> 
> >> Fixed
> >> 
> >> > > +    exit(1);
> >> > > +}
> >> > 
> >> > There is still __wrap_execl() defined which is unused.
> >> 
> >> Oops, removed.
> >> 
> >> I also added a new valgrind suppression so that our CI actually works.
> >> (Thanks Nikolai for helping me with that!) and linked the test with
> >> KEYUTILS_LIBS otherwise Debian linker would complain.
> >> 
> >> > 
> >> > 
> >> > > > 
> >> > > > ======================================================
> >> > > > AD: Rename ad_set_ad_id_options to ad_set_sdap_options
> >> > > > 
> >> > > > ACK
> >> > > 
> >> > > RB: you
> >> > > 
> >> > > > 
> >> > > > ====================================================================
> >> > > > AD: Rename ad_create_default_options to ad_create_2way_trust_options
> >> > > > 
> >> > > > ACK
> >> > > 
> >> > > RB: you
> >> 
> >> I added KEYUTILS_LIBS to LDADD of the test, otherwise the tests on
> >> Debian wouldn't link properly. No other changes, so I retained the RB
> >> tag.
> >> 
> >> > > 
> >> > > > 
> >> > > > =======================================
> >> > > > AD: Split off ad_create_default_options
> >> > > > 
> >> > > > > +static void test_ad_create_default_options(void **state)
> >> > > > > +{
> >> > > > > +    struct ad_options *ad_options;
> >> > > > > +    const char *s;
> >> > > > > +
> >> > > > > +    ad_options = ad_create_default_options(global_talloc_context);
> >> > > > > +
> >> > > > > +    assert_non_null(ad_options->basic);
> >> > > > > +
> >> > > > > +    /* Not too much to test here except some defaults */
> >> > > > > +    s = dp_opt_get_string(ad_options->basic, AD_DOMAIN);
> >> > > > > +    assert_null(s);
> >> > > > 
> >> > > > Maybe you can check the value as well?
> >> > > 
> >> > > The default domain is NULL..
> >> > 
> >> > ah, sorry, I misread the test
> >> > 
> >> > ACK
> >> 
> >> Added RB: you
> >> 
> >> I admit the test is not the smartest. But if you look at
> >> ad_basic_opts[], it's mostly NULLs.
> >> 
> >> Maybe I could extend the test to just iterate over ad_basic_opts[] and
> >> compare the right values? But I don't think we should hold off this
> >> patchset for that..
> >> 
> >> > 
> >> > > 
> >> > > maybe we could check other parameters, like that we default to GC
> >> > > first..
> >> > > 
> >> > > > 
> >> > > > > +
> >> > > > > +    assert_non_null(ad_options->id);
> >> > > > > +}
> >> > > > 
> >> > > > ========================================================
> >> > > > IPA/AD: Set up AD domain in ad_create_2way_trust_options
> >> > > > 
> >> > > > ACK
> >> > > 
> >> > > RB: you
> >> > > 
> >> > > > 
> >> > > > ===================================
> >> > > > IPA: Do not set AD_KRB5_REALM twice
> >> > > > 
> >> > > > ACK
> >> > > 
> >> > > RB: you
> >> > > 
> >> > > > 
> >> > > > ====================================
> >> > > > AD: Add ad_create_1way_trust_options
> >> > > > 
> >> > > > Typo in the commit message 'unline'.
> >> > > 
> >> > > Fixed and added RB: you
> >> > > 
> >> > > > 
> >> > > > ==========================================================
> >> > > > IPA: Utility function for setting up one-way trust context
> >> > > > 
> >> > > > > +    ad_options = ad_create_1way_trust_options(id_ctx,
> >> > > > > +                                              ad_domain,
> >> > > > > +                                              id_ctx->server_mode->hostname,
> >> > > > > +                                              keytab,
> >> > > > > +                                              principal);
> >> > > > > +    if (ad_options == NULL) {
> >> > > > > +        talloc_free(keytab);
> >> > > > > +        talloc_free(principal);
> >> > > > > +        return NULL;
> >> > > > > +    }
> >> > > > > +
> >> > > > > +    talloc_steal(ad_options, keytab);
> >> > > > > +    talloc_steal(ad_options, principal);
> >> > > > 
> >> > > > Why do you have to keep keytab and principal around? They should have been
> >> > > > talloc_strdup'd in ad_create_1way_trust_options.
> >> > > 
> >> > > Yes, I don't think we do.
> >> > 
> >> > ACK
> >> > > 
> >> > > > 
> >> > > > > +    return ad_options;
> >> > > > 
> >> > > > ================================================================================
> >> > > > WORKAROUND: setting keytab through environment variable is no longer a good idea
> >> > > > 
> >> > > > vs
> >> > > > 
> >> > > > remove setenvs
> >> > > > 
> >> > > > Which is the right one?
> >> > > 
> >> > > Neither, I didn't mean to send them for review in the current state.
> >> > > 
> >> > > But for Alpha, I will prepare a patch that does setenv/unsetenv and for
> >> > > Beta, I'll finish testing ccache collections.
> >> > 
> >> > ok, your current oneway branch still has those two.
> >> 
> >> Yeah, sorry, I used the branch for development/experiments along with
> >> the review which I shouldn't do. I have a separate oneway-wip branch
> >> now..
> >> 
> >> Actually, I think we can just remove the setenvs with keytabs now. It's
> >> the right thing to do, because ldap_child learns about the keytab
> >> through the packet we write it to and other processes has no business
> >> about the keytab (yes, during Beta development, I need to test
> >> referrals..when I work on making ldap_child using ccache collection..)
> >> 
> >> So I squashed the two together and added a nicer commit message. 
> >> 
> >> 
> >> > 
> >> > > 
> >> > > > 
> >> > > > ===========================================================
> >> > > > LDAP: Consolidate SDAP_SASL_REALM/SDAP_KRB5_REALM behaviour
> >> > > > 
> >> > > > ACK
> >> > > 
> >> > > Ah, thanks, I didn't mean to send this one for review yet, but I'm glad
> >> > > you like it.
> >> > 
> >> > yes, it adds a slight change of behaviour to sdap_gssapi_init() but I
> >> > would say it is the expected behaviour anyway.
> >> 
> >> Thanks, I added RB: you. I hope for such a simple function it's OK to
> >> not have a test for now.
> >> 
> >> CI now passes also after adding the valgrind suppression and changing
> >> the LDADD for unit tests so that Debian links fine:
> >>     http://sssd-ci.duckdns.org/logs/job/17/07/summary.html
> >> 
> >> New patches are pushed to my fedorapeople branch:
> >>    https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=oneway 
> >
> >ok, ACK to all:
> >d2caa89 LDAP: Consolidate SDAP_SASL_REALM/SDAP_KRB5_REALM behaviour
> >ac69a24 LDAP: Do not set keytab through environment variable
> >ded7d0a IPA: Utility function for setting up one-way trust context
> >5a1b409 AD: Add ad_create_1way_trust_options
> >f807e08 IPA: Do not set AD_KRB5_REALM twice
> >49dc4bf IPA/AD: Set up AD domain in ad_create_2way_trust_options
> >78f2ba4 AD: Split off ad_create_default_options
> >a6f133e AD: Rename ad_create_default_options to ad_create_2way_trust_options
> >5347cd9 AD: Rename ad_set_ad_id_options to ad_set_sdap_options
> >1f0cc0b IPA: Fetch keytab for 1way trusts
> >85557c2 TESTS: Add unit test for the subdomain_server.c module
> >85d0fc3 TESTS: Move krb5_try_kdcip to nested group test
> >ca7b814 TESTS: Add a common function to set up sdap_id_ctx
> >75edc2b TESTS: Add a common mock_be_ctx function
> >5022a15 TESTS: Split off keytab creation into a common module
> >14e4199 IPA: Make constructing an IPA server mode context async
> >91c6b99 IPA: Read forest name for trusted forest roots as well
> >f30d84c IPA: Include ipaNTTrustDirection in the attribute set for trusted domains
> >3d1037b IPA: Split two functions to new module ipa_subdomains_utils.c
> >d18abe9 IPA: Move server-mode functions to a separate module
> >8f5b267 IPA: Also update master domain when initializing subdom handler
> >d2708b5 IPA: Fold ipa_subdom_enumerates into ipa_subdom_store
> >b795f2e IPA: Check master domain record before subdomain records
> >0148f4e IPA: Add ipa_subdomains_handler_get_{start,cont} wrappers
> >ae4e23f SYSDB: Add a forest root attribute to sss_domain_info
> >5bbff18 SYSDB: Add realm to sysdb_master_domain_add_info
> >3948141 TESTS: Add a test for sysdb_subdomains.c
> >0fa850e UTIL/SYSDB: Move new_subdomain() to sysdb_subdomains.c and make it private
> >ef75425 SYSDB: Store trust direction for subdomains
> >
> >
> >
> >CI passes for me as well
> >http://sssd-ci.duckdns.org/logs/job/17/08/summary.html
> >
> CI didn't pass.
> http://sssd-ci.duckdns.org/logs/job/17/38/fedora_rawhide/ci-build-debug/ci-make-intgcheck.log
> 
> Integration test are failing with each commit.
> 
> As a part of teardown, it tried to remove
> files from directory ${prefix}/var/lib/sss/db/
> 
> However it failed becuase there is a new directory
> ${prefix}/var/lib/sss/db/keytabs

Sounds like a bug in test :-)

btw why didn't we see this when both me and Sumit tried running CI ? We
really need to run the tests automatically or else they have limited
value, sorry. We can't expect developers to add magic switches to the CI
calls :-)

I run the tests with:
    git push ci HEAD:master
Where CI is our CI machine remote.

I was also running contrib/ci/run during development and didn't see the
failures..

> 
> Users are used to cleaning cache with rm -rf /var/lib/sss/db/*
> The keytabs directory will be removed but it won't be created.
> Because it is created only as part of installation
> (the spec contains "%attr(700,sssd,sssd) %dir %{dbpath}/keytabs")
> 
> Could we rather use "${prefix}/var/lib/sss/keytabs"?

I guess yes, normally you shouldn't presume anything if you remove files
from project's private directory but it's true that many users are
unfortunately used to rm files in the cache dir. So this would cause
support calls..

We already have gpo_cache, so we can move the files into sss/keytabs..



More information about the sssd-devel mailing list