[Pki-devel] [PATCH] 0001-KRA-changes-for-archiving-and-recovering-symmetric-keys.patch

[Endi Sukma Dewata]

On 2/8/2012 1:36 PM, John Magne wrote:
> Provides the ability to archive and recover symmetric keys and pass phrases under the restful interface.
> Java client included to test functionality.

These are just some minor issues/nitpicking, but I hope it will lead to 
cleaner code in the long run. Some of these might have been discussed 
already.

1. The throw statement in KeyDAO.java:124 for null params can be moved 
right after the params assignment in line 111. In general I think we 
should check null right after assignment and before doing anything else.

2. The keyData null checking In KeyDAO.java:160,168 is unnecessary 
because in all possible code execution the keyData always gets a new 
instance (line 128 and 159). The 'new' operation never returns a null, 
if it fails for any reason it will throw an exception so the check will 
never be executed.

3. The null initialization for params in KeyRequestDAO.java:230 is 
unnecessary because the code guarantees that params will be initialized 
with a value (line 231) before it's read for the first time (line 233). 
In general if we forgot to initialize a variable the code will fail to 
build, so we don't need to worry about removing unnecessary null 
assignments.

4. Formatting issue, some expressions/params between parentheses have 
unbalanced spaces:

   if ( expression) {
   method( params);

I'm not sure if we have a coding standard for this, but at least it 
should be consistent within a single statement, either use spaces in 
both positions or not at all:

   if ( expression ) {
   if (expression) {

5. Similar to #3, the null initialization for IV in DRMTest.java:133 is 
unnecessary because the following try-catch clause guarantees that it 
will be initialized regardless of exceptions. Similar reason for 
IV_server in line 134.

6. According to our coding standard the variable names should begin with 
lower case. So in #5 it should be iv and iv_server.

7. The toString() in DRMTest.java:139 (and some other places) is 
unnecessary. In general any string concatenation will automatically call 
the toString() of the object.

8. The extra parentheses in DRMTest.java:562 is unnecessary because the 
compiler can distinguish the constructor from the method. The following 
code will work just fine:

   new PKIArchiveOptions.Template().decode(inStream);

9. In ArchiveOptions class (and some other new classes) the member 
variables are prefixed with 'm':

     private String mSymmAlgOID = null;
     private byte mSymmAlgParams[] = null;
     private byte mEncSymmKey[] = null;
     private byte mEncValue[] = null;

I think we discussed previously that the prefix is unnecessary, so no 
need to add it in new code. The null initialization is also unnecessary 
because all member variables are assigned to null by default during 
instantiation.

10. Array constants like in EncryptionUnit.java:652 can be defined like 
this for simplicity:

     SymmetricKey.Usage usages[] = {
         SymmetricKey.Usage.WRAP,
         SymmetricKey.Usage.UNWRAP
     };

11. In SecurityDataRecoveryService.java the assignment in line 168 can 
be moved into the catch clause above it because iv will only be null if 
there's an exception. The nextBytes() will not make iv null because 
method parameters are passed by value. The clone() is not necessary 
either, iv can just point to the iv_default. The rnd can also be moved 
inside the try block to minimize variable scope.

     iv = new byte[8];

     try {
         Random rnd = new Random();
         rnd.nextBytes(iv);
     } catch (Exception e) {
         iv = iv_default;
     }

-- 
Endi S. Dewata