<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Hi Valerie, thank you for all the helpful comments Valerie, comments
    in-line:<br>
    <br>
    <div class="moz-cite-prefix">On 6/13/2019 7:11 PM, Valerie Peng
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:eac0ad3b-d2d6-6a48-fa63-d266cc406dc1@oracle.com">
      <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
      <p>Hi, Jamil,</p>
      <p>Here are some comments:</p>
      <p>1. ParamProcessor and its various implementations: Essentially,
        you are doing the same thing as parsing/encoding of
        AlgorithmParameters for various algorithms. For RC2, we already
        have code for doing this and it seems that for max consistency
        and interoperability, we should try to leverage what we have
        instead of writing new code. As RC2_CBC can be considered as one
        usage case of RC2Parameters, I think we can do without the new <span
          class="new">rc2ParamProcessor class and just add
          PBES2-specific checking once the parsing is done. For RC5
          parameters which isn't currently supported, we can add support
          for AlgorithmParameters. Or, another alternative is to put
          these encoding/decoding code into a separate class and
          refactor existing code to all call into this new class, and do
          PBES2-specific checking inside PBES2Parameters.<br>
        </span></p>
    </blockquote>
    JN: I think we can make this work using AESParameters,
    DESParameters, DESedeParameters and RC2Parameters.  I had originally
    tried to avoid making and exposing a new AlgorithmParameters
    standard name to accommodate RC5, but if we move to using those to
    do the parsing then I may as well.  Most of the code I've written
    should carry over.<br>
    <blockquote type="cite"
      cite="mid:eac0ad3b-d2d6-6a48-fa63-d266cc406dc1@oracle.com">
      <p><span class="new"> </span></p>
      <p><span class="new">2. line 169, move the constant </span><span
          class="new">KEYLEN_ANY up to where the other constants are
          defined, i.e. ~line149. line 170, I think you mean to
          initialize keysize as KEYLEN_ANY? I think it'll be clearer to
          do so, instead of just "-1".<br>
        </span></p>
    </blockquote>
    JN: Yes, good call.  I'll do that.<br>
    <blockquote type="cite"
      cite="mid:eac0ad3b-d2d6-6a48-fa63-d266cc406dc1@oracle.com">
      <p><span class="new"> </span></p>
      <p><span class="new">3. validateRC2PS(), line 987, possible NPE?
          We should check and reject null IV as CBC mode requires IV.
          However RC2ParameterSpec may return null IV. <br>
        </span></p>
    </blockquote>
    JN: An NPE can't happen here, because the calling function checks
    for a null cipherParam field and throws an exception if it is not
    provided.  But IIRC I was trying to handle cases where
    init(AlgorithmParameterSpec) was called with a null parameter (which
    does happen, BTW...some PKCS#12 unit tests require it).  But I don't
    allow a null RC2ParameterSpec.  Maybe I need to.  RFC 8018 already
    says that a missing effective key bits should default to 32 and a
    missing IV parameter spec *could* be interpreted by an
    AlgorithmParameters consumer as all zeroes.<br>
    <br>
    Before I started on this fix we allowed people to init with a null
    APS and we would return a PBEParameterSpec with a null value for the
    underlying cipher parameter spec.  I don't think I want to change
    that behavior.<br>
    <br>
    Long story short, if I do decide to allow a null RC2ParameterSpec on
    the way in, I do need to check for a null as you suggest.<br>
    <blockquote type="cite"
      cite="mid:eac0ad3b-d2d6-6a48-fa63-d266cc406dc1@oracle.com">
      <p><span class="new"> </span></p>
      <p><span class="new">4. validateRC5PS(), line 1008 - 1014: we
          already have same check inside
          javax.crypto.spec.RC5ParameterSpec, so we don't have to
          duplicate it here.</span></p>
    </blockquote>
    JN: Yes, you're right.  I can remove this.<br>
    <blockquote type="cite"
      cite="mid:eac0ad3b-d2d6-6a48-fa63-d266cc406dc1@oracle.com">
      <p><span class="new">5. line 886 - 888: seems strange that you
          check that </span><span class="changed">kdfType.prfParams is
          not null, but then hard code the 1st argument to be null when
          calling k</span><span class="changed">dfType.prfParams.encode(...).
          When setting up these enums, you already construct them with 
        </span><span class="new">nullParamProcessor, but yet when
          calling the encode(...), the caller has to know to pass null
          argument again, seems a bit redundant to define a
          nullParamProcessor but then still have to pass null.</span></p>
    </blockquote>
    JN: Well, here was my thinking on this one.  Right now every PRF we
    work with doesn't need parameters and always encodes it using an
    ASN.1 NULL in the AlgorithmIdentifier parameters field.  When I made
    the PrfType enum, I didn't want to build it in such a way that
    didn't account for the possibility (remote, I will admit) that we
    might get a PRF down the line that needs one, like maybe HmacSHA512T
    (the one where you can truncate to a variable length).<br>
    <br>
    In making the ParamProcessor interface, I wanted it to be generic to
    any AlgorithmIdentifier param implementation, and some of these need
    parameters (the encryption ones) and some don't (the PRFs
    currently).  So when I got down to the line you cited, I took a
    short cut and just threw the null in there for the time being, since
    even the new PRF algs I've added support for don't need any
    AlgorithmParameterSpec.  I figured I could do that for now for
    simplicity and if we ever got a PRF that needs parameters we could
    rework the logic.  Also it is worth noting that more would have to
    change to even support PRFs with parameters, as we have no way
    through our public PBES2 interfaces to provide them today.<br>
    <br>
    With the changes from your #1 comment above, I might not need a
    ParamProcessor interface any longer since every PRF we support takes
    no parameters and there's no public interface to provide them yet,
    and all the encryption schemes would be handled in separate
    classes.  Scaling this back seems like the better way to go.<br>
    <br>
    <blockquote type="cite"
      cite="mid:eac0ad3b-d2d6-6a48-fa63-d266cc406dc1@oracle.com">
      <p><span class="new">I am still reviewing the rest of changes, but
          thought that I will forward you what I have so you have more
          time to think about it.</span></p>
      <span class="new">Thanks,</span><br>
      <span class="new"></span><br>
      <span class="new">Valerie</span><br>
      <span class="new"></span>
      <p><span class="new"><br>
        </span></p>
      <p><span class="changed"></span></p>
      <p><span class="new"></span></p>
      <p><span class="new"></span></p>
      <div class="moz-cite-prefix">On 5/24/2019 3:51 PM, Jamil Nimeh
        wrote:<br>
      </div>
      <blockquote type="cite"
        cite="mid:6afc52c5-cf29-827a-0591-27714bcd6068@oracle.com">
        <meta http-equiv="content-type" content="text/html;
          charset=UTF-8">
        <p>Hello all, happy Friday!<br>
        </p>
        <p>Please review the following CSR and code review.  This makes
          updates to the SunJCE implementation of PBES2-based
          AlgorithmParameters.  Many of the details are in the CSR (see
          the link below).  But a short list of the updates:</p>
        <ul>
          <li>Add DER Encode/Decode support for the following OIDS from
            RFC 8018:<br>
          </li>
          <ul>
            <li>PRFs: HmacSHA512/224, HmacSHA512/256</li>
            <li>Encryption Schemes: AES-192-CBC, DES, Triple-DES, RC2,
              RC5</li>
          </ul>
          <li>Enforce init-time type consistency between
            AlgorithmParameterSpec objects and the algorithms they are
            used with (i.e. No using RC5ParameterSpec with AES-128-CBC.</li>
          <li>Enforce sanity checks on AlgorithmParameterSpec objects
            used to init (e.g. IV length checks, integer range checks,
            etc.)</li>
          <li>Fixed a bug where explicit DER decoding of the optional
            key length field in PBKDF2-params would cause the PRF to be
            forced to HmacSHA1 even if the DER indicated otherwise</li>
          <li>Allow incoming DER encoded AlgorithmIdentifier structures
            to honor the OPTIONAL qualifier on the parameters field for
            both PRFs and Encryption Schemes.</li>
          <li>If a null encryption scheme AlgorithmParameterSpec is
            provided during init time, omit the
            PBES2-params.encryptionScheme's parameter segment since it
            is OPTIONAL per the ASN.1 from RFC 5280</li>
        </ul>
        <p>More details are in the CSR.<br>
        </p>
        <p>CSR: <a class="moz-txt-link-freetext"
            href="https://bugs.openjdk.java.net/browse/JDK-8221936"
            moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8221936</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></p>
        <p>Webrev: <a class="moz-txt-link-freetext"
            href="http://cr.openjdk.java.net/~jnimeh/reviews/8076999/webrev.01/"
            moz-do-not-send="true">http://cr.openjdk.java.net/~jnimeh/reviews/8076999/webrev.01/</a></p>
        <p>--Jamil<br>
        </p>
        <p><br>
        </p>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>