Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
OtherName.java @93,97
PR:https://git.openjdk.java.net/jdk/pull/7167
if (derValue1.isContextSpecific((byte) 0) &&
derValue1.isConstructed()) {
nameValue = derValue1.data.toByteArray();
} else {
throw new IOException("value is not [0]");
}
That exception string isn't correct (the value is usually not just the
tag), nor very descriptive. How about instead:
throw new IOException ("value is not EXPLICTly tagged [0]");
Also, the derValue1.data should be parseable into a DerValue itself.
Should that be checked here as well and an error thrown if invalid?
I.e., add this after nameValue = ...
try {
new DerValue (nameValue);
} catch (IOException ex) {
throw new IOException ("Body of OtherName is not a valid BER or
DER value", ex);
}
Thanks - Mike
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Fri, 18 Feb 2022 16:28:25 GMT, Weijun Wang wrote: >> While I understand that, `new DerValue(byte[])` will be ignored and this >> will be also inconsistent with the remaining general names. Looking at >> sun.security.x509.GeneralName.GeneralName(DerValue, boolean) they all throw >> `IOException`. > > For other known names, you either return a parsed name or fail. For > `otherName`, you already have the raw data and this further parsed name is a > bonus. I just don't like a problem in getting the bonus to ruin the original > benefit. Also, I think the caller will have to check the length and the type > anyway so this will not be an extra burden. Besides, I always feel that > `otherName` could be freely extended and the quality of its encoding might > not be as guaranteed as the other ones. @seanjmullan, any comment here? > > Too many "other" used and hopefully no one get confused. This seems like a reasonable balance between preserving existing behavior and still providing the data for the application to inspect. To help debugging, you could log the exception. - PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Fri, 18 Feb 2022 15:17:30 GMT, Michael Osipov wrote: >> I want to minimize behavior change and I'm leaving them to notice there's no >> string there and fail. > > While I understand that, `new DerValue(byte[])` will be ignored and this will > be also inconsistent with the remaining general names. Looking at > sun.security.x509.GeneralName.GeneralName(DerValue, boolean) they all throw > `IOException`. For other known names, you either return a parsed name or fail. For `otherName`, you already have the raw data and this further parsed name is a bonus. I just don't like a problem in getting the bonus to ruin the original benefit. Also, I think the caller will have to check the length and the type anyway so this will not be an extra burden. Besides, I always feel that `otherName` could be freely extended and the quality of its encoding might not be as guaranteed as the other ones. @seanjmullan, any comment here? Too many "other" used and hopefully no one get confused. - PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Fri, 18 Feb 2022 15:06:07 GMT, Weijun Wang wrote: >> So you leave it to the user to read the ASN.1 value and fail if the encoding >> is incorrect instead of throwing a `Ceritificate..Exception` although other >> GeneralNames do? > > I want to minimize behavior change and I'm leaving them to notice there's no > string there and fail. While I understand that, `new DerValue(byte[])` will be ignored and this will be inconsistent with the remaining general names. Looking at sun.security.x509.GeneralName.GeneralName(DerValue, boolean) they all throw `IOException`. - PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Fri, 18 Feb 2022 06:56:04 GMT, Michael Osipov wrote: >> Yes. I don't want to let the method fail. Since `instanceof String` should >> be called anyway the caller can decide how to fail. > > So you leave it to the user to read the ASN.1 value and fail if the encoding > is incorrect instead of throwing a `Ceritificate..Exception` although other > GeneralNames do? I want to minimize behavior change and I'm leaving them to notice there's no string there and fail. - PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Thu, 17 Feb 2022 20:41:50 GMT, Weijun Wang wrote: >> @wangweij So you have selected to really swallow the exception here? > > Yes. I don't want to let the method fail. Since `instanceof String` should be > called anyway the caller can decide how to fail. So you leave it to the user to read the ASN.1 value and fail if the encoding is incorrect instead of throwing a `Ceritificate..Exception` although other GeneralNames do? - PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Thu, 17 Feb 2022 19:42:30 GMT, Michael Osipov wrote: >> But in this case, we still have a place to provide the raw bytes. Maybe >> that's better? Or you'd rather be guaranteed that one particular otherName >> should always have a string there and there's no need to do an `instanceof` >> check? What if the tag is already wrong and I don't know it should be a >> string? > > @wangweij So you have selected to really swallow the exception here? Yes. I don't want to let the method fail. Since `instanceof String` should be called anyway the caller can decide how to fail. - PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 15:55:50 GMT, Weijun Wang wrote: >> Correct, but they don't swallow at least. > > But in this case, we still have a place to provide the raw bytes. Maybe > that's better? Or you'd rather be guaranteed that one particular otherName > should always have a string there and there's no need to do an `instanceof` > check? What if the tag is already wrong and I don't know it should be a > string? @wangweij So you have selected to really swallow the exception here? - PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 15:16:58 GMT, Weijun Wang wrote: >> The enhancement adds two extra items in the `getSubjectAlternativeNames()` >> output for an OtherName. >> >> It also fix several errors: >> 1. In `OtherName.java`, `nameValue` should be the value inside `CONTEXT [0]` >> without the tag and length bytes. >> 2. The argument in constructor `extClass.getConstructor(Object.class)` is >> suspicious. Maybe it meant `byte[]`. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > string at 4th place src/java.base/share/classes/java/security/cert/X509Certificate.java line 600: > 598: * defined for otherNames, X.400 names, EDI party names, or any > 599: * other type of names. They are returned as byte arrays > 600: * containing the ASN.1 DER encoded form of the name. I would move this sentence up before "otherNames ..." and remove the "otherNames" from it and the end of the sentence. So basically: > * No standard string format is > * defined for X.400 names or EDI party names. They are returned as byte > arrays > * containing the ASN.1 DER encoded form of the name. > * otherNames are returned as a byte array ... - PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 16:22:17 GMT, Weijun Wang wrote: > So my current opinion is to make it as vague as possible (Ex: `a valid > supported character string`) so people always remember to check `instanceof > String` first. I agree with that, but consider https://github.com/openjdk/jdk/pull/7167#discussion_r807016503 also. - PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 15:16:58 GMT, Weijun Wang wrote: >> The enhancement adds two extra items in the `getSubjectAlternativeNames()` >> output for an OtherName. >> >> It also fix several errors: >> 1. In `OtherName.java`, `nameValue` should be the value inside `CONTEXT [0]` >> without the tag and length bytes. >> 2. The argument in constructor `extClass.getConstructor(Object.class)` is >> suspicious. Maybe it meant `byte[]`. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > string at 4th place So my current opinion is to make it as vague as possible (Ex: `a valid supported character string`) so people always remember to check `instanceof String` first. - PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 16:09:13 GMT, Michael Osipov wrote: >> But in this case, we still have a place to provide the raw bytes. Maybe >> that's better? Or you'd rather be guaranteed that one particular otherName >> should always have a string there and there's no need to do an `instanceof` >> check? What if the tag is already wrong and I don't know it should be a >> string? > > I have thought about this actually. Now the parse is free of any semantics, > which is naive. Actually, you need a list of wellknown OIDs to know the > target tag type to perform the conversion. E.g., you know that MS UPN must be > UTF8String, if not this is an error. If you don't know the OID, don't touch > it. > OpenSSL knows the semantics and decodes it, otherwise don't touch it and > leave it: > https://github.com/openssl/openssl/blob/317acac5cc0a2cb31bc4b91353c2b752a3989d8a/crypto/x509/v3_san.c#L113-L120 Maybe adopt the list of OpenSSL, otherwise return byte array? - PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 15:55:50 GMT, Weijun Wang wrote: >> Correct, but they don't swallow at least. > > But in this case, we still have a place to provide the raw bytes. Maybe > that's better? Or you'd rather be guaranteed that one particular otherName > should always have a string there and there's no need to do an `instanceof` > check? What if the tag is already wrong and I don't know it should be a > string? I have thought about this actually. Now the parse is free of any semantics, which is naive. Actually, you need a list of wellknown OIDs to know the target tag type to perform the conversion. E.g., you know that MS UPN must be UTF8String, if not this is an error. If you don't know the OID, don't touch it. OpenSSL knows the semantics and decodes it, otherwise don't touch it and leave it: https://github.com/openssl/openssl/blob/317acac5cc0a2cb31bc4b91353c2b752a3989d8a/crypto/x509/v3_san.c#L113-L120 - PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 15:16:58 GMT, Weijun Wang wrote:
>> The enhancement adds two extra items in the `getSubjectAlternativeNames()`
>> output for an OtherName.
>>
>> It also fix several errors:
>> 1. In `OtherName.java`, `nameValue` should be the value inside `CONTEXT [0]`
>> without the tag and length bytes.
>> 2. The argument in constructor `extClass.getConstructor(Object.class)` is
>> suspicious. Maybe it meant `byte[]`.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> string at 4th place
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?
-
PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 15:46:10 GMT, Michael Osipov wrote:
>> 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
If you really like it, I'll write
if (derValue1.isContextSpecific((byte) 0) && derValue1.isConstructed())
{
nameValue = derValue1.data.toByteArray();
} else {
throw new IOException("value is not [0]");
}
Turns out I don't need to move all lines into the block.
>> Up to debate. Other blocks in `makeAltNames` throw `RuntimeException`.
>
> Correct, but they don't swallow at least.
But in this case, we still have a place to provide the raw bytes. Maybe that's
better? Or you'd rather be guaranteed that one particular otherName should
always have a string there and there's no need to do an `instanceof` check?
What if the tag is already wrong and I don't know it should be a string?
-
PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 15:48:17 GMT, Weijun Wang wrote:
>> 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
>
> If you really like it, I'll write
>
> if (derValue1.isContextSpecific((byte) 0) &&
> derValue1.isConstructed()) {
> nameValue = derValue1.data.toByteArray();
> } else {
> throw new IOException("value is not [0]");
> }
>
> Turns out I don't need to move all lines into the block.
Yes, that is fine also!
-
PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 15:40:42 GMT, Weijun Wang 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
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 15:28:29 GMT, Michael Osipov wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> string at 4th place
>
> 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.
> 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`.
-
PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 15:21:08 GMT, Michael Osipov wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> string at 4th place
>
> 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.
-
PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 15:16:58 GMT, Weijun Wang wrote:
>> The enhancement adds two extra items in the `getSubjectAlternativeNames()`
>> output for an OtherName.
>>
>> It also fix several errors:
>> 1. In `OtherName.java`, `nameValue` should be the value inside `CONTEXT [0]`
>> without the tag and length bytes.
>> 2. The argument in constructor `extClass.getConstructor(Object.class)` is
>> suspicious. Maybe it meant `byte[]`.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> string at 4th place
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?
-
PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
On Tue, 15 Feb 2022 15:16:58 GMT, Weijun Wang wrote:
>> The enhancement adds two extra items in the `getSubjectAlternativeNames()`
>> output for an OtherName.
>>
>> It also fix several errors:
>> 1. In `OtherName.java`, `nameValue` should be the value inside `CONTEXT [0]`
>> without the tag and length bytes.
>> 2. The argument in constructor `extClass.getConstructor(Object.class)` is
>> suspicious. Maybe it meant `byte[]`.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> string at 4th place
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..
-
PR: https://git.openjdk.java.net/jdk/pull/7167
Re: RFR: 8277976: Break up SEQUENCE in X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames in otherName [v5]
> The enhancement adds two extra items in the `getSubjectAlternativeNames()` > output for an OtherName. > > It also fix several errors: > 1. In `OtherName.java`, `nameValue` should be the value inside `CONTEXT [0]` > without the tag and length bytes. > 2. The argument in constructor `extClass.getConstructor(Object.class)` is > suspicious. Maybe it meant `byte[]`. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: string at 4th place - Changes: - all: https://git.openjdk.java.net/jdk/pull/7167/files - new: https://git.openjdk.java.net/jdk/pull/7167/files/bca0b6d3..1086d540 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7167=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7167=03-04 Stats: 27 lines in 4 files changed: 13 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/7167.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7167/head:pull/7167 PR: https://git.openjdk.java.net/jdk/pull/7167
