<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>