<html>
  <head>
    <meta http-equiv="content-type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p>Hello all,</p>
    <p>I'm finally getting back around to this after dusting off the
      cobwebs.</p>
    <p>Webrev:
      <a class="moz-txt-link-freetext" href="https://cr.openjdk.java.net/~jnimeh/reviews/8076999/webrev.03">https://cr.openjdk.java.net/~jnimeh/reviews/8076999/webrev.03</a></p>
    <p>Bug: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8076999">https://bugs.openjdk.java.net/browse/JDK-8076999</a><br>
    </p>
    <p>Valerie, you had some comments from way back (7/9/2019).  Just a
      short summary of what's been done to address them:</p>
    <ul>
      <li>Removed unused imports</li>
      <li>Added a default "PBES2" string value for when the toString
        method is called on a PBES2Parameters object before the init()
        method.</li>
      <li>Tested encoding of PBKDF2 parameters AlgorithmIdentifiers with
        the optional parameters field not present (as opposed to an
        ASN.1 NULL).  OpenSSL seems happy with it so that's how we'll
        encode those.</li>
      <li>Switched order on the IV and keysize parsing for RC2
        parameters</li>
      <li>Using KEYLEN_ANY (changed to KEYLEN_UNSPEC) now in lieu of -1
        in the conditionals you cited.</li>
      <li>For algorithms where the key length is implicit either due to
        the algorithm or the specific OID, we no longer assert the key
        length in the KDF parameters.  This is consistent with other
        implementations such as OpenSSL.<br>
      </li>
      <li>Regarding the comment from the parsing in engineInit(byte[])</li>
    </ul>
    <p>"By calling data.getDerValue(), we are essentially peeling one
      layer off, right? If you still agree with me at this point, then
      note that pBES2_params is a local variable and its value should be
      the same unless explicitly re-assigned (as on line 413). Thus, per
      my reading of the code, the tag that you are checking on line 419
      is not the one for encryption scheme, but rather the outer
      sequence tag encapsulating kdf and encryption scheme. Its current
      location is very misleading though, in between kdf and encryption
      scheme. To really check the tag for kdf and encryption scheme, the
      tag checking should be in parseKDF(...) and parseES(...) against
      the DerValue argument."</p>
    <p>I did add a DerValue check in parseKDF because it is appropriate
      as you stated.  I also removed the check from line 419 in the old
      webrev.  With the new code that check is redundant as we are using
      AlgorithmId.parse() now as the initial operation in parseES, which
      in turn does the sequence tag check for us.</p>
    <p>The check itself on 419 though is testing the ASN.1 tag for the
      Encryption scheme, not the higher level sequence for
      PBES2-params.  Otherwise neither the KDF nor the encryption scheme
      would parse properly and none of the tests would pass.<br>
    </p>
    <p>With this new code, parseKDF and parseES are testing the outer
      SEQUENCE tags for each of the AlgorithmIdentifier objects
      described by keyDerivationFunc and encryptionScheme per RFC 8018.</p>
    <ul>
      <li>keysize setting: This one is a bit tricky because key size is
        specified in multiple ways.  The basic flow is this.  Key size
        will start as KEYLEN_UNSPEC.  If it is at any point specified,
        either via the constructor, by KDF params, or by enc params then
        that is the value that is set.  At near the end of parsing a
        validation of the encryption parameters occurs and at that point
        the key length is checked against the algorithm.  If it is an
        alg that uses a fixed value then keysize has to be consistent
        with it.  If it is variable length and it is still
        KEYSIZE_UNSPEC that is also a failure (since something needs to
        be specified to distinguish RC2_40 from RC2_128, for example).</li>
      <ul>
        <li>This approach seems to work and catches the issue you found
          where certain DER encodings could yield things like
          PBEWithHmacSHA256AndRC5_-1.</li>
      </ul>
      <li>Added some new tests to handle the changes listed above.</li>
    </ul>
    <p>I'll be updating the CSR shortly to reflect comments there and
      I'll send a separate review notice for that.</p>
    <p>Thanks,</p>
    <p>--Jamil<br>
    </p>
    <ul>
    </ul>
  </body>
</html>