<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    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>
    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>
    <br>
    PBKDF2Wrapper.java<br>
    1) typo on line 43 and 52 - should be T<u>R</u>ANSFORMATION<br>
    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>
    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>
    <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>
      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>
      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>
      4) line 66, keyStrength is better phrased as variousKeySize in my
      opinion.
      <br>
      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>
      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>
      <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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~rhalade/8048601/webrev.00/">http://cr.openjdk.java.net/~rhalade/8048601/webrev.00/</a>
        <br>
        Bug: <a 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>
  </body>
</html>