Mark Phalan wrote: > On Fri, 2008-03-21 at 16:48 -0600, Shawn M. Emery wrote: > >> Mark Phalan wrote: >> >>> On Fri, 2008-03-21 at 00:15 -0600, Shawn M Emery wrote: >>> >>> >>>> Mark, >>>> >>>> Here is the webrev for kv2: >>>> >>>> http://borg.sfbay.sun.com/cube/builds/semery/kv2/webrev >>>> >>>> The CIFS team is currently testing it as well, so I may have updates >>>> based on any of their findings. I will give you the diffs of these if I >>>> do end up changing anything. >>>> >>>> >>> I'll take this to the cottage over the weekend. I won't have net access >>> before Tuesday so don't expect to hear from me before then :) >>> >>> >> The CIFS team will also be reviewing the changes, so don't go overboard. >> > > Here's what I've got: > > > kadm5_host_srv_names.c: 112 > Return value not checked - could end up passing garbage to > "profile_get_string" > kadm5_host_srv_names.c: 125 > Return value not checked. > kadm5_host_srv_names.c: 133 > Return value not checked. > Done (all of the above). BTW, uses identical code as get master, but that can be handled in a separate bug. > client_init.c: 312 > 'hp' should be initialized to NULL otherwise the code path at > line 352 may not be taken > Done. > exception_list_i386: 573 > Why krb5kdc? > Was previously out of order. > exception_list_sparc: 578 > Why krb5kdc? > See above. > klookup.c: 41 > Comment should be improved at least to mention DNS. > Done. > ksetpw.c: 79,93,99,108,131,153,159,169,177,194 > gettext() should be used for error message string. > krb5_get_error_message() should be used to print the string > reprsentation of the Kerberos error (where applicable). > com_err() could probably be used. > Done. > ksetpw.c: 122 > Return value of malloc() not checked. > Done. > ksetpw.c: 124 > Return value of realloc() not checked. > Done. > ksetpw.c: 169 > The error message could include "vprincstr" > Done. > ksetpw.c: 173 > Use "stdin" instead of '0'? > Done (fileno(stdin)). > ksetpw.c: 174 > Why 300? Instead of '300' a constant may read better > I believe that PASS_MAX should be sufficient. > ksetpw.c: 181 > Return value of strdup() not checked. > Done. > ksetpw.c: 183 > gettext() should be used > Done. > ksetpw.c: 187 > Return value of strdup() not checked. > Done. > ksetpw.c: 207: > Print error message for kt_remove_entries() failure. > Function already reports error messages. > ksetpw.c: 341-346 > gettext() should be used. > Done. > kdyndns.c: 41,42 > gettext() should be used. > Done. > kdyndns.c: 81 > Return value of smb_nic_init() not checked > Done. > kdyndns.c: 83 > Return value of smb_nic_fini() not checked. > None provided. > ksmb.c: 43-47,98,105 > gettext() should be used. > Done. > ksmb.c: 95 > Use "stdin" instead of '0'? > fileno(stdin) > ksmb.c: 96 > Why 300? Instead of '300' a constant may read better > PASS_MAX should be sufficient. > ksmb.c: 102,108 > Return value of strdup() not checked. > Done. > kclient.sh: > 46,47,57,193,821,1009,1010,1023,1044,1105,1114,1174,1240,1311,1312 > Message should to to stderr > This fix is doesn't change this behavior, a separate bug should be filed instead. > kclient.sh: 76,78 > TPAM initialized twice > Done. > kclient.sh: 78 > mktemp may fail > Please explain; how would a single instance in a newly created directory fail? > kclient.sh: 137, 145, 170 > kadmin is using $KRB5CCNAME but the "kinit" command never > populates the cred cache (kclient.sh:583,585) > $KRB5CCNAME is set back on 1416. > kclient.sh: 139,141,147,150,174 > How will this work in a non-C locale? > Done. > kclient.sh: 290 > Why no_addresses = true? > Typically clients that do not have keytab files is from the fact that they are configured with DHCP or behind a NAT, in those cases you would still want to allow tickets to be used. > kclient.sh: 308-327 > Looks wrong when English isn't being used > Done. > kclient.sh: 85 > Missing '$' before 'KLOOKUP' > 485: Done. > kclient.sh: 567-577 > What if the admin principal isn't using the name/admin convention? > I'm not changing this behavior w/this fix. The documentation supports the regular convention for this, anything that is not the default convention would not be supported IMO. > kclient.sh: 583, 585 > Output of kinit should be suppressed/captured > Provides prompting for admin. > Return code should be checked. > Output being checked. > kclient.sh: 588 > Looks wrong when English isn't being used > Done. > kclient.sh: 645 > gettext should be used for "System" > Done. > kclient.sh: 936 > what if the interface has an address and a netmask but isn't UP? > We are looking for local site names, we are not as concerned about transient states. > what about inet6? > Uh, well it's not available in 2003- and I wasn't aware of site boundaries defined in IPv6 by default in any version. > kclient.sh: 1257, 1303 > cryptoadm would seem to be a more natural interface than "encrypt" > when determining supported crypto mechanisms. > Uses the same interfaces, pkcs#11, for CF as cryptoadm does. > kclient.sh: 1333,1354 > Return code not checked. > Done. > kclient.sh: 1377, 1764 > Any reason '-rf' wasn't used? > Done. > kclient.sh: 1439-1451 > uid checks against 0 aren't kosher any more. The user may have > the relevent execution profiles/authorizations/privileges. The > error messages should inform the user what execution > Hmm, not relevant to these changes and this check is still valid as the user that belongs to the "Kerberos Client Management" protocol is given an effective uid of 0. > profiles/auths/privs he needs to run the script. > kclient.sh: 1700 > 'svcadm enable' should be run with '-s' and have its return code > checked. > Done.
New webrev can be found here: http://cr.opensolaris.org/~semery/6287615.1 Thanks, Shawn. --
