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 A general comment is that since we are adding checks for illegal values to the `*Extension` classes, we should probably go one step further and do the same for all the classes in `sun.security.x509` package. I'm ok if you want to handle this as a separate issue though. ------------- PR: https://git.openjdk.org/jdk/pull/11137
