[Freeipa-devel] [PATCH 0002] Port from python-krbV to python-gssapi

[Simo Sorce]

On Mon, 2015-08-24 at 17:18 +0200, Michael Šimáček wrote:
> On 2015-08-24 14:50, Jan Cholasta wrote:
> > On 23.8.2015 23:27, Michael Šimáček wrote:
> >>
> >>
> >> On 2015-08-21 15:52, Michael Šimáček wrote:
> >>>
> >>>
> >>> On 2015-08-20 20:42, Robbie Harwood wrote:
> >>>> Michael Šimáček <msimacek at redhat.com> writes:
> >>>>
> >>>>> On 2015-08-20 12:32, Michael Šimáček wrote:
> >>>>>
> >>>>>>>>> Michael Šimáček <msimacek at redhat.com> writes:
> >>>>>>>>>
> >>>>>>>>>> Attaching new revision of the patch. Changes from the previous:
> >>>>>>>>>> - ldap2's connect now chooses the bind type same way as in
> >>>>>>>>>> ipaldap
> >>>>>>>>>> - get_default_realm usages replaced by api.env.realm
> >>>>>>>>>> - fixed missing third kinit attempt in trust-fetch-domains
> >>>>>>>>>> - removed rewrapping gssapi errors to ccache errors in krb_utils
> >>>>>>>>>> - updated some parts of exception handling
> >>>>>>
> >>>>>> Rebased on top of current master.
> >>>>>
> >>>>> One of the commits reintroduced krbV dependency that I didn't notice.
> >>>>> Attaching updated revision. Only changes against previous revision are
> >>>>> in files daemons/dnssec/ipa-dnskeysync-replica and
> >>>>> daemons/dnssec/ipa-ods-exporter.
> >>>>
> >>>> This is much better, thanks!  I've got some comments inline.
> >>>>
> >>>>> +except gssapi.exceptions.GSSError:
> >>>>>       # If there was failure on using keytab, assume it is stale and
> >>>>> retrieve again
> >>>>>       retrieve_keytab(api, ccache_name, oneway_keytab_name,
> >>>>> oneway_principal)
> >>>>
> >>>> This code still bothers me a bit, but I think fixing it is probably
> >>>> beyond the scope of a python-gssapi port.
> >>>
> >>> The code catches all GSSAPI exceptions and retries to do the same thing
> >>> with different keytab. So if there was a problem unrelated to keytab,
> >>> the same exception will be raised again. Nothing will be ignored
> >>> silently.
> >>>
> >>>>
> >>>>> +    try:
> >>>>> +        creds = get_credentials(name=name, ccache_name=ccache_name)
> >>>>> +        # property access would raise exception if expired
> >>>>> +        if creds.lifetime > 0:
> >>>>> +            return creds
> >>>>> +    except gssapi.exceptions.ExpiredCredentialsError:
> >>>>> +        return None
> >>>>
> >>>> Per rfc2744, lifetime is unsigned.  It's not immediately clear what
> >>>> will
> >>>> happen when `creds.lifetime == 0`; perhaps an explicit `return Nune` in
> >>>> that case?
> >>>
> >>> I think the check is probably redundant, gssapi raises exception upon
> >>> inquiring expired credentials. In trust-fetch-domains I just access the
> >>> lifetime in try-except without using the value, so I could do the same
> >>> here. It would be nice if gssapi provided some 'is_valid' or
> >>> 'is_expired' method, so I wouldn't need to rely on side-effects of
> >>> property access, which is hard to read and confuses pylint.
> >>>
> >>>>
> >>>>>           # Setup LDAP connection
> >>>>>           try:
> >>>>> -            ctx = krbV.default_context()
> >>>>> -            ccache = ctx.default_ccache()
> >>>>> -            api.Backend.ldap2.connect(ccache)
> >>>>> +            api.Backend.ldap2.connect()
> >>>>>               cls.ldap = api.Backend.ldap2
> >>>>> -        except krbV.Krb5Error as e:
> >>>>> +        except gssapi.exceptions.GSSError:
> >>>>>               sys.exit("Must have Kerberos credentials to migrate
> >>>>> Winsync users.")
> >>>>
> >>>> Can you log the error here?  The other places GSSError is being caught
> >>>> are doing a great job of either filtering-and-raising or
> >>>> logging-and-exiting, so thanks for fixing those.
> >>>
> >>> Yes, I'll update it in next revision of the patch.
> >>>
> >>>>
> >>>>> +# Ugly hack for test purposes only. GSSAPI has no way to get default
> >>>>> ccache
> >>>>> +# name, but we don't need it outside test server
> >>>>> +def get_default_ccache_name():
> >>>>> +    try:
> >>>>> +        out = check_output(['klist'])
> >>>>> +    except CalledProcessError:
> >>>>> +        raise RuntimeError("Default ccache not found. Did you
> >>>>> kinit?")
> >>>>> +    match = re.match(r'^Ticket cache:\s*(\S+)', out)
> >>>>> +    if not match:
> >>>>> +        raise RuntimeError("Cannot obtain ccache name")
> >>>>> +    return match.group(1)
> >>>>
> >>>> Yup, this is still ugly.  Ah well, it's only test code.
> >>>>
> >>>
> >>> I was trying to modify the code to not need the variable and just use
> >>> the default, but it is used for manipulating it as a file - in
> >>> production it is always defined by mod_auth_gssapi. So I'd keep this
> >>> as is.
> >>>
> >>
> >>
> >> Next revision of the patch. Changes from previous rev: printing
> >> exception in ipa_winsync_migrate and more thorough dealing with
> >> credentials expiration in krb_utils and trust-fetch-domains.
> >
> > 1) There is a merge conflict in freeipa.spec.in, please rebase the patch
> > on top of current master.
> >
> 
> Done.
> 
> >
> > 2) pylint fails with:
> >
> > ************* Module ipa-ods-exporter
> > daemons/dnssec/ipa-ods-exporter:23: [E0611(no-name-in-module), ] No name
> > 'GSSError' in module 'gssapi')
> > ************* Module ipa-dnskeysync-replica
> > daemons/dnssec/ipa-dnskeysync-replica:15: [E0611(no-name-in-module), ]
> > No name 'GSSError' in module 'gssapi')
> >
> > Both failures are caused by "from gssapi import GSSError" statement,
> > which should read "from gssapi.exceptions import GSSError".
> >
> 
> Fixed.
> 
> >
> > 3) ipa-adtrust-install fails with:
> >
> > admin password:
> >
> > Unrecognized error during check of admin rights:
> > admin at abc.idm.lab.eng.brq.redhat.com: user not found
> >
> > Apparently there is a "user-show admin at abc.idm.lab.eng.brq.redhat.com"
> > call where a "user-show admin" call should be.
> >
> 
> Fixed. python-gssapi has a display_as method that could pull the name 
> from it, but it doesn't work in current version, therefore using 
> partition to split on '@'
> 
> >
> > 4) ipa-client-automount fails with:
> >
> > Failed to obtain host TGT: Major (851968): Unspecified GSS failure.
> > Minor code may provide more information, Minor (2529639111): Bad format
> > in credentials cache
> >
> 
> gssapi apparently refuses to accept empty file as ccache. In order to 
> not create a race condition / security hole by deleting and recreating 
> the temp file, I changed it to use per-process kernel keyring (I believe 
> non-persistent keyring should be available on all supported platforms).

Ypu can create a temp directory in tmp where only the user has access
to, then you can have a file within it. This way you can still use a
file w/o having to create it, w/o race condition, but with compatibility
to file ccaches in case the kernel/krb5 libs are not built with keyring
support.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York