On Apr 28, 2020, at 6:19 AM, Valerie Peng <[email protected]> wrote:
Hi Max,
Thanks for reviewing this~ Please find my replies inline.
On 4/25/2020 3:28 AM, Weijun Wang wrote:
OidString.java
==============
1. ExtendedKeyUsage names: used to be "serverAuth", now "ServerAuth". First
letter capitalized, is this necessary?
Yes, I made a change here. Using "ServerAuth" seems more consistent with the
rest of constants in OidString. So I made a change here. It affects the displayed out but
no regression tests are affected by this change. Well, if we want to be 100% same as
before, I can change these to start with lower case, but then we probably need comments
so it's clear that these are intentional changes.
I'd rather use the old name.
2. Can we move name2oidStr() from OidString to AlgorithmId? The computeOidTable
process looks like an alien.
Well, I think it's better to consolidate all oid string/algorithm names to as
few places as possible.
AlgorithmId class used to contain a lot of such conversions and now that there
is OidString class (we can discuss a better name for it), it should be updated
to use OidString and just be a general impl class for handling ASN AlgorithmId
structure. Let me try putting this utility method name2oidStr() elsewhere and
see.
computeOidTable() is only called by name2oid() only called by name2oidStr()
only called by algOID() which is in AlgorithmId.
algOID() in AlgorithmId is called by AlgorithmId::get which makes it quite the
core of that class.
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?
Yes, initially I was relying on the ordering of enums to handle the 1-to-N
mapping. Later, I changed to explicitly define the mapping as in the webrev as
this should be more robust. I should have added other algorithms here too. Will
update.
I caught them by looking at the "skipped" debug output. Can we add some logic
to detect this? For example, for those nonpreferred OIDs, use a special constructor?
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.
For other lines => are you referring to the extra aliases?
Oh, I meant lines 415 - 428.
Instead of specifying the aliases for each enum explicitly as in current
webrev, store them into the constructor and save them into the name2enum map
while looping through the enum values? The current approach has the benefit of
being straightforward and no need to worry about duplicate aliases.
We can also populate the name2enum map inside the constructor, there is no need
to store the aliases in each enum.
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?
OID is tied to an algorithm name. Thus, it should be universal to all JDK
providers as long as the algorithm impl is interoperable. In the past, it's
very easy to miss adding the OIDs as they are hardcoded in different places.
For providers who are lower in the provider list, registering the OIDs may not
be that important if a more preferred provider already supports the same impl.
Rather than registering identical entries, with this change, the OID support
should be consistent across providers.
Good to know.
5. I found KnownOIDs to be a better class name.
Sure, fine with me.
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.
Yes, I debated about it. It's tricky to draw the line. My current thought is to keep
these "public static" constants intact if they are directly referenced
somewhere. There are also many places where I think further trimming/cleanup can be made.
But as it is, it is already extensive. Maybe it's safer to be conservative and gradually
clean up...
Fair enough.
If we want to eliminate the use of these objects later, is there a way to not
create a new ObjectIdentifier object each time? I think I've read some C
projects that defines OID as the DER encoding but not as strings. String is
only best for log usages. But this is a much broader topic.
I haven't read other files yet. Will send more comment later.
Sure, these two are the key files.
I will experiment on the utility method and we can discuss it further.
Thanks,
Max
Thanks,
Valerie
Thanks,
Max
On Apr 24, 2020, at 7:11 AM, Valerie Peng <[email protected]> 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
