On Fri, 2015-08-21 at 15:52 +0200, Michael Šimáček wrote:
> On 2015-08-20 20:42, Robbie Harwood wrote:
> > Michael Šimáček <msima...@redhat.com> writes:
> >> On 2015-08-20 12:32, Michael Šimáček wrote:
> >>>>>> Michael Šimáček <msima...@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.
Inquiring the credentials *is* the method to check if they are valid or
expired, what don't you like about it? The fact it raises when they are
> >> # 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.
Ideally we use export_cred/store_cred and not manipulate files directly,
but with memcache in the picture we have to compromise anyway, oh
Simo Sorce * Red Hat, Inc * New York
Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code