http://bugzilla.mindrot.org/show_bug.cgi?id=125 ------- Additional Comments From djm@mindrot.org 2004-04-26 11:12 -------
(From update of attachment 618)
This is on the list to be fixed before 3.9p1.
>- if (!allowed_user(pw))
>- return (NULL);
>+ if (pw != NULL && !allowed_user(pw))
>+ pw = NULL;
These shouldn't be necessary - we take steps to ensure that pw is never NULL,
so these just obscure the real changes.
>+ if (pw != NULL) {
>+ pw = pwcopy(pw);
>+#if defined(HAVE_BSM_AUDIT_H) && defined(HAVE_LIBBSM)
>+ solaris_audit_save_pw(pw);
>+#endif /* BSM */
>+ }
>+ return (pw);
Why do you return pw here? We fake one later for invalid users anyway.
>+#if defined(HAVE_BSM_AUDIT_H) && defined(HAVE_LIBBSM)
Rather than this slightly verbose test, perhaps you should add:
#if defined(HAVE_BSM_AUDIT_H) && defined(HAVE_LIBBSM)
# define USE_BSD 1
#endif
to defines.h and just do "#ifdef USE_BSM" everywhere.
>+ if (!authenticated) {
>+ PRIVSEP(solaris_audit_bad_pw("public key"));
>+ }
>+#endif /* BSM */
>--- openbsd-compat/Makefile.in~ 2004-01-21 01:07:23.000000000 -0500
>+++ openbsd-compat/Makefile.in 2004-03-03 17:37:39.243034000 -0500
Please avoid reformatting the dependancy lists - the changes obscure any real
additions that you make be making. BTW we used to keep the dependancy lists in
a prettier format, but it was too much work to maintain :)
>Index: openbsd-compat/bsd-solaris.c
>--- openbsd-compat/bsd-solaris.c~ 2004-03-03 17:37:39.253019000 -0500
>+++ openbsd-compat/bsd-solaris.c 2004-03-03 17:38:15.103435000 -0500
>@@ -0,0 +1,447 @@
>+/*
>+ * Copyright 1988-2002 Sun Microsystems, Inc. All rights reserved.
>+ * Use is subject to license terms.
What is the lineage of this code? We need to be very careful about importing
code from vendors.
>+ solaris_audit_record(1, gettext("logins disabled by /etc/nologin"),
>+ AUE_openssh);
I'm not sure whether we will add a dependancy on gettext right now, given that
we don't use it anywhere else.
>+void
>+solaris_audit_logout(void)
>+{
>+ char textbuf[BSM_TEXTBUFSZ];
>+
>+ (void) snprintf(textbuf, sizeof (textbuf),
>+ gettext("sshd logout %s"), sav_name);
>+
>+ solaris_audit_record(0, textbuf, AUE_logout);
>+}
A lot of this code is pretty repetitive. Perhaps it could be factored out into
a common varargs function. E.g.
void
solaris_write_audit(int what, const char *fmt, ...)
{
va_list args;
char textbuf[BSM_TEXTBUFSZ];
va_start(args, fmt);
vsnprintf(textbuf, sizeof(textbuf), fmt, args);
va_end(args);
solaris_audit_record(0, textbuf, what);
}
Also, in future could you please attach patches directly into bugzilla? It
makes them more easy to review and discuss.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.