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.
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.
bye,
Sumit
>
> Martin
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
