On Tue, 22 Nov 2022 17:42:06 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Inside JDK we support a lot of X.509 certificate extensions. Almost every
>> extension has a rule about what is legal or not. For example, the names in
>> `SubjectAlternativeNameExtension` cannot be missing or empty. Usually, a
>> rule is enforced in the `encode()` method, where the extension value is
>> assigned null for illegal extension and the method throws an `IOException`.
>> However, before the `encode()` method is called, the illegal extension can
>> always be created successfully, whether from a constructor using extension
>> components (For example, `new SubjectAlternativeNameExtension(names)`) or
>> using the encoded value (for example, `new
>> SubjectAlternativeNameExtension(derEncoding)`).
>>
>> This code change tries to prevent illegal extensions from being created
>> right from the beginning but the solution is not complete. Precisely, for
>> constructors using extension components, new checks are added to ensure the
>> correct components are provided and the extension can be encoded correctly.
>> Fortunately, most of these conditions are already met inside JDK calls to
>> them. The only exception is inside the `keytool -gencrl` command where the
>> reason code of a revoked certificate could be zero. This has been fixed in
>> this code change. There are some constructors having no arguments at all.
>> These are useless and also removed.
>>
>> On the other hand, constructors using the encoded value are complicated.
>> Some of them check for legal values, some do not. However, since the
>> encoding is read from the argument and already stored inside the object,
>> there is no need to calculate the encoding in the `encode()` method and this
>> method always succeed.
>>
>> In short, while we cannot ensure the extensions created are perfectly legal,
>> we ensure their `encode()` methods are always able to find a non-null
>> extension value to write out.
>>
>> More fine comments in the code change.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> IssuerAlternativeNameExtension names
More comments.
src/java.base/share/classes/sun/security/x509/CertificatePoliciesExtension.java
line 113:
> 111: public CertificatePoliciesExtension(Boolean critical,
> 112: List<PolicyInformation> certPolicies) throws IOException {
> 113: if (certPolicies == null || certPolicies.isEmpty()) {
You should probably also change `PolicyInformation(CertificatePolicyId
policyIdentifier, Set<PolicyQualifierInfo> policyQualifiers)` to
check for a null policyIdentifier and an empty policyQualifiers set.
src/java.base/share/classes/sun/security/x509/Extension.java line 178:
> 176:
> 177: Objects.requireNonNull(extensionId,
> 178: "Null OID to encode for the extension!");
I would probably drop the exclamation points. And change "Null OID" to "No OID".
src/java.base/share/classes/sun/security/x509/NameConstraintsExtension.java
line 142:
> 140: if (permitted == null && excluded == null) {
> 141: throw new IllegalArgumentException(
> 142: "permitted and exclude cannot both be null");
s/exclude/excluded/
src/java.base/share/classes/sun/security/x509/PolicyMappingsExtension.java line
78:
> 76: * Create a PolicyMappings with the List of CertificatePolicyMap.
> 77: *
> 78: * @param maps the List of CertificatePolicyMap, cannot be null or
> empty.
Do you want to add similar null checks to the `CertificatePolicyMap` ctor?
src/java.base/share/classes/sun/security/x509/SubjectAlternativeNameExtension.java
line 91:
> 89: */
> 90: public SubjectAlternativeNameExtension(Boolean critical, GeneralNames
> names)
> 91: throws IOException {
`GeneralNames` should probably also be modified so that it cannot contain an
empty or null List.
src/java.base/share/classes/sun/security/x509/SubjectInfoAccessExtension.java
line 89:
> 87: if (accessDescriptions == null || accessDescriptions.isEmpty()) {
> 88: throw new IllegalArgumentException(
> 89: "AccessDescription cannot be null or empty");
s/AccessDescription/accessDescriptions/
src/java.base/share/classes/sun/security/x509/SubjectKeyIdentifierExtension.java
line 76:
> 74: */
> 75: public SubjectKeyIdentifierExtension(byte[] octetString)
> 76: throws IOException {
Do you want to change the `KeyIdentifer` ctor to check for null instead of
letting it throw NPE?
-------------
PR: https://git.openjdk.org/jdk/pull/11137
