[Freeipa-devel] python-kerberos patch

[Simo Sorce]

On Tue, 2014-01-14 at 13:12 -0500, Rob Crittenden wrote:
> Simo Sorce wrote:
> > On Mon, 2014-01-13 at 14:45 -0500, Rob Crittenden wrote:
> >> In an effort to be able to to use GSS-Proxy as a client in IPA I've
> >> written a patch against python-kerberos to add a call to
> >> gss_cred_inquire so we can peek at the current principal name. This will
> >> replace the python-krbV call in ipapython/util.py::get_current_principal().
> >>
> >> The patch is pending review upstream at
> >> https://trac.calendarserver.org/ticket/830#comment:1 but it hasn't seen
> >> any activity yet and I'm impatient.
> >>
> >> Anyone have a few moments to take a look? I'm not super happy with the
> >> way one has to call it and some feedback would be helpful.
> >
> > I do not like their python API, but ... we can;t change that.
> >
> > One the C code:
> > you call it yadda_client_yadda but then you user server_creds as the
> > variable name, that's confusing.
> >
> > You should simply initialize name and name_token to 0 and
> > unconditionally free/release them on error, and do that all at the end
> > based on the return you want.
> >
> > Also you can simplify string copying..
> >
> > How I'd rewrite the last 15 lines after gss_display_name()
> >
> > if (GSS_ERROR(maj_stat)) {
> >      set_gss_error(maj_stat, min_stat);
> >      ret = AUTH_GSS_ERROR;
> >      goto end;
> > }
> >
> > state->username = strndup(name_token.value, name_token.length);
> > if (!state->username) {
> >      set_gss_error(GSS_S_FAILURE, ENOMEM);
> >      ret = AUTH_GSS_ERROR;
> > }
> >
> > end:
> >      (void)gss_release_cred(&min_stat, &server_creds);
> >      (void)gss_release_name(&min_stat, &name);
> >      (void)gss_release_buffer(&min_state, name_token);
> >
> >      return ret;
> > }
> >
> >
> > You could simplify even more without using ret, and jumping to end on
> > any maj_stat error and at end
> > if (maj_stat != GSS_S_COMPLETE) {
> >      set_gss_error(maj_state, min_stat);
> >      ...
> >
> > Simo.
> >
> 
> Thanks for the detailed review. I applied your changes and updated the 
> attachment on the upstream ticket.

Looks good.

> If you think it is acceptable, I'd like to add this patch to at least 
> rawhide if not F-20.

Maybe rawhide, but I'd like to know that upstream accepted the python
API for it before committing, the last thing we want is to have
incompatible python API should upstream decide they do not like the one
you proposed.

Simo.

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