Mark Phalan wrote: > I've just uploaded a webrev of my resync/pkinit workspace. There still > needs to be some work on pkinit so don't expect the code in > usr/src/lib/krb5/plugins/preauth/pkinit/ to be complete (you can ignore > it for now). I'll post another incremental webrev with any changes I > make to the pkinit code later on. The rest of the changes are resync > changes for MIT 1.6.3. The hg comment needs to be updated, I'll do that > once we get the pkinit PSARC case submitted. > > I've chunked the review up into four pieces as I expect the krb team to > do the review. > > Shawn: Chunk 1 > Peter: Chunk 2 > Glenn: Chunk 3 > Will: Chunk 4 > > I'd like to have this completed by 17th Sept. Let me know if thats a > problem for anyone. > > webrev here: > http://cr.opensolaris.org/~mbp/pkinit/ > > Cheers, > > -Mark > > > > > > > _______________________________________________ > kerberos-discuss mailing list > kerberos-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/kerberos-discuss >
Couple days late, but finally got thru my chunk. Here are my comments: usr/src/lib/gss_mechs/mech_krb5/krb5/krb/auth_con.c Still has Sun Copyright. I assume because of the few /* Solaris Kerberos */ comments. If so, then filea: usr/src/lib/gss_mechs/mech_krb5/krb5/keytab/read_servi.c usr/src/lib/gss_mechs/mech_krb5/krb5/krb/chpw.c should not have had the copyright removed. ------------- Conversely, why is the copyright kept in the following files: usr/src/lib/gss_mechs/mech_krb5/krb5/krb/conv_princ.c --------------------- usr/src/lib/gss_mechs/mech_krb5/krb5/krb/chk_trans.c What is that on line 360 of your modified code? If the user passes a -x and/or a -v, that would populate argv[1] and argv[2] respectfully, right? If so, then I am not sure the subsequent references to argv[1] and argv[2] - like lines 395 thru 398. ------------------ usr/src/lib/gss_mechs/mech_krb5/krb5/krb/chpw.c If we return at line 115, where does the free that is on line 119 take place? Memory leak maybe? Actually, it looks like a lot of the returns in this routine do not call krb5_free_error(context,krberror) and also krb5_xfree(clearresult.data). -------------- usr/src/lib/gss_mechs/mech_krb5/krb5/krb/copy_addrs.c Should there be a /* Solaris kerberos */ comment above line 45 and possibly a Sun Copyright? ------------ usr/src/lib/gss_mechs/mech_krb5/krb5/krb/copy_data.c ditto on above comment for lines 55 and 75 and copyright? --------------- usr/src/lib/gss_mechs/mech_krb5/krb5/krb/gic_pwd.c 357 - I do not see where result string is set to any value before you could attempt to free it at lines 368 and/or 377 Looks like kadm5_chpass_principal_util is supposed to be call to krb5_change_password. --------------- usr/src/lib/gss_mechs/mech_krb5/krb5/krb/krb5_libinit.c Did you mean to change the modes on this file? mode change: 755 to 644 -------------- usr/src/lib/gss_mechs/mech_krb5/krb5/krb/preauth.c I did not review this file as I assume it is a direct copy of MIT stuff....did you want me to review this logic? ---------- usr/src/lib/gss_mechs/mech_krb5/krb5/os/changepw.c It appears that they just moved code around a bunch, and added some new things. I did not see any issues with the logic, but would think this this code path would be hit a lot and would be quite noticable if something is not right. Testing this did not run into issues, right? ------------ usr/src/lib/gss_mechs/mech_krb5/krb5/os/locate_kdc.c is line 698 suppose to be "case kdc_ports:"? -------------- usr/src/lib/gss_mechs/mech_krb5/krb5/os/sendto_kdc.c 68 #undef DEBUG 69 70 #ifdef DEBUG How can 70 ever be true if you #undef it at line 68?
