<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<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>
<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>
<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>
<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>
<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>
<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>
</body>
</html>