On Tue, 22 Nov 2022 21:43:42 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: > > more null checks and comments update Other than the comments about the test bugids, LGTM, if these are the last changes you are making. So I'm approving. test/jdk/sun/security/x509/Extensions/DefaultCriticality.java line 28: > 26: * @summary Change default criticality of policy mappings and policy > constraints > 27: certificate extensions > 28: * @bug 8059916 8296742 I don't think the bugid needs to be listed here. This has nothing to do with what is being tested, it is just a side-effect of the internal API behavior changing, and details can be seen in the commit log, if needed. test/jdk/sun/security/x509/X509CertImpl/V3Certificate.java line 26: > 24: /* > 25: * @test > 26: * @bug 8049237 8242151 8296742 I also don't think the bugid needs to be listed here, for same reason. ------------- Marked as reviewed by mullan (Reviewer). PR: https://git.openjdk.org/jdk/pull/11137
