RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]

[Michael Osipov]

On Tue, 15 Feb 2022 15:40:42 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> src/java.base/share/classes/sun/security/x509/OtherName.java line 93:
>> 
>>> 91:         oid = in.getOID();
>>> 92:         DerValue derValue1 = in.getDerValue();
>>> 93:         if (!derValue1.isContextSpecific((byte)0) || !derValue1.isConstructed()) {
>> 
>> It might be purely a matter of taste, but isn't `!(isCSTag0 && isConst)` easier to read?
>
> I have difficulty describing `!(a && b)`. There is no parentheses in human language and `!` has higher order than `&&`.
> 
> I thought about completely reverse the block but that means everything after the throw will be inside a block and I don't want to move so many lines.

My wording for the &&: If the tag is not a constructed and context-specific tag number 0, then an exception is thrown. The parens denote that both conditions need to apply:

!(isCSTag0 && isConst)

true, true = !(true && true) = !true = false
true, false = !(true && false) = !false = true
false, true = !(false && true) = !false = true
false, false = !(false && false) = !false = true


!isCSTag0 || !isConst

true, true = !true || !true = false || false = false
true, false = !true || !false = false || true = true
false, true = !false || !true = true || false = true
false, false = !false || !false = true || true = true

>> src/java.base/share/classes/sun/security/x509/X509CertImpl.java line 1594:
>> 
>>> 1592:                         String v = new DerValue(nameValue).getAsString();
>>> 1593:                         nameEntry.add(v == null ? nameValue : v);
>>> 1594:                     } catch (IOException ioe) {
>> 
>> Attention, this catch block will hide invalid ASN.1 encoding of the other name element from:
>> * sun.security.util.DerValue.init(boolean, InputStream, boolean)
>> * sun.security.util.DerValue.getIA5String()
>> 
>> Other blocks throw:
>> 
>> throw new CertificateException("Unable to parse DER value of SAN:otherName", ioe);
>> 
>> 
>> Do you really intend to hide an encoding error int the cert from the user?
>
> Up to debate. Other blocks in `makeAltNames` throw `RuntimeException`.

Correct, but they don't swallow at least.

>> test/jdk/sun/security/x509/OtherName/Parse.java line 89:
>> 
>>> 87:         int found = 0;
>>> 88:         for (var san : x.getSubjectAlternativeNames()) {
>>> 89:             if (san.size() == 4) {
>> 
>> Make it `>=` if this is going to change in the future..
>
> OK, but this is always implementation-specific. Precisely, it's a "may" so I should not check the count.

Right, let's leave as-is.

-------------

PR: https://git.openjdk.java.net/jdk/pull/7167