<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p>Hi Jamil,<br>
    </p>
    <p>I looked at java.security.AlgorithmParameters and need to update
      my earlier comment below</p>
    <p>
      <blockquote type="cite">- By convention, each init() is a fresh
        start and wipes out the effect previous init() calls. But in the
        current webrev, they seems to apply changes on top of each
        other. This may not be the right model of how things should be
        handled. In addition, with the existing code handles both PBES2
        and PBEwith<KDF>And<CIPHER>, we may need extra logic
        to restore the fields back to when they are first constructed at
        the beginning of every engineInit(...).<br>
      </blockquote>
    </p>
    <p>It turns out that AlgorithmParameters have an "initialized" flag
      which would only accept one successful init() call.</p>
    <p>Subsequent call would be rejected by throwing either IOException
      or InvalidParameterSpecException. So, I guess we don't need to
      worry about multiple init() calls. Just that the supplied data is
      legit and is not conflicting with the assumed algorithm values if
      the object is requested under the "friendly" name.</p>
    Thanks,<br>
    Valerie<br>
    <div class="moz-cite-prefix">On 3/30/2020 8:25 PM, Valerie Peng
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:15e3ec7a-b85b-dd36-2bc3-d86dcee57b2d@oracle.com">
      <p>Hi Jamil,</p>
      <p>Thanks for being so patient. It take me sometime to play around
        with the changes and think about various scenarios...</p>
      <p>Here are some comments:</p>
      <p><RC5Parameters.java></p>
      <p>- Line 38 has RFC 2268 which is for RC2, RC5 is in RFC 2040. <br>
      </p>
      <p>- Line 48-51 comments can be simplified further, essentially,
        IV (provided or not) shall have the same length as the block
        size. If not provided explicitly, then its values are all 0s.
        Just some nit. <br>
      </p>
      <p><RC5ParametersTests.java></p>
      <p>- line 187: variable name 'pbeSpec' should probably be named as
        'rc5Spec' as it has nothing to do with PBE.</p>
      <p><PBES2ParametersTests.java></p>
      <p>- The test is very extensive... May I ask how are the test
        vectors generated? Are they from some RFCs or external website
        like NIST or internally generated using these new impl? In
        particular, I wonder about the test "HmacSHA256 and AES-256-CBC
        [No Enc parameters]", this is expected to pass, but AES CBC
        requires IV and if no parameters are explicitly specified, how
        do things work for decryption where IV is required and cannot
        generate random values as default IV? I am also not sure about
        the InitByDERAndEncodeTest, I think we should not alter the
        given encoding based on our own assumptions/defaults. If given a
        DER encoding bytes, the same (minor difference such as an absent
        null params is ok) encoding should be returned when getEncoded()
        is called.<br>
      </p>
      <p>- Line 143 expVals[4] should be expVals[3]?</p>
      <p><PBES2Parameters.java></p>
      <p>- Current changes are a bit too complicated and I am not sure
        if it's worthwhile to support RC2, RC5, DESede and DES and do
        their algorithm-specific checking. PBES2 AlgorithmParameters
        parsing is already complicated due we support the original name
        (PBES2) and also the "friendly"  JCA naming convention of
        including the cipher name and KDF name. On top of it, the
        current webrev seem to trying to support parsing of all possible
        algorithms stated in PKCS#5 even when SunJCE provider only
        support the AES_xxx variants. If only parsing is done, the
        overhead may be acceptable, but then when there is also
        algorithm specific checking, I feel this is a bit much as I
        doubt that they will ever be used. <br>
      </p>
      <p>- There are a lot of String parsing inside the String-arg
        constructor which can be avoided if we replace this String-arg
        constructor with a 2-arg constructor with PrfType and EncType.
        This should simplify the constructor code greatly and make it
        more robust. Then we probably can remove the getByName() method
        for both enum types. No need for the constructor to throw
        NoSuchAlgorithmException as all inputs are provided by provider
        and unsupported algorithm should be detected before calling this
        constructor.<br>
      </p>
      <p>- Line 259, add "," after the word "key"?</p>
      <p>- The ordering of things under
        engineInit(AlgorithmParameterSpec paramSpec) seems a bit
        un-intuitive. User-supplied values should only be stored after
        pass validation. The assignment (line 324-326) should be done
        after the various checks. The check at line 338 should be moved
        up before assigning the default kdf type. <span class="new"><br>
        </span></p>
      <p><span class="new">- Missing PBE subclasses for AES_192?</span></p>
      <p><span class="new">- Some of the static oid constants seems
          unnecessary as they are only used inside the enum and can be
          moved there.<br>
        </span></p>
      <p>- By convention, each init() is a fresh start and wipes out the
        effect previous init() calls. But in the current webrev, they
        seems to apply changes on top of each other. This may not be the
        right model of how things should be handled. In addition, with
        the existing code handles both PBES2 and
        PBEwith<KDF>And<CIPHER>, we may need extra logic to
        restore the fields back to when they are first constructed at
        the beginning of every engineInit(...).</p>
      <p>- I am not too sure about the usefulness of <span class="new">"pbes2AlgorithmName"
          field. In the current impl, it is set in various places. If it
          is meant to reflect the latest KDF and CIPHER algorithm used,
          it's more robust to construct its value when needed.
          Otherwise, we need to remember updating this value whenever
          one of these 3 values, i.e. KDF, CIPHER, and keysize, changed.<br>
        </span></p>
      <p>- Consider grouping the fields of salt, iteration count,
        keysize, prfType into separate class and move the PBKDF2
        parsing/encoding code there. This simplifies the validation and
        setting of these 4 fields.<br>
      </p>
      <p>- Avoid algorithm-specific checking in this class as it is not
        scalable and duplicates the checking in the algorithm-specific
        classes. If you feel they must be done, delegate to the
        algorithm-specific classes as much as possible. Instead of
        explicitly checking the parameter spec, use the sequence of
        AlgorithmParameters.getInstance(String), and its init(...) call
        and see if the call passes.</p>
      <p>- There are also code which sets the keysize for RC2 and RC5
        key sizes based on the PBKDF2 and cipher parameters. I think
        it's reasonable to derive the value from the PBKDF2 params, but
        not cipher parameters. In the case of RC5, it even assigns a
        default value (line 760) when all else is failed. Given the
        purpose of this class is for PBES2 algorithm parameters and we
        don't support PBES2 cipher with RC2 or RC5, I think we should
        not go this far. Is there anything that I missed for requiring
        to set the keysize in this class? If "PBES2" AlgorithmParameters
        are requested and initialized with DER encoding, we should
        return the same encoding (unless it's mistakenly encoded as
        stated in line )  when getEncoded() is called. It's also
        somewhat strange that the toString() method returns only the
        "friendly" expanded name without other info. This is different
        from other AlgorithmParameters impl. But this is just nit
        comparing to other things.<br>
      </p>
      <p>I adapted your changes with most of my feedback above (except
        the one on each engineInit() call being independent) and you can
        find the changes here: <a class="moz-txt-link-freetext"
          href="http://cr.openjdk.java.net/~valeriep/pbes2Exp/webrev/"
          moz-do-not-send="true">http://cr.openjdk.java.net/~valeriep/pbes2Exp/webrev/</a>
        (The regression test PBES2ParametersTest.java has to be updated
        a little in order to pass). Hope this can help you understand my
        comments.<br>
      </p>
      Thanks!<br>
      Valerie
      <p><br>
      </p>
      <div class="moz-cite-prefix">On 3/18/2020 3:59 PM, Valerie Peng
        wrote:<br>
      </div>
      <blockquote type="cite"
        cite="mid:06fc3f21-225f-7262-fea3-29a6d803b328@oracle.com">
        <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>
      </blockquote>
    </blockquote>
  </body>
</html>