Mark Phalan wrote: > On Thu, 2008-04-03 at 10:34 -0600, Shawn M Emery wrote: > ... > >> > 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). >> > > Looks generally good. > kadm5_host_srv_names.c:130,142 - memleak of "def_realm", "kpasswd"?. > kpasswd: no > More generally looks like "def_realm" is leaked in this function. > def_realm: done. >> BTW, uses identical code as get master, but >> that can be handled in a separate bug. >> > > Have you filed it or should I? > Please, go ahead. >> Done. >> > exception_list_i386: 573 >> > Why krb5kdc? >> > >> Was previously out of order. >> > exception_list_sparc: 578 >> > Why krb5kdc? >> > >> See above. >> > > Yup, I somehow missed that. > > ... > >> > 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. >> > > Want me to file it? > Please, go ahead. >> > 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? >> > > Wrong permissions? Created by the same process that created the directory. > File system full? 0 byte file. > The man page indicates that it has > failure modes. > Done. >> > 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. >> > Ok. For consistency I'd prefer to see kinit using '-c' as well but I > don't really mind. > Ok. >> > 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. >> > > Makes sense. > > ... > >> > 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. >> > > Ok. > > >> > kclient.sh: 583, 585 >> > Output of kinit should be suppressed/captured >> > >> Provides prompting for admin. >> > > Ok. > > >> > Return code should be checked. >> > >> Output being checked. >> > > Ok. > > General note: > I'm really wondering though if this will work properly in a non English > locale. The translations will have to be very consistent. > I'm not a i18n (procedure) expert, do you know some one that could verify this? >> > 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. >> > > Ok. > > >> > 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. >> > > Ok. It stuck out at me as it was ipv4 specific... > > >> > 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. >> > > Understood, however I'd still prefer to see cryptoadm but I'll leave it > up to you. > > ... > >> > 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. >> > > Ok, I guess I can open a low priority bug on this. > Yes, low priority, given the fact that the regex will match either uid or euid (which the associated profile specifies). >> > 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. >> > > Everything else looks fine. > Thanks for the review. I also simplified usr/src/lib/krb5/kadm5/clnt/client_init.c, see:
http://cr.opensolaris.org/~semery/6287615.2 Shawn. --
