On Mon, 2008-03-03 at 17:59 -0800, Glenn Barry wrote: ... > looks good in general... > > http://cr.opensolaris.org/~mbp/daemon_error_messages/usr/src/cmd/krb5/kadmin/cli/keytab.c.wdiff.html > > 357 + et = > permitted_etypes[0].ks_enctype; > 358 + fprintf(stderr, gettext("%s: > Encryption types " > 359 + "requested: %d"), whoami, > et); > > * you're getting the requested etype from the permitted_etypes var, > that correct?
Correct. > > * and I guess this will change soon to print etype names instead of > nums, so I should re-review, correct? Yes. The output now looks like: zup# ./kadmin -p mark/admin -q "ktadd -k /tmp/t t" Authenticating as principal mark/admin with password. Password for mark/admin at ACME.COM: kadmin: Bad encryption type while changing t's key kadmin: Encryption types requested: aes256-cts-hmac-sha1-96 (18), aes128-cts-hmac-sha1-96 (17), des3-cbc-sha1 (16), arcfour-hmac (23), des-cbc-md5 (3), des-cbc-crc (1) zup# The following files were changed for this update: usr/src/cmd/krb5/kadmin/cli/keytab.c usr/src/lib/gss_mechs/mech_krb5/crypto/enctype_to_string.c usr/src/lib/gss_mechs/mech_krb5/mapfile-vers usr/src/uts/common/gssapi/mechs/krb5/include/krb5.h > > http://cr.opensolaris.org/~mbp/daemon_error_messages/usr/src/cmd/krb5/kadmin/server/ovsec_kadmd.c.wdiff.html > > 450 + /* Solaris Kerberos: Indicates whether loalhost is master > or not */ > > * sp: localhost > > 992 + * Solaris Kerberos: > 993 + * List the logs (FILE, STDERR, etc) which are > currently being > 994 + * logged to and print to stderr. Useful when > trying to > 995 + * track down a failure via SMF. > 996 + */ > 997 + if (ret = krb5_klog_list_logs(whoami)) { > > * whats an example output of this? When krb5.conf contains the following: [logging] admin_server = FILE:/tmp/logging admin_server = CONSOLE soe-280r-4# /usr/lib/krb5/kadmind kadmind: logging to FILE=/tmp/logging, CONSOLE > > * should it be sooner (as soon as possible before an err might get > triggered)? The behaviour is to log stderr and the log locations as indicated in krb5.conf until the daemon daemonizes. If an error occurs before the logs locations are listed it will be shown on stderr immediately. The listing of log files is intended to be useful when tracking down errors that happen AFTER daemonization. > > http://cr.opensolaris.org/~mbp/daemon_error_messages/usr/src/cmd/krb5/krb5kdc/main.c.wdiff.html > > 376 + /* > 377 + Solaris Kerberos: > 378 + Set the current context back to the general context > > * comment style nit: "*" missing > Fixed. > http://cr.opensolaris.org/~mbp/daemon_error_messages/usr/src/cmd/krb5/slave/kpropd.c.wdiff.html > > 284 + if (buf[0] != '#' && !isspace(buf[0])) > 285 + seen_file = 1; > > * is checking buf[0] sufficient here (or just good enough:)? > > 378 + fprintf(stderr, gettext("%s: Logging to SYSLOG > \n"), progname); > > 750 + fprintf(stderr, gettext("%s: Logging to SYSLOG > \n"), progname); > 2/27 > > * maybe list syslog level.fac for more good info? Fixed. > > + krb5_kt_close(kpropd_context,kt); > > * ", kt" > > 1141 + progname = (strrchr(argv[0], '/') ? > 1142 + strrchr(argv[0], '/') + 1 : argv[0]); > > * basename(3c) > Updated to use basename. > http://cr.opensolaris.org/~mbp/daemon_error_messages/usr/src/lib/gss_mechs/mech_krb5/et/kdb5_err.c.wdiff.html > > * where are these new errs from? MIT 1.6.3. They really should have been brought in with the LDAP work. > > http://cr.opensolaris.org/~mbp/daemon_error_messages/usr/src/lib/krb5/kadm5/alt_prof.c.wdiff.html > > 962 params.iprop_polltime = > strdup(params_in->iprop_polltime); > 962 963 if (params.iprop_polltime) > 963 964 params.mask |= > KADM5_CONFIG_POLL_TIME; > 964 965 } else { > 965 966 if (aprofile && ! > krb5_aprof_get_string(aprofile, hierarchy, > 966 967 TRUE, &svalue)) { > 968 + free(params.iprop_polltime); > 967 969 params.iprop_polltime = > strdup(svalue); > 968 970 params.mask |= > KADM5_CONFIG_POLL_TIME; > 969 971 krb5_xfree(svalue); > > * looks like strdup ret val not checked correctly (or at all) Fixed. > > http://cr.opensolaris.org/~mbp/daemon_error_messages/usr/src/lib/krb5/kadm5/kadm_host_srv_names.c.wdiff.html > > 157 + * Try to determine if this is the master KDC for a given realm > 158 + */ > 159 +kadm5_ret_t kadm5_is_master(krb5_context context, const char > *realm, > > * this gonna work for multi-homed sys? Do you mean hosts with multiple IPs/interfaces? If so then yes. It works in the following way: 1. Finds the admin server for the given realm. 2. Looks up the address of the admin server 3. Find all the local addresses. 4. Comparses the address of the admin server to the list of local addresses. If one matches then return true. This should also work if running multiple kadminds for different realms on the same host (is this supported?). > > http://cr.opensolaris.org/~mbp/daemon_error_messages/usr/src/lib/krb5/kadm5/srv/logger.c.wdiff.html > > 1113 +void krb5_klog_set_context(krb5_context context) { > 1121 +static const char * facility2string(int facility) { > > * cstyle - is not the style for the rest of this file from mit like? Would you like me to change the style from cstyle to MIT style? I can do that but even the MIT "style" is very inconsistant so I'm not sure it buys us much. > > ret type > funcname(....) > > 1220 + memcpy(&log_control.log_entries[0], > &def_log_entry, > > * memcpy returns (void *), so cast needed of (void) Fixed. Is this for lint or cstyle? I didn't get any lint warnings when I built. > > 1231 + } else { > 1232 + return (errno); > > * need to free log_entries? Yup, fixed. > > 262 + log_control.log_entries = > 1263 + > realloc(log_control.log_entries, > 1264 + (log_control.log_nentries + 1) > * > 1265 + sizeof(struct log_entry)); > 1266 + if (log_control.log_entries != > NULL) > 1267 + log_control.log_nentries--; > > * looks like realloc fail is not checked here correctly Fixed. > > http://cr.opensolaris.org/~mbp/daemon_error_messages/usr/src/lib/krb5/kadm5/srv/server_init.c.wdiff.html > > 184 + if (emsg) > 185 + *emsg = NULL; > > * possible mem leak? No, this is simply NULL'ing a param which is used to return an alloc'ed string. > > > 290 + *emsg = strdup(m); > 291 + krb5_free_error_message(handle->context, m); > > * strdup ret chk? No need. Its ok for emsg to be NULL. > > 404 +kadm5_ret_t kadm5_init(char *client_name, char *pass, > 405 + char *service_name, > 406 + kadm5_config_params *params_in, > 407 + krb5_ui_4 struct_version, > 408 + krb5_ui_4 api_version, > 409 + char **db_args, > 410 + void **server_handle) { > 411 + return (_kadm5_init(client_name, pass, > service_name, params_in, > > * maybe better naming here: kadm5_init2 or kadm5_init_extended > > also, make it clear its for Sol Kerb Changed to kadm5_init2 and added a /* Solaris Kerberos */ comment. > > http://cr.opensolaris.org/~mbp/daemon_error_messages/usr/src/lib/krb5/plugins/kdb/db2/kdb_db2.c.wdiff.html > > 353 + snprintf(errbuf, sizeof(errbuf), gettext("Failed to > open \"%s\": "), filename); > 372 + snprintf(errbuf, sizeof(errbuf), > (more of these in this file) > > > * snprintf returns int so cast it (void) Fixed. Thanks for the review! -Mark
