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.