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

Lukas Slebodnik lslebodn at redhat.com
Mon Jun 15 06:43:56 UTC 2015


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

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"?

LS



More information about the sssd-devel mailing list