[SSSD] [PATCHES] Support one-way trusts with AD domains in IPA server mode
Sumit Bose
sbose at redhat.com
Tue Jun 9 11:22:20 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.
>
> 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
=================================
IPA: Fetch keytab for 1way trusts
Why do you have to wrap execl(), looks like only execle() is used?
> + 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?
> +
> + 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?
> + 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.
> + 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.
> +
> + /* The child should never end up here */
> + ret = errno;
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + "execv failed [%d][%s].\n", ret, sss_strerror(ret));
> + exit(1);
======================================================
AD: Rename ad_set_ad_id_options to ad_set_sdap_options
ACK
====================================================================
AD: Rename ad_create_default_options to ad_create_2way_trust_options
ACK
=======================================
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?
> +
> + assert_non_null(ad_options->id);
> +}
========================================================
IPA/AD: Set up AD domain in ad_create_2way_trust_options
ACK
===================================
IPA: Do not set AD_KRB5_REALM twice
ACK
====================================
AD: Add ad_create_1way_trust_options
Typo in the commit message 'unline'.
==========================================================
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.
> + return ad_options;
================================================================================
WORKAROUND: setting keytab through environment variable is no longer a good idea
vs
remove setenvs
Which is the right one?
===========================================================
LDAP: Consolidate SDAP_SASL_REALM/SDAP_KRB5_REALM behaviour
ACK
bye,
Sumit
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> _______________________________________________
> 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