[PATCH] Add syslog messages for LDAP GSSAPI bind

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

We will now emit a level 0 debug message on keytab errors, and
also write to the syslog (LOG_AUTHPRIV)

Resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=591677
https://fedorahosted.org/sssd/ticket/485

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkws2+gACgkQeiVVYja6o6OuxgCgnoQuU4CjCUZzHSjfdTGQ4VAR
> K+EAn0F5KWl1ysbv5Zi/dA4TAje6S6cU
> =9/R6
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/01/2010 02:18 PM, Stephen Gallagher wrote:
> We will now emit a level 0 debug message on keytab errors, and
> also write to the syslog (LOG_AUTHPRIV)
>
> Resolves:
> https://bugzilla.redhat.com/show_bug.cgi?id=591677
> https://fedorahosted.org/sssd/ticket/485

General question to whoever reviews this: I'm not sure whether
LOG_AUTHPRIV or LOG_DAEMON is more appropriate here.

On Fedora, LOG_AUTHPRIV logs by default to /var/log/secure while
LOG_DAEMON logs by default to /var/log/messages.

I'm not sure which would be more expected for these errors.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkwt1JkACgkQeiVVYja6o6PKEgCgl9zyjH48H/BZmfgEiAi60Fkp
> BzoAniOMn6d2qMvnTncaFj03mpjmRG2+
> =KRTG
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/02/2010 07:59 AM, Stephen Gallagher wrote:
> On 07/01/2010 02:18 PM, Stephen Gallagher wrote:
> > We will now emit a level 0 debug message on keytab errors, and
> > also write to the syslog (LOG_AUTHPRIV)
> > Resolves:
> > https://bugzilla.redhat.com/show_bug.cgi?id=591677
> > https://fedorahosted.org/sssd/ticket/485
>
> General question to whoever reviews this: I'm not sure whether
> LOG_AUTHPRIV or LOG_DAEMON is more appropriate here.
>
> On Fedora, LOG_AUTHPRIV logs by default to /var/log/secure while
> LOG_DAEMON logs by default to /var/log/messages.
>
> I'm not sure which would be more expected for these errors.

On further reflection, I think LOG_DAEMON is the correct choice. Updated
patch attached.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkwt4MAACgkQeiVVYja6o6M5MACfQG3qE0VIWFDV3p1SpNPmvVNd
> W8EAnjXYsnEdafPC5Lkay+UQwvNqxpQ4
> =d3kF
> -----END PGP SIGNATURE-----

[Dmitri Pal]

Stephen Gallagher wrote:
> On 07/02/2010 07:59 AM, Stephen Gallagher wrote:
> > On 07/01/2010 02:18 PM, Stephen Gallagher wrote:
> > > We will now emit a level 0 debug message on keytab errors, and
> > > also write to the syslog (LOG_AUTHPRIV)
> > > Resolves:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=591677
> > > https://fedorahosted.org/sssd/ticket/485
> > General question to whoever reviews this: I'm not sure whether
> > LOG_AUTHPRIV or LOG_DAEMON is more appropriate here.
> > On Fedora, LOG_AUTHPRIV logs by default to /var/log/secure while
> > LOG_DAEMON logs by default to /var/log/messages.
> > I'm not sure which would be more expected for these errors.
>
> On further reflection, I think LOG_DAEMON is the correct choice. Updated
> patch attached.

Nack

You are leaking entry in success scenario.
I suggest a little bit cleaner approach:

while((ret = krb5_kt_next_entry(context, keytab, &entry, &cursor))
== 0){
        krb5_unparse_name(context, entry.principal, &principal);
        krb5_free_keytab_entry_contents(context, &entry);
        if (strcmp(full_princ, principal) == 0) found = true;
        free(principal);
        if (found) break;
    }

And I think that if you have explicit syslog messages in case of
failures it makes sense to have one also in the case krb5_kt_end_seq_get
fails.

-------------------------

_______________________________________________
sssd-devel mailing list
[e-mail]
https://fedorahosted.org/mailman/listinfo/sssd-devel

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/02/2010 09:37 AM, Dmitri Pal wrote:
> Nack
>
> You are leaking entry in success scenario.
> I suggest a little bit cleaner approach:
> while((ret = krb5_kt_next_entry(context, keytab, &entry, &cursor))
>
> == 0){
>         krb5_unparse_name(context, entry.principal, &principal);
>         krb5_free_keytab_entry_contents(context, &entry);
>         if (strcmp(full_princ, principal) == 0) found = true;
>         free(principal);
>         if (found) break;
>     }

Good catch. Fixed.
> And I think that if you have explicit syslog messages in case of
> failures it makes sense to have one also in the case krb5_kt_end_seq_get
> fails.

Sure. It should hopefully never happen, but added nonetheless.

Thanks for the review! New patch attached.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkwt8R8ACgkQeiVVYja6o6OlhwCcCjAqAc7n1GvlQvT6x/icDm4q
> 3LEAniCZdLpA2GbryIDqXtiHiNJvcKMa
> =4ag2
> -----END PGP SIGNATURE-----

[Dmitri Pal]

Stephen Gallagher wrote:
> On 07/02/2010 09:37 AM, Dmitri Pal wrote:
> > Nack
> > You are leaking entry in success scenario.
> > I suggest a little bit cleaner approach:
> > while((ret = krb5_kt_next_entry(context, keytab, &entry, &cursor))
> >
> > == 0){
> >         krb5_unparse_name(context, entry.principal, &principal);
> >         krb5_free_keytab_entry_contents(context, &entry);
> >         if (strcmp(full_princ, principal) == 0) found = true;
> >         free(principal);
> >         if (found) break;
> >     }
>
> Good catch. Fixed.
> > And I think that if you have explicit syslog messages in case of
> > failures it makes sense to have one also in the case krb5_kt_end_seq_get
> > fails.
>
> Sure. It should hopefully never happen, but added nonetheless.
>
> Thanks for the review! New patch attached.

Visual ack.

-------------------------

_______________________________________________
sssd-devel mailing list
[e-mail]
https://fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

On Fri, Jul 02, 2010 at 10:29:47AM -0400, Dmitri Pal wrote:
> Stephen Gallagher wrote:
> > On 07/02/2010 09:37 AM, Dmitri Pal wrote:
> > > Nack
> > > You are leaking entry in success scenario.
> > > I suggest a little bit cleaner approach:
> > > while((ret = krb5_kt_next_entry(context, keytab, &entry, &cursor))
> > >
> > > == 0){
> > >         krb5_unparse_name(context, entry.principal, &principal);
> > >         krb5_free_keytab_entry_contents(context, &entry);
> > >         if (strcmp(full_princ, principal) == 0) found = true;
> > >         free(principal);
> > >         if (found) break;
> > >     }
> >
> > Good catch. Fixed.
> > > And I think that if you have explicit syslog messages in case of
> > > failures it makes sense to have one also in the case krb5_kt_end_seq_get
> > > fails.
> >
> > Sure. It should hopefully never happen, but added nonetheless.
> >
> > Thanks for the review! New patch attached.
>
> Visual ack.

I would recommend to move the 'Verify the keytab' section into a
separate function and call it during the initialisation of the provider.

For the syslog messages I think it would make sense to create a new
utility function which does openlog(), syslog() and closelog(). This
would make it easier to send syslog messages for other parts of the code
and would help to add 'other logging APIs'.

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel
>
> -- 
> Thank you,
> Dmitri Pal
>
> Engineering Manager IPA project,
> Red Hat Inc.
>
> Looking to carve out IT costs?
> www.redhat.com/carveoutcosts/
>
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel