Whoops, forgot to look at RFC 6961. Over there, both revoked and
structural failures are both supposed to yield
bad_certificate_status_response alerts. I think what we have is
conformant with the spec.
--Jamil
On 2/14/2019 11:17 AM, Jamil Nimeh wrote:
On 2/14/2019 10:24 AM, Sean Mullan wrote:
On 2/11/19 2:32 PM, Xuelei Fan wrote:
Hi,
Could I get the update reviewed?
http://cr.openjdk.java.net/~xuelei/4919790/webrev.00/
721 alert = Alert.UNSUPPORTED_CERTIFCATE;
Can we fix this typo while we are cleaning this up?
s/CERTIFCATE/CERTIFICATE/
Also, I was a bit curious about these lines (not part of your fix):
711 if (reason == BasicReason.REVOKED) {
712 alert = chc.staplingActive ?
713 Alert.BAD_CERT_STATUS_RESPONSE :
714 Alert.CERTIFICATE_REVOKED;
If a certificate is revoked, why would you set the alert status to
BAD_CERT_STATUS_RESPONSE if stapling is enabled?
I think that was something I did back when we did OCSP stapling in JDK
9. IIRC, RFC 6066 says that clients receiving stapled responses must
check them and alert with bad_certificate_status_response "if the
response is not satisfactory" (their words, not mine). Back then I
interpreted "unsatisfactory" to mean both structurally wrong or
revoked. Perhaps that was too broad an interpretation.
It's probably worth seeing how a couple other clients handle this
case. I know I had tested with them back in pre-JDK9 days, but I
don't recall the results. I might have some old packet captures lying
about still also, I just need to dig them up.
Also, bug needs a noreg label.
--Sean
It had been a while that the SunJSSE provider use
certificate_unknown or certificate_revoked (or
bad_certificate_status_response for OCSP stapling) as the
certificate issues alert. Other certificate alert like
certificate_expired are not used.
The bug was reported in JDK 6. With the introducing of
CertPathValidatorException.BasicReason in JDK 7. Now we can handle
the alert more accuracy.
Note: please don't rely on the certificate alert type for
application development. The alert type may be changed and
different per the provider preference.
No new regression test as the update is simple and straightforward.
Thanks,
Xuelei
