RFR: 8296742: Illegal X509 Extension should not be created

[Weijun Wang]

On Mon, 14 Nov 2022 16:47:22 GMT, Weijun Wang <weijun at 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.

src/java.base/share/classes/sun/security/x509/NameConstraintsExtension.java line 351:

> 349:         if (updated) {
> 350:             encodeThis();
> 351:         }

This is the only extension where `encodeThis()` might be called (and thus set `extensionVaue` to null) even if the extension is created from an encoding. The `updated` flag is added to make sure the re-encoding is only done when there is a real modification. In this case, at least one of `permitted` and `excluded` is not null and a non-null encoding can be calculated.

In fact, because of this `merge` method, this is also the only extension that cannot be easily rewritten into an immutable class. This can be considered in a future enhancement.

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

PR: https://git.openjdk.org/jdk/pull/11137