On Thu, 03 Oct 2013, Sumit Bose wrote:
On Thu, Oct 03, 2013 at 06:04:24PM +0200, Martin Kosek wrote:
On 10/03/2013 03:10 PM, Alexander Bokovoy wrote:
> On Wed, 02 Oct 2013, Sumit Bose wrote:
>>> Please note that I did not test with more than 1 subdomain, since I
>>> do not have more ADs available.
>>>
>>
>> I have done some testing as well and the patches are working as expected
>> except the trustdomain-disable issue Tomas mentioned. But I think it
>> would be sufficient to add a comment to the release notes and fix this
>> with the next release to not delay this release anymore.
>>
>> The patches are also working for trusts which were added with older
>> releases. So ACK from my side for the functional part.
> New patchset is attached. I've fixed all outstanding issues and
> implemented proper SID filtering for subdomains. In addition, I've
> added MS-PAC cache eviction when we change blacklists from IPA side
> and forced removal of the domain from SID blacklist if the domain is
> being removed by trustdomain-del.
>
1) Minor issue in 0118:
+ if keys[0].lower() == keys[1].lower():
+ raise errors.ValidationError(name='trustdomain_enable',
+ error=_("Root domain of the trust is always enabled for the
existing trust"))
The error message looks weird (double trustdomain_enable)
# ipa trustdomain-enable realm domain
ipa: ERROR: invalid 'trustdomain_enable': Root domain of the trust is always
enabled for the existing trust
I would rather do something like
+ raise errors.ValidationError(name='domain',
2) trustconfig-enable and trustconfig-disable should use standard output like
other enable/disable methods. See user-enable/user-disable for example. Current
situation puts all the authoritative information to summary which:
a) Does not look nice in terminal
# ipa trustdomain-disable very.long.long.long.realm very.long.long.long.domain
----------------------------------------------------------------------------------------------------------------------------
Domain very.long.long.long.domain of trust very.long.long.long.realm is not
allowed to access IPA resources
----------------------------------------------------------------------------------------------------------------------------
b) How am I supposed to parse an information about the result if all I get is a
text in summary? Using standard errors and output values will allow easier
consumption of the API later (like in Web UI).
I am attaching a patch (0001) how to make it consistent with other
enable/disable commands. Example:
# ipa trustdomain-disable realm domain
ipa: ERROR: This entry is already disabled
# ipa trustdomain-enable realm domain
-----------------------------
Enabled trust domain "domain"
-----------------------------
3) Let's use standard primary key for the trustdomain object. This will let us
overcome some hacks and also let us use handle_not_found method - patch
attached (0002).
0002 also changes some ValidationError errors to standard errors, just for
being consistent with the rest of the API.
Note that in order to make primary_key=True, I had to enhance trustdomain_del
command to manage multiple domains.
I think these API fixes are a must, it would be very hard to amend the API
later. If these patches are squashed to your 0118, it would be ACK from me to
the Python side. I will let C parts and a functional test to Sumit's mighty
hands.
Functional it is an ACK to all patches except 0123. The
trustdomain-disabled issue found by Tomas is fixed with 0124.
Patch 0123 is not needed and breaks setups with unpatched MIT Kerberos,
which currently are more or less all. It does not allow users from the
trusted forest root to get tickets for the IPA domain. In this case the
TGT does not have any transited data, because it is a direct trust, but
the client realm is not ours. So the plugin returns
KRB5_PLUGIN_NO_HANDLE which is interpreted as an error in current MIT
Kerberos versions. I would recommend to just drop the patch for this
release and include an improved version in the next.
I've fixed 0123 by allowing to proceed with the trust checks in case of
empty transited realm and both client and server realms are not ours.
The result will be success in case both client and server realms are
either our realm or belong to any of the trusted domains. For this case
empty transited realm considered a match.
For me following cases work now:
- principal from our realm asking for ticket of a service in our realm
- principal from root level trusted domain asking for a ticket of a
service in our realm
- principal from a trusted domain asking for a ticket of a service in
our realm
- principal from our realm asking for a ticket of a service in a root
level trusted domain
- principal from our realm asking for a ticket of a service in a
trusted domain
These are supported use cases without your PoC patches, the last one
works due to manual configuration file written by SSSD 1.11.3.
Patch 0124 look good although the new parent_name member looks a bit
useless to me. I also find the name ipadb_free_sid_blacklists() a bit
irritating because it is not clear (from the name) if it should be used
to free the lists of SID strings or of struct dom_sid. But both can be
fixed (if needed) later when we touch this part of the code again to
include support for transitively trusted forests.
We reallocate trusts array as we parse entries returned by the LDAP
query. It means we don't really know complete size of the trusts array
in advance. I would need to keep a book aside to record trust name and
its parent name to perform resolution later. It would be additional
hassle, so I simply added a parent name to the trust struct.
I don't think this needs refactoring as it would bring more complexity
than warranted.
ipadb_free_sid_blacklists() can be renamed to ipadb_free_sid_string_lists() if
needed.
--
/ Alexander Bokovoy
>From 3927619695773abb9882e93584353270aed29699 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <[email protected]>
Date: Sat, 28 Sep 2013 21:49:57 +0200
Subject: [PATCH 7/8] KDC: implement transition check for trusted domains
When client principal requests for a ticket for a server principal
and we have to perform transition, check that all three belong to either
our domain or the domains we trust through forest trusts.
In case all three realms (client, transition, and server) match
trusted domains and our domain, issue permission to transition from client
realm to server realm.
Part of https://fedorahosted.org/freeipa/ticket/3909
---
daemons/ipa-kdb/ipa_kdb.c | 2 +-
daemons/ipa-kdb/ipa_kdb.h | 5 +++-
daemons/ipa-kdb/ipa_kdb_mspac.c | 63 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 68 insertions(+), 2 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb.c b/daemons/ipa-kdb/ipa_kdb.c
index 5e4d047..c807bbc 100644
--- a/daemons/ipa-kdb/ipa_kdb.c
+++ b/daemons/ipa-kdb/ipa_kdb.c
@@ -602,7 +602,7 @@ kdb_vftabl kdb_function_table = {
NULL, /* decrypt_key_data */
NULL, /* encrypt_key_data */
ipadb_sign_authdata, /* sign_authdata */
- NULL, /* check_transited_realms */
+ ipadb_check_transited_realms, /* check_transited_realms */
ipadb_check_policy_as, /* check_policy_as */
NULL, /* check_policy_tgs */
ipadb_audit_as_req, /* audit_as_req */
diff --git a/daemons/ipa-kdb/ipa_kdb.h b/daemons/ipa-kdb/ipa_kdb.h
index f4d3555..1c2aefc 100644
--- a/daemons/ipa-kdb/ipa_kdb.h
+++ b/daemons/ipa-kdb/ipa_kdb.h
@@ -253,7 +253,10 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
krb5_error_code ipadb_reinit_mspac(struct ipadb_context *ipactx, bool
force_reinit);
void ipadb_mspac_struct_free(struct ipadb_mspac **mspac);
-
+krb5_error_code ipadb_check_transited_realms(krb5_context kcontext,
+ const krb5_data *tr_contents,
+ const krb5_data *client_realm,
+ const krb5_data *server_realm);
/* DELEGATION CHECKS */
krb5_error_code ipadb_check_allowed_to_delegate(krb5_context kcontext,
diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index 08b55af..e20de36 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -2490,3 +2490,66 @@ done:
ldap_msgfree(result);
return kerr;
}
+
+krb5_error_code ipadb_check_transited_realms(krb5_context kcontext,
+ const krb5_data *tr_contents,
+ const krb5_data *client_realm,
+ const krb5_data *server_realm)
+{
+ struct ipadb_context *ipactx;
+ bool has_transited_contents, has_client_realm, has_server_realm;
+ int i;
+ krb5_error_code ret;
+
+ ipactx = ipadb_get_context(kcontext);
+ if (!ipactx || !ipactx->mspac) {
+ return KRB5_KDB_DBNOTINITED;
+ }
+
+ has_transited_contents = false;
+ has_client_realm = false;
+ has_server_realm = false;
+
+ /* First, compare client or server realm with ours */
+ if (strncasecmp(client_realm->data, ipactx->realm,
client_realm->length) == 0) {
+ has_client_realm = true;
+ }
+ if (strncasecmp(server_realm->data, ipactx->realm,
server_realm->length) == 0) {
+ has_server_realm = true;
+ }
+
+ if ((tr_contents->length == 0) || (tr_contents->data[0] == '\0')) {
+ /* For in-realm case allow transition */
+ if (has_client_realm && has_server_realm) {
+ return 0;
+ }
+ /* Since transited realm is empty, we don't need to check for
it, it is a direct trust case */
+ has_transited_contents = true;
+ }
+
+ if (!ipactx->mspac || !ipactx->mspac->trusts) {
+ return KRB5_PLUGIN_NO_HANDLE;
+ }
+
+ /* Iterate through list of trusts and check if any of input belongs to
any of the trust */
+ for(i=0; i < ipactx->mspac->num_trusts ; i++) {
+ if (!has_transited_contents &&
+ (strncasecmp(tr_contents->data,
ipactx->mspac->trusts[i].domain_name, tr_contents->length) == 0)) {
+ has_transited_contents = true;
+ }
+ if (!has_client_realm &&
+ (strncasecmp(client_realm->data,
ipactx->mspac->trusts[i].domain_name, client_realm->length) == 0)) {
+ has_client_realm = true;
+ }
+ if (!has_server_realm &&
+ (strncasecmp(server_realm->data,
ipactx->mspac->trusts[i].domain_name, server_realm->length) == 0)) {
+ has_server_realm = true;
+ }
+ }
+
+ ret = KRB5KRB_AP_ERR_ILL_CR_TKT;
+ if (has_client_realm && has_transited_contents && has_server_realm) {
+ ret = 0;
+ }
+ return ret;
+}
--
1.8.3.1
_______________________________________________
Freeipa-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/freeipa-devel
