Re: svn commit: r244604 - head/usr.sbin/gssd

Rick Macklem wrote:
> Garrett Cooper wrote:
> > On Sun, Dec 30, 2012 at 4:49 PM, Rick Macklem <rmacklem_at_uoguelph.ca>
> > wrote:
> > > bf1783 wrote:
> > >> >Author: rmacklem
> > >> >Date: Sat Dec 22 23:21:17 2012
> > >> >New Revision: 244604
> > >> >URL: http://svnweb.freebsd.org/changeset/base/244604
> > >> >
> > >> >Log:
> > >> >  It was reported via email that some sshds create kerberos
> > >> >  credential cache files with names other than
> > >> >  /tmp/krb5cc_<uid>.
> > >> >  The gssd daemon does not know how to find these credential
> > >> >  caches.
> > >> >  This patch implements a new option "-s" that does a search for
> > >> >  credential cache files, using roughly the same algorithm as
> > >> >  the
> > >> >  gssd daemon for Linux uses. The gssd behaviour is only changed
> > >> >  if the new "-s" option is specified. It also implements two
> > >> >  other
> > >> >  new options related to the "-s" option.
> > >> >
> > >> >  Reported by: Piete.Brooks at cl.cam.ac.uk, Herbert Poeckl
> > >> >  Tested by: Herbert Poeckl (admin at ist.tugraz.at), Illias A.
> > >> >  Marinos
> > >> >  MFC after: 2 weeks
> > >>
> > >> ...
> > >>
> > >> >+#include <krb5.h>
> > >>
> > >> Rick:
> > >>
> > >> This breaks world built WITHOUT_KERBEROS and WITH_GSSAPI.
> > >>
> > >> Regards,
> > >> b.
> > > Could you please test the attached patch.
> > >
> > > Also, if someone who is familiar with the build/Makefile side
> > > of things could review this, it would be appreciated.
> >
> > 1. I would name WITHOUT_KERBEROS to KERBEROS_SUPPORT in the
> > sourcefile
> > and CFLAGS to avoid potential confusion/noise with build logic.
> >
> WITHOUT_KERBEROS is used other places, like telnetd. Were you aware of
> that?
> (I just thought it would keep it consistent, but if you think it is
> better
> to use a different name, I don't care.)
> 
Oh, I see you were suggesting that the polarity be reversed. Well,
although the #ifndef is a bit ugly, the utility is useless without
Kerberos, so I think I'd rather stick with "enabled by default".

Also, there is KPROGS in head/kerberos5/Makefile, which is a list
of programs that depend on kerberos. gssd isn't in the list, but
maybe it should be? (And that list is used to "dekerberise" them
by setting -DWITHOUT_KERBEROS.)

So, unless others feel strongly about it, I think I'd rather stick
with using WITHOUT_KEREBEROS.

rick

> > 2. This code should be revised per style(9):
> >
> > +#else
> > + fprintf(stderr, "This option not available when built"
> > + " without MK_KERBEROS\n");
> > + exit(1);
> >
> > In particular:
> >
> > errx(1, "This option requires Kerberos support");
> >
> > Seems more succinct and addresses the actual item at hand.
> >
> Yea, I'll switch it to errx(). I just cribbed the code further
> down, that used fprintf().
> 
> > 3. This could be simplified as well potentially:
> >
> > +.if ${MK_KERBEROS} != "no"
> > DPADD= ${LIBGSSAPI} ${LIBKRB5} ${LIBHX509} ${LIBASN1} ${LIBROKEN}
> > ${LIBCOM_ERR} ${LIBCRYPT} ${LIBCRYPTO}
> > LDADD= -lgssapi -lkrb5 -lhx509 -lasn1 -lroken -lcom_err -lcrypt
> > -lcrypto
> > +.else
> > +CFLAGS+= -DWITHOUT_KERBEROS
> > +DPADD= ${LIBGSSAPI}
> > +LDADD= -lgssapi
> > +.endif
> >
> > to this:
> >
> > DPADD= ${LIBGSSAPI}
> > LDADD= -lgssapi
> > .if ${MK_KERBEROS} != "no"
> > CFLAGS+= -DKERBEROS_SUPPORT
> > DPADD+= ${LIBKRB5} ${LIBHX509} ${LIBASN1} ${LIBROKEN} ${LIBCOM_ERR}
> > ${LIBCRYPT} ${LIBCRYPTO}
> > LDADD+= -lkrb5 -lhx509 -lasn1 -lroken -lcom_err -lcrypt -lcrypto
> > .endif
> >
> Yea, I can do this change too. I think the latter is more readable.
> 
> Thanks, rick
> 
> > Thanks!
> > -Garrett
> > _______________________________________________
> > freebsd-current_at_freebsd.org mailing list
> > http://lists.freebsd.org/mailman/listinfo/freebsd-current
> > To unsubscribe, send any mail to
> > "freebsd-current-unsubscribe_at_freebsd.org"
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to
> "freebsd-current-unsubscribe_at_freebsd.org"