[Freeipa-devel] [PATCH] External group membership for trusted domains

[Sumit Bose]

Hi Alexander,

On Thu, Jun 21, 2012 at 06:26:02PM +0300, Alexander Bokovoy wrote:
> Hi!
> 
> Attached is the patch to support external group membership for trusted
> domains. This is needed to get proper group membership with the work
> Sumit and Jan are doing on both IPA and SSSD sides.
> 
> We already have ipaExternalGroup class that includes ipaExternalMember
> attribute (multivalued case-insensitive string). The group that has
> ipaExternalGroup object class will have to be non-POSIX and
> ipaExternalMember
> attribute will contain security identifiers (SIDs) of members from
> trusted domains.
> 
> The patch takes care of three things:
> 1. Extends 'ipa group-add' with --external option to add
>    ipaExternalGroup object class to a new group
> 2. Modifies 'ipa group-add-member' to accept --external CSV argument
>    to specify SIDs
> 3. Modifies 'ipa group-del-member' to allow removing external members.

thank you for the patch, it works as expected, but I have a few
comments:

- there is a trailing whitespace at the end of the "This means we can't
  check the correctness of a trusted domain SIDs" line
- when using ipa group-add-member with --external there are still prompt
  for [member user] and [member group], can those be suppressed?
- with ipa group-mod --posix it is possible to add the posxiGroup
  objectclass together with a GID to the extern group object. This
  should result in an error and also the other way round, adding
  --external to Posix groups.

bye,
Sumit

> 
> When adding new external member we also perform SID correctness checks.
> This is important part of the patch due to potential security
> implications of allowing random SIDs. SIDs are universal identifiers and
> can point to objects in own domain as well as any other. If so-called
> builtin SIDs are used, they are resolved against local domain which will
> allow granting permissions trusted domain user should have never had.
> 
> Below is how we do perform validation of SIDs:
> 1. Use Samba 4 bindings to parse SID and validate its format
> 2. If SID is outside S-1-5- prefix (SID_NT_AUTHORITY), we reject it.
> 3. If SID is from our own domain, we reject it.
> 4. If SID is from any of our trusted domains, we accept it
> 5. Otherwise we reject SID.
> 
> Here is real code:
> +    def is_trusted_sid_valid(self, sid):
> +        if not self.domain:
> +            # our domain is not configured or self.is_configured() never run
> +            # reject SIDs as we can't check correctness of them
> +            return False
> +        # Parse sid string to see if it is really in a SID format
> +        try:
> +            test_sid = security.dom_sid(sid)
> +        except TypeError:
> +            return False
> +        (dom, sid_rid) = test_sid.split()
> +        sid_dom = str(dom)
> +        # Now we have domain prefix of the sid as sid_dom string and can
> +        # analyze it against known prefixes
> +        if sid_dom.find(security.SID_NT_AUTHORITY) != 0:
> +            # Ignore any potential SIDs that are not S-1-5-*
> +            return False
> +        if sid_dom.find(self.sid) == 0:
> +            # A SID from our own domain cannot be treated as trusted domain's SID
> +            return False
> +        # At this point we have SID_NT_AUTHORITY family SID and really need to
> +        # check it against prefixes of domain SIDs we trust to
> +        if not self._domains:
> +            self._domains = self.get_trusted_domains()
> +        if len(self._domains) == 0:
> +            # Our domain is configured but no trusted domains are configured
> +            # This means we can't check the correctness of a
> trusted domain SIDs +            return False
> +        # We have non-zero list of trusted domains and have to go through them
> +        # one by one and check their sids as prefixes
> +        for (dn, domaininfo) in self._domains:
> +            if sid_dom.find(domaininfo[self.ATTR_TRUSTED_SID][0]) == 0:
> +                return True
> +        return False
> 
> 
> 
> -- 
> / Alexander Bokovoy