Giuliano Santorelli wrote: > Hi, > > I need code review for bug 6254183: > http://bugs.opensolaris.org/view_bug.do?bug_id=6254183 > Synopsis: encrypt(1) should support blowfish > > > The code is posted here: http://cr.opensolaris.org/~mao/bug-6254183
There are several really big problems with this I'm afraid. You webrev is very strange it should have full paths from usr/src downwards not just the leaf file names, this means some of the webrev views don't work (in particular frames which is the version I prefer to use). DJM-0a you can't just add a #define for CKM_BLOWFISH_CBC_PAD to pkcs11t.h, those defines are controlled by the RSA PKCS#11 working group. We also have a "policy" in the crypto group that we NEVER ever modify that file, we only ever bring in complete new versions of it from the RSA Cryptoki working group site. This is fundamental to this been acceptable and until the RSA PKCS#11 working group assigns a mech number for CKM_BLOWFISH_CBC_PAD we can't do it that way. What can be done though is we can add a vendor defined mechanism (those with mech numbers above 0x80000000) but that must get added to cryptoki.h not any of the pkcs11?.h files. DJM-0b The padding code should not be added to softtoken code, instead CBC_PAD should be added as a new mode in $SRC/common/crypto/modes/. Any existing algorithms that do CBC_PAD should be updated to use the new algorithm independent mode code. By adding it as a new mode it makes it available to both the userland and kernel land frameworks. I'm sorry to say but the whole approach here is not acceptable given the current way the crypto framework is implemented. If you have been misguided by our team in the past I'm sorry but unfortunately other changes have been made that makes your approach to fixing this bug highly undesirable. Given the above the rest of my comments are really not that relevant because DJM-0a and DJM-0b are fundamental issues that will change the way the rest of this is done anyway. I've listed the rest of the comments below in the hope that you may find them a useful codereview experience. DJM-1 softSlotToken.c has a mis merge you have readadded a removed comment. DJM-2 You need to run 'hg nits' and fix all the things it complains about, like #pragma idents etc that are no longer allowed. DJM-3 decrypt.c you have lots of changes to the carefully written comment that I very strongly disagree with. In particular why have you choosen to rewrite the initial block comment to remove all the options and remove all the information about tokens. Maybe this is just a missmerge. DJM-4 decrypt.c:197 We never comment out code like this, if it is no longer needed it should be removed. DJM-5 decrypt.c:307 what is this for ? DJM-6 decrypt.c:400 This makes no sense. DJM-7 decrypt.c:441/443 Don't change these lines they are purposely uint32_t because they are written to files and need to be the same in a 32 bit and 64 bit compilation environment since the command is delivered as both 32 and 64 bit binaries. Is this maybe yet another mis merge ? DJM-8 decrypt.c:512/529. What is the (1) and (2) about ? DJM-9 decrypt.c:903/904 same comment as DJM-7 DJM-10 decrypt.c:1055 English comments only please. DJM-11 decrypt.c: crypt_multipart what are all the extra variables for they don't appear (from the diffs) to be used. -- Darren J Moffat _______________________________________________ opensolaris-code mailing list opensolaris-code@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
