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

Jakub Hrozek jhrozek at redhat.com
Wed Jun 10 19:30:43 UTC 2015


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.

> 
> ============================================================================
> - 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);
> 
> =====================================================
> 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
> 
> 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.

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

> 
> =========================================================
> 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.

> 
> ==================================
> 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?)

> 
> =========================================================================
> 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.

> 
> > +            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.

> 
> ======================================================= 
> 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
> 
> =====================================================
> 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..

> 
> ======================================================
> 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.



More information about the sssd-devel mailing list