<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Ok, looks good.<br>
    Thanks,<br>
    Valerie<br>
    <br>
    On 8/25/2015 1:10 PM, Rajan Halade wrote:
    <blockquote cite="mid:55DCCBA9.2090105@oracle.com" type="cite">
      <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
      Hi Valerie,<br>
      <br>
      Thanks for comments! I have fixed all of them.<br>
      <br>
      <a moz-do-not-send="true" class="moz-txt-link-freetext"
        href="http://cr.openjdk.java.net/%7Erhalade/8048601/webrev.01/">http://cr.openjdk.java.net/~rhalade/8048601/webrev.01/</a><br>
      <br>
      - Rajan<br>
      <br>
      <div class="moz-cite-prefix">On 8/20/15 4:19 PM, Valerie Peng
        wrote:<br>
      </div>
      <blockquote cite="mid:55D66095.4090309@oracle.com" type="cite">
        <meta content="text/html; charset=UTF-8"
          http-equiv="Content-Type">
        Rajan,<br>
        <br>
        Here are some comments for the other half of the webrev:<br>
        TestCipherTextLength.java<br>
        1) line 82 and 85, why TEXT_LEN is needed? I don't find any
        usage of it inside runAll(..) method?<br>
      </blockquote>
      removed.<br>
      <blockquote cite="mid:55D66095.4090309@oracle.com" type="cite"> 2)
        the names here are a bit too similar, e.g. PBEWrapper,
        PBECipherWrapper, AESPBEWrapper. The individual subclasses
        aren't that big, why not make them inner static classes of the
        parent class? No need to repeat the Wrapper and PBE in the
        subclass names. To match your other wrapper class
        DESCipherWrapper, perhaps you can rename the PBE counterpart to
        something like:<br>
        <br>
            PBECipherWrapper (parent) with subclasses (defined
        statically inside PBECipherWrapper) named PBKDF2, AES, and
        Legacy (these are PKCS#5 v1.5 defined algorithms).<br>
      </blockquote>
      fixed, thanks!<br>
      <blockquote cite="mid:55D66095.4090309@oracle.com" type="cite"> <br>
        PBKDF2Wrapper.java<br>
        1) typo on line 43 and 52 - should be T<u>R</u>ANSFORMATION<br>
      </blockquote>
      fixed<br>
      <blockquote cite="mid:55D66095.4090309@oracle.com" type="cite"> 2)
        Instead of "PKDF2_DEFAULT_KEY_LEN" (typo? I think u meant
        PBKDF2), it should really be CIPHER_KEY_SIZE since the purpose
        here is to generate keys for the Cipher object. <br>
      </blockquote>
      renamed the variable.<br>
      <blockquote cite="mid:55D66095.4090309@oracle.com" type="cite"> 3)
        the key that you generated on line 57 can be viewed as an
        intermediate key. You could have moved the processing in
        initCipher(...) method into the constructor and store the
        resulting cipher key into 'key'. For example:<br>
            SecretKey derivedKey =
        keyFactory.generateSecret(pbeKeySpec);<br>
            key = new SecretKeySpec(derivedKey.getEncoded(),
        KEY_ALGORITHM);<br>
      </blockquote>
      key is generated in consturctor.<br>
      <blockquote cite="mid:55D66095.4090309@oracle.com" type="cite"> <br>
        Thanks,<br>
        Valerie<br>
        <br>
        On 8/19/2015 3:38 PM, Valerie Peng wrote:
        <blockquote cite="mid:55D50580.8090402@oracle.com" type="cite">Rajan,


          <br>
          <br>
          TestCipher.java <br>
          1) this class is used as the base for many algorithm, modes,
          and paddings. But then it hardcodes some values or parameter
          types which may not work against some algorithms, e.g.
          IvParameterSpec with 8-byte value, padding length being 8
          bytes. PAD_LEN=8 vs PAD_BYTES=16, is this difference
          intentional? <br>
          Seems to me that this TestCipher class only works against
          Ciphers w/ 8-byte blocks and uses iv. I think it's clearer to
          have a constant named BLKSIZE instead of just 8. <br>
        </blockquote>
      </blockquote>
      Yes. I have changed iv to be final variable now initialized in
      constructor.<br>
      <blockquote cite="mid:55D66095.4090309@oracle.com" type="cite">
        <blockquote cite="mid:55D50580.8090402@oracle.com" type="cite">
          2) Also, u don't need to repeatedly generate random bytes for
          input data, IV, etc. The input data can be generated once
          outside of loop. As for the parameters, either u can just let
          the cipher object generates it or grab the IV values from
          whatever data that u have already, e.g. certain part of the
          input data. <br>
        </blockquote>
      </blockquote>
      fixed, no need for random anymore.<br>
      <blockquote cite="mid:55D66095.4090309@oracle.com" type="cite">
        <blockquote cite="mid:55D50580.8090402@oracle.com" type="cite">
          2) input length is always 800. Would be nice to have a comment
          stating the requirement, e.g. multiple of blocks in order to
          work against ciphers w/ NoPadding <br>
          3) line 54, Would be nice to clarify that the range of tested
          key size is from MINIMUM_KEY_SIZE to what returned by
          Cipher.getMaxAllowedKeyLength(ALGORITHM) with the increment of
          KEYCUTTER. <br>
        </blockquote>
      </blockquote>
      added comments.<br>
      <blockquote cite="mid:55D66095.4090309@oracle.com" type="cite">
        <blockquote cite="mid:55D50580.8090402@oracle.com" type="cite">
          4) line 66, keyStrength is better phrased as variousKeySize in
          my opinion. <br>
        </blockquote>
      </blockquote>
      renamed to variousKeySize.<br>
      <blockquote cite="mid:55D66095.4090309@oracle.com" type="cite">
        <blockquote cite="mid:55D50580.8090402@oracle.com" type="cite">
          5) MINIMUM_KEY_SIZE and KEYCUTTER should be algorithm
          specific. It'd be clearer to explicitly list out the
          algorithms which works against the the hardcoded values for
          the sake of maintenance. <br>
        </blockquote>
      </blockquote>
      added comments.<br>
      <blockquote cite="mid:55D66095.4090309@oracle.com" type="cite">
        <blockquote cite="mid:55D50580.8090402@oracle.com" type="cite">
          6) It looks to me that String[] modes and String[] paddings
          could be moved to runAll(). This seems more consistent with
          the signature of runTest() where mode and padding are again
          specified here. <br>
        </blockquote>
      </blockquote>
      I left it as is and changed runTest to private method. Either is
      ok with me. <br>
      <br>
      Thanks,<br>
      Rajan<br>
      <blockquote cite="mid:55D66095.4090309@oracle.com" type="cite">
        <blockquote cite="mid:55D50580.8090402@oracle.com" type="cite">
          <br>
          Will send u comments regarding the TestLength ones in a
          separate email. <br>
          Thanks, <br>
          Valerie <br>
          <br>
          On 7/14/2015 1:27 PM, Rajan Halade wrote: <br>
          <blockquote type="cite">May I request you to review new tests
            added to check Cipher operations with different algorithms
            and modes. <br>
            <br>
            Webrev: <a moz-do-not-send="true"
              class="moz-txt-link-freetext"
              href="http://cr.openjdk.java.net/%7Erhalade/8048601/webrev.00/">http://cr.openjdk.java.net/~rhalade/8048601/webrev.00/</a>
            <br>
            Bug: <a moz-do-not-send="true"
              class="moz-txt-link-freetext"
              href="https://bugs.openjdk.java.net/browse/JDK-8048601">https://bugs.openjdk.java.net/browse/JDK-8048601</a>
            <br>
            <br>
            Thanks, <br>
            Rajan <br>
          </blockquote>
        </blockquote>
      </blockquote>
      <br>
    </blockquote>
  </body>
</html>