Hi Max,
Here is the updated webrev
http://cr.openjdk.java.net/~valeriep/8242151/webrev.01/
which should address your earlier comments (see below)
- OidString renamed to KnownOIDs and updated the aliasing handling code
in both KnownOIDs and SecurityProviderConstants classes
- Removed additional oid constants in AlgorithmId class which are only
referenced by regression tests, so webrev.01 adds two more regression
tests accordingly
- Moved the computeOidTable() and its relevant code to AlgorithmId class
- Use extra standard name arg for ExtendedKeyUsage enums.
Inside getAliases(), why must there be an exception when there is no
OidString.findMatch(o)? Does every algorithm name need an OID? I thought of
X.509 but you have added it into aliases.
Hmm, the exception is meant to catch scenarios where the specified name
has a typo and to signal the caller that expected aliases is not found.
I updated the code in webrev.01 to make it clearer.
Valerie
On 4/27/2020 6:37 PM, Weijun Wang wrote:
On Apr 28, 2020, at 8:08 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
Yes, it can be quite a confusing scenario. Some algorithms have multiple oids
(say X) and multiple algorithm names (say Y). We need to be able to handle all
of them (X+Y). However, the separation of alias definition for OidString and
SecurityProviderConstants are intentional and I have them separated with the
reason below:
The aliases in OidString can be viewed as internal, e.g. from JCA classes and
used by AlgorithmId class and its regression tests. All of the supported oids
(X of them) have to have an OidString enum type as their usage may be present
in the DER encoding and all internal classes need to handle it. As for the
multiple algorithm names, supposedly, there is only one standard name and the
rest are all aliases. Aliases can be used to request the impl from provider,
but that's all. Internal classes do not need to understand these
external/provider-related aliases. By separating these external aliases inside
SecurityProviderConstants, it is clear that these aliases are for providers
only.
I understand.
In SecurityProviderConstants::getAliases, is it possible to combine
aliases.get(o) and OidString.findMatch(o)? Or we can rewrite store() and grab
info from OidString so that there is no need to always add OID.oid and oid in
the store() calls.
Inside getAliases(), why must there be an exception when there is no
OidString.findMatch(o)? Does every algorithm name need an OID? I thought of
X.509 but you have added it into aliases.
StdName itself should not be its aliases. Which line in
SecurityProviderConstants class has this problem?
Sorry, I got it wrong. I've mistakenly thought that OidString.DSA.value() is
the name.
Thanks,
Max
Thanks,
Valerie
On 4/25/2020 7:46 AM, Weijun Wang wrote:
And I am confused by aliases in SecurityProviderConstants, together with the
duplicated stdName in OidString, it seems to show that one OID could have
multiple names, and one name could have multiple OIDs. Can we consolidate
aliases inside OidString also?
Also, looking at the store() calls in SecurityProviderConstants, it looks like
a stdName itself is one of its aliases. Is this really useful? From what I read
in the provider classes, you are adding ServiceKeys based on these aliases to a
serviceMap which already has a ServiceKey based on stdName.
Thanks,
Max
On Apr 25, 2020, at 6:28 PM, Weijun Wang <weijun.w...@oracle.com> wrote:
OidString.java
==============
1. ExtendedKeyUsage names: used to be "serverAuth", now "ServerAuth". First
letter capitalized, is this necessary?
2. Can we move name2oidStr() from OidString to AlgorithmId? The computeOidTable
process looks like an alien.
3. Two questions on the following lines:
415 // set extra alias mappings or specify the preferred one when
416 // one standard name maps to multiple enums
417 // NOTE: key must use UPPER CASE
418 name2enum.put("SHA1", SHA_1);
419 name2enum.put("SHA", SHA_1);
420 name2enum.put("SHA224", SHA_224);
421 name2enum.put("SHA256", SHA_256);
422 name2enum.put("SHA384", SHA_384);
423 name2enum.put("SHA512", SHA_512);
424 name2enum.put("SHA512/224", SHA_512$224);
425 name2enum.put("SHA512/256", SHA_512$256);
426 name2enum.put("DH", DiffieHellman);
427 name2enum.put("DSS", SHA1withDSA);
428 name2enum.put("RSA", RSA);
a) For line 428, is this because both RSA and ITUX509_RSA have the same stdName and you are setting the
preferred one? However, I can see that "DiffieHellman", "DSA", and
"SHA1withDSA" also appear in multiple places. Do they need special attention?
b) For the other lines, can we make this info somewhere inside the
constructor? After all our goal is to consolidate all info in one single place,
and a single line is better than a single file, esp, a very big file.
4. Are you sure the OID <-> name mapping is always the same everywhere (for all
primitives and in all providers)? I mean for a stdName, if one OID alias is added in
one place, should it always be added the same way in another? Have you compared the
aliases map after this change?
5. I found KnownOIDs to be a better class name.
AlgorithmId.java
================
There are still many lines like
public static final ObjectIdentifier MD2_oid = algOID(OidString.MD2);
here. Can they be eliminated? I use IntelliJ IDEA to find their usages and most
are used in only one place and some are not used at all.
I haven't read other files yet. Will send more comment later.
Thanks,
Max
On Apr 24, 2020, at 7:11 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
Hi Max,
Would you have time to review this change? The current webrev attempts to cover
all security classes where hard-coded oid strings and consolidate these known
oid string values into a single enum type. The changes are quite extensive, I
can trim back and only cover the provider algorithm oids if you prefer. There
are pros and cons for both approach.
I know that the naming convention is to use all upper case for enum constants,
but choose to use the same naming convention as standard names to simplify the
code. SecurityProviderConstants class defines the common mappings which are
general to providers. Provider-specific alias mappings are handled in specific
provider class, e.g. SunJSSE class.
RFE: https://bugs.openjdk.java.net/browse/JDK-8242151
Webrev: http://cr.openjdk.java.net/~valeriep/8242151/webrev.00/
Mach5 runs clean.
Valerie
