Sure, thanks for the detailed review~

Valerie

On 5/31/2020 9:45 PM, Weijun Wang wrote:
It looks like I am suggesting you to rollback to the old "new 
RSAXyzKey(AlgorithmId,...)" style, only that the 1st argument is now an RsaId (child 
class extends AlgorithmId) that remembers the type and spec so you don't need to call 
RSAUtil methods to translate them back.

I tried to implement this idea in RSAPublicKeyImpl but found it uncomfortable. Whenever I 
need that type and spec info from a key class, I'll have to cast algId to RsaId and this 
really smells bad. Or, I can add a "RsaId rsaId" field there but it's actually 
the same object as algId, and this is not better than your current webrev that simple 
store both type and spec into the class.

So just ignore my suggestion. You current webrev is good.

Thanks,
Max

On Jun 1, 2020, at 11:26 AM, Weijun Wang <weijun.w...@oracle.com> wrote:

The code change looks fine to me.

One small comment: I notice that "KeyType type" and "AlgorithmParameterSpec 
keyParams" always go hand in hand, either as fields inside a class, or in the argument list of 
a method. Does it make sense to combine these 2 guys into a single class? It will look like a child 
class of AlgorithmId which understand what its alg and params mean. Of course you won't be able to 
make KeyType an enum anymore.

All up to you.

Thanks,
Max


On May 30, 2020, at 5:07 AM, Valerie Peng <valerie.p...@oracle.com> wrote:

Re-worked the fix a bit to take in Max's feedback:

Webrev updated athttp://cr.openjdk.java.net/~valeriep/8242897/webrev.01/

Instead of completely reverting to the pre-PSS behavior of ignoring the OID in 
the DER encoding, this updated webrev has special handling to work with the 
scenario in the bug report. Most changes are code refactoring driven by the 
argument type change...

Thanks,
Valerie


On 5/7/2020 4:17 PM, Valerie Peng wrote:
Ping?

On 4/23/2020 3:28 PM, Valerie Peng wrote:
Anyone has time to help review this fix? After the support for RSASSA-PSS keys is added, 
when parsing the DER encoding, the key algorithm is based on the parsed algorithm 
oid/name. However, an exception is thrown if the parsed algorithm oid/name is neither RSA 
nor RSASSA-PSS. For this particular report, the algorithm oid is 1.3.14.3.2.15 which is 
unsupport/unrecognized by JDK. In earlier releases, the bytes are parsed but key 
algorithm is always "RSA".

To maintain this backward compatibility behavior, I changed the current impl to 
set the key algorithm upon key construction time w/ a KeyType argument (RSA or 
RSASSA-PSS) even when DER encoding is given. After parsing the DER encoding, 
for non-RSA keys, the parsed algorithm oid/name should match the given key 
type, otherwise an exception is thrown.

Bug: https://bugs.openjdk.java.net/browse/JDK-8242897

Webrev: http://cr.openjdk.java.net/~valeriep/8242897/webrev.00/

Mach5 run is clean.

Thanks,
Valerie

Reply via email to