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

Sumit Bose sbose at redhat.com
Mon Jun 8 15:03:02 UTC 2015


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.

For SD_TRUST_DIRECTION_NOT_SET you should pick a suitable value, maybe 128?

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.

============================================================================
- UTIL/SYSDB: Move new_subdomain() to sysdb_subdomains.c and make it private

ACK

==========================================
- TESTS: Add a test for sysdb_subdomains.c

ACK

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

> 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

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

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

=========================================================
IPA: Add ipa_subdomains_handler_get_{start,cont} wrappers

ACK

========================================================
IPA: Check master domain record before subdomain records

ACK

=====================================================
IPA: Fold ipa_subdom_enumerates into ipa_subdom_store

ACK

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

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.

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

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

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

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

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

========================================
TESTS: Add a common mock_be_ctx function

ACK, but please fix Author and Copyright date.

==================================================
TESTS: Add a common function to set up sdap_id_ctx

ACK

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

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?

======================================================
TESTS: Add unit test for the subdomain_server.c module

ACK

> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list