Hi, Jamil,
Here are some comments:
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 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.
2. line 169, move the constant 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".
3. validateRC2PS(), line 987, possible NPE? We should check and reject
null IV as CBC mode requires IV. However RC2ParameterSpec may return
null IV.
4. validateRC5PS(), line 1008 - 1014: we already have same check inside
javax.crypto.spec.RC5ParameterSpec, so we don't have to duplicate it here.
5. line 886 - 888: seems strange that you check that kdfType.prfParams
is not null, but then hard code the 1st argument to be null when calling
kdfType.prfParams.encode(...). When setting up these enums, you already
construct them with 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.
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.
Thanks,
Valerie
On 5/24/2019 3:51 PM, Jamil Nimeh wrote:
Hello all, happy Friday!
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:
* Add DER Encode/Decode support for the following OIDS from RFC 8018:
o PRFs: HmacSHA512/224, HmacSHA512/256
o Encryption Schemes: AES-192-CBC, DES, Triple-DES, RC2, RC5
* Enforce init-time type consistency between AlgorithmParameterSpec
objects and the algorithms they are used with (i.e. No using
RC5ParameterSpec with AES-128-CBC.
* Enforce sanity checks on AlgorithmParameterSpec objects used to
init (e.g. IV length checks, integer range checks, etc.)
* 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
* Allow incoming DER encoded AlgorithmIdentifier structures to honor
the OPTIONAL qualifier on the parameters field for both PRFs and
Encryption Schemes.
* 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
More details are in the CSR.
CSR: https://bugs.openjdk.java.net/browse/JDK-8221936
Bug: https://bugs.openjdk.java.net/browse/JDK-8076999
Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8076999/webrev.01/
--Jamil
