<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p><br>
    </p>
    <p>Right, I recall reviewing this and made some comments. Will take
      a look at the updated webrev.</p>
    Thanks,<br>
    Valerie<br>
    <div class="moz-cite-prefix">On 3/17/2020 4:48 PM, Jamil Nimeh
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:b3f31bd1-a6a9-c9f1-4801-a4a7b08d032d@oracle.com">
      <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"
          moz-do-not-send="true">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"
          moz-do-not-send="true">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>
    </blockquote>
  </body>
</html>