<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Valerie, I don't recall if I ever responded to you on this as I
stepped away from this to work another issue. Comments are in-line,
and I have another email following this one with more because you
had extra comments in that other email.<br>
<br>
Thanks,<br>
--Jamil<br>
<br>
<div class="moz-cite-prefix">On 7/9/2019 7:44 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:d5d13b50-7a0a-7073-2877-6b1a5fed1d09@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<p><src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java></p>
<p>- line 174: considering add AES128/192/256 w/ CBC oids as they
are used by both AlgorithmParameters and Cipher entries.</p>
</blockquote>
JN: I'm not exactly sure what you're asking for here. The 3 OIDs in
that call to createAliasesWithOid are the OIDs for
AES128/192/256-CBC. If you're asking me to use the variables
"aes128Oid", "aes192Oid" and "aes256Oid" then I cannot use those
just by themselves. They'd end up having to get "2" appended to
them, just as it is done for the AES/<nn>/CBC Cipher lines
around 245-290. Is there some advantage to doing that over what's
currently there? Am I missing the point of what you're asking for?<br>
<blockquote type="cite"
cite="mid:d5d13b50-7a0a-7073-2877-6b1a5fed1d09@oracle.com">
<p> </p>
<p><src/java.base/share/classes/com/sun/crypto/provider/RC5Parameters.java></p>
<p>- line 108 seems redundant? <br>
</p>
<p>- line 131-132: why not simply: (iv.length != (blockSizeInBits
>> 3)?<br>
</p>
</blockquote>
JN: I like your approach much better, I'll fix that.<br>
<blockquote type="cite"
cite="mid:d5d13b50-7a0a-7073-2877-6b1a5fed1d09@oracle.com">
<p> </p>
<p>- line 135: after checking iv.length, should we check for extra
trailing bytes and error out if (val.data.available() > 0)?</p>
</blockquote>
JN: It turns out not to be necessary. The encapsulation of the DER
encoded data into the DerValue as the first thing done already
checks to make sure there's no extra data. I did add a test to
RC5ParametersTests that adds another block of DER-encoded data so at
least we test that specific case explicitly.<br>
<blockquote type="cite"
cite="mid:d5d13b50-7a0a-7073-2877-6b1a5fed1d09@oracle.com">
<p>- line 191 encoder can be moved inside the block of iv-dumping
code as it's only used there.<br>
</p>
</blockquote>
JN: No problem. Will do.<br>
<blockquote type="cite"
cite="mid:d5d13b50-7a0a-7073-2877-6b1a5fed1d09@oracle.com">
<p> </p>
<p> </p>
<p><test/jdk/com/sun/crypto/provider/AlgorithmParameters/PBES2Parameters.java></p>
<p><test/jdk/com/sun/crypto/provider/AlgorithmParameters/RC5Parameters.java></p>
<p>- Can the tests be renamed? So it's clear that they are tests
instead of some utility classes for test.</p>
</blockquote>
JN: Sure, I can just make it <BASENAME>Tests.java<br>
<blockquote type="cite"
cite="mid:d5d13b50-7a0a-7073-2877-6b1a5fed1d09@oracle.com">
<p>- The ASN.1 encoding are generated offline using your changes?
Just curious.</p>
</blockquote>
JN: The ASN.1 encodings for PBES2Parameters was done using OpenSSL.
For RC5Parameters, I did those by hand since OpenSSL doesn't
automatically generate them through any tool like the pkcs8
command. Fortunately they're small ASN.1 structures, so it's pretty
easy to crank out with a hex editor.<br>
<blockquote type="cite"
cite="mid:d5d13b50-7a0a-7073-2877-6b1a5fed1d09@oracle.com">
Thanks,<br>
Valerie<br>
<div class="moz-cite-prefix">On 7/8/2019 4:50 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:f52ee176-1d0c-473d-de4d-bcbc94997927@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
<p>Hi Jamil,</p>
<p>Sorry for the late reply. It's been a long while since I
looked at this PBES2 scheme and I need to think a few things
through.<br>
</p>
<p><src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java></p>
<p>- line 29-30, 37, I don't see Constructor, BigInteger, and
ArrayList being used?</p>
<p>- line 158: the pbes2AlgorithmNameis initialized to null. Is
it our intention to return null if toString() is called upon
an PBES2 AlgorithmParameters object without init(...) call?</p>
<p>- line 176: does "keysize" means the optional keyLength field
inside the PBES2-params struct? Its value is from various
sources. A proper definition would help ensure its value is
correctly set.</p>
<p>- line 207: If encoding to a DerOutputStream, why not decode
using a DerInputStream? <br>
</p>
<p>- line 203: based on RFC3370, 5911, the preferred way for
HMAC is to omit the parameters instead of encoding a NULL. So,
we probably should not encode a null on line 203. Also, for
decode, line 520 to 522 can be moved here so that the decode
can handle both cases, i.e. omitted and present NULL, for max
compatibility.</p>
<p>- line 295 - 298: if there is a comment on line 271 which
explains when "keyLen" is for, then we don't need this block
of comment. Essentially, keyLen holds that restricted key
length value, right? KEYLEN_ANY means no restriction
whatsoever.</p>
<p>- line 376: the impl of validateEncParams() seems to allow
null cipherParams as it returns immediately if its value is
null. I am not sure if we should allow null cipherParam though
as this cipherParam object is needed for encoding
"encryptionScheme" field of the PBES2-params struct. For
example, if the parameters field for AES_128_CBC must contain
16-byte IV, then a null cipherParams should be rejected. Same
goes to other encryption schemes.<br>
</p>
<p>- line 379: Since encyptionType is found using cipherAlgo and
keysize in the constructor, why can't we just format <span
class="changed">encryptionType.schemeName as in</span> <span
class="changed">getEncSchemeName()? </span><span
class="changed"><br>
</span></p>
<p>- line 419-422: Shouldn't this check be moved up to line 414,
i.e. in the block of code which handles the buggy encoding?
Otherwise, it looks like a duplicate check of line 400.</p>
<p>- line 494, 630, 735: change the check to use KEYLEN_ANY?</p>
<p>-line 638: extra indentation?</p>
<p>- line 725-752: Seems better to check the IV length before
checking and set the keysize.<br>
</p>
<p>- line 788: May return RC2_-1 or RC5_-1?<br>
</p>
<p>Will send you comments for the rest of webrev separately.</p>
<p>Thanks,</p>
<p>Valerie<br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><br>
</p>
<p><src/java.base/share/classes/com/sun/crypto/provider/RC2Parameters.java></p>
<p><br>
</p>
<p><src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java></p>
<p><br>
</p>
<p><src/java.base/share/classes/com/sun/crypto/provider/RC5Parameters.java></p>
<p><br>
</p>
<p><test/jdk/com/sun/crypto/provider/AlgorithmParameters/PBES2Parameters.java></p>
<p><br>
</p>
<p><test/jdk/com/sun/crypto/provider/AlgorithmParameters/RC5Parameters.java><br>
</p>
<p><br>
<code></code></p>
<div class="moz-cite-prefix">On 6/20/2019 6:59 PM, Jamil Nimeh
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:bd96ce08-18d5-74bc-045f-3ed48e534fff@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
Hello all,<br>
<br>
I've updated the fix to 8076999 with the following changes:<br>
<ul>
<li>We now use sun.security.x509.AlgorithmId and it
internally uses AlgorithmParameters implementations to
handle the DER encoding and decoding of encryption scheme
parameters.<br>
</li>
<ul>
<li>This means that we need to add one new standard name
and some OID aliases for some AlgorithmParameters. See
the CSR link below for details.</li>
</ul>
<li>Added a new RC5Parameter AlgorithmParameters
implementation to SunJCE, plus unit tests.</li>
</ul>
<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.02"
moz-do-not-send="true">http://cr.openjdk.java.net/~jnimeh/reviews/8076999/webrev.02</a></p>
<br>
<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>
<br>
</blockquote>
</blockquote>
</blockquote>
<br>
</body>
</html>