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