Replace the reflective revocation checker with the JDK's PKIX checker - #1948
Draft
kevinherron wants to merge 4 commits into
Draft
Replace the reflective revocation checker with the JDK's PKIX checker#1948kevinherron wants to merge 4 commits into
kevinherron wants to merge 4 commits into
Conversation
OpcUaCertificateRevocationChecker subclassed the JDK's internal RevocationChecker through reflection, so it only worked when the JVM was started with --add-opens java.base/sun.security.provider.certpath. Without that flag validation silently fell back to the default checker, which ignored REVOCATION_LISTS and, until recently, could accept a certificate that a supplied CRL listed as revoked. Configure the public PKIXRevocationChecker instead. It evaluates the trust list's CRLs only (PREFER_CRLS, NO_FALLBACK) and is isolated from the JVM-wide ocsp.* security properties another component may set, which the JDK otherwise reads even in CRL-only mode. A revocation established by an applicable CRL is always enforced. REVOCATION_LISTS decides whether an unknown status is rejected or tolerated; tolerated results are read back from the checker's soft-fail list and logged with the certificate, issuer, and reason. REVOCATION is deprecated for removal because it no longer has an effect. The JDK checker fetches from a certificate's CRL distribution point when the supplied CRLs do not cover its issuer, and the public API offers no way to turn that off. Document the timeouts and the event-loop cost rather than hide it. Remove the never-implemented OpcUaCertPathValidator and the unused OpcUaCertificateValidityChecker, the --add-opens surefire argument, and the sun.security OSGi import that only existed for the reflective access.
The PKIX path builder rejects a certificate outside its validity period before validation runs, and its exception does not say which rule failed, so an expired CA-issued certificate surfaced as the generic Bad_SecurityChecksFailed. Check the presented chain's validity when path building fails so the peer receives the Part 4 6.1.3 status code and can tell a renewal is needed.
Add a migration section covering the fixed meaning of the revocation flags, the deprecation of ValidationCheck.REVOCATION, the removed --add-opens requirement, the distribution-point retrieval the JDK checker performs when a CRL is missing, and the one deployment shape that changes behavior on upgrade.
Allow unknown revocation status in fixtures without CRLs so validation reaches the compatibility check. Assert Bad_CertificateUseNotAllowed instead of accepting any UaException.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Revocation checking in
CertificateValidationUtilrelied onOpcUaCertificateRevocationChecker, a reflective subclass of the JDK's internalRevocationChecker. It only worked when the JVM was started with--add-opens java.base/sun.security.provider.certpath=ALL-UNNAMED. Without that flag, validation silently fell back to the default checker,REVOCATION_LISTShad no effect, and until #1944 a certificate that a supplied CRL listed as revoked could be accepted.This replaces the reflective checker with the public
PKIXRevocationCheckerAPI. The checker is configured for CRLs only (PREFER_CRLS,NO_FALLBACK), fed the trust list's CRLs after the existing same-name issuer selection, and isolated from the JVM-wideocsp.*security properties the JDK otherwise reads even in CRL-only mode. A revocation established by an applicable CRL is always enforced.REVOCATION_LISTSnow decides only whether an unknown status is rejected or tolerated; tolerated results are read back from the checker's soft-fail list and logged atWARNwith the certificate, issuer, and reason.ValidationCheck.REVOCATIONis deprecated for removal since it no longer has an effect.One consequence worth knowing about: when the supplied CRLs do not cover an issuer, the JDK checker fetches from a CRL distribution point named in the certificate before treating the status as unknown, and the public API offers no way to turn that off. It runs on the validating thread (the channel's event loop) and waits up to the
com.sun.security.crl.timeoutandcrl.readtimeoutdefaults per distribution point when the host is unreachable. The package docs and the 1.2.0 migration guide describe this and the mitigation (supply the issuer's CRL through the trust list). If you would rather keep the 1.1.x no-network default, the only option is to skip the checker under the tolerant policy when no applicable CRL exists, which I did not do here.A second commit fixes a related reporting gap: the PKIX path builder rejects an expired CA-issued certificate before validation runs, and that surfaced as
Bad_SecurityChecksFailed.buildCertPathnow checks the presented chain's validity on builder failure so the peer receivesBad_CertificateTimeInvalid(or the issuer variant). This is an observable status code change.The never-implemented
OpcUaCertPathValidator, the unusedOpcUaCertificateValidityChecker, the surefire--add-opensargument, and thesun.security.*OSGi import are removed. The migration guide gains a section on the fixed meaning of the revocation flags and the one deployment shape that changes behavior on upgrade.Verification:
spotless:checkandclean compile, plus the stack-core test classesCertificateRevocationTest(new, including a child-JVM test for theocsp.*isolation and a local distribution point server),CertificateValidationUtilTest,DefaultCertificateValidatorTest, andBrainpoolCertificateValidationTest. Not yet exercised against the CTT; that is planned separately before this leaves draft.