Repository navigation
Conversation
madislm
changed the title
5a45d6d to
56d68ae
Compare
Signed-off-by: Madis Jaagup Laurson <madisjaagup.laurson@nortal.com>
b313adf to
1a97a19
Compare
| LOG.debug("OCSP response received successfully"); | ||
|
|
||
| verifyOcspResponse(basicResponse, ocspService, certificateId, issuerCertificate, maxOcspResponseThisUpdateAge); | ||
| verifyOcspResponse(basicResponse, ocspService, certificateId, issuerCertificate); |
There was a problem hiding this comment.
We should run checkNonce() before verifyOcspResponse(), because in case of unknown certificate status verification throws before reaching the nonce check. In this case a replayed, otherwise valid response with a mismatched nonce becomes the final result and prevents fallback.
Also, checking the nonce first avoids unnecessary validation on mismatch. RFC 6960 allows checking the nonce first, it specifies acceptance requirements without prescribing their execution order.
I suggest to simply move verifyOcspResponse() below the if (ocspService.doesSupportNonce()) block.
| LOG.debug("OCSP response received successfully"); | ||
|
|
||
| verifyOcspResponse(basicResponse, ocspService, certificateId, issuerCertificate, maxOcspResponseThisUpdateAge); | ||
| verifyOcspResponse(basicResponse, ocspService, certificateId, issuerCertificate); |
There was a problem hiding this comment.
As with ResilientOcspCertificateRevocationChecker, we should run checkNonce() before verifyOcspResponse(), as checking the nonce first avoids unnecessary validation on a mismatch.
As with ResilientOcspCertificateRevocationChecker, I suggest to simply move verifyOcspResponse() below the if (ocspService.doesSupportNonce()) block.
| // Clear any recordExceptions list of the given configuration first, because it is combined | ||
| // with the predicate below by OR and would otherwise widen what counts as a failure. | ||
| .recordExceptions() | ||
| .recordException(throwable -> throwable instanceof ResilientUserCertificateOCSPCheckFailedException) |
There was a problem hiding this comment.
Should we explicitly ignore UserCertificateOCSPException with ignoreException() here?
I put the circuit into HALF_OPEN so that one successful check would close it, then forced OcspNonceExtension.create() to throw an IOException. No OCSP request was sent, but the circuit closed as if the responder had recovered. As I understand it, the local failure was counted as a successful responder check: recordException() returned false, which means success unless the exception is explicitly ignored.
As I understand it, using ignoreException() would count this error as neither success nor failure, so the circuit would remain HALF_OPEN and ready for a real responder check. I tested that using ignoreException() fixed the problem.
Ignoring UserCertificateOCSPException leaves ResilientUserCertificateRevokedException and ResilientUserCertificateUnknownStatusException counted as circuit-breaker successes, as they do not inherit from UserCertificateOCSPException.
This is a minor defensive hardening issue, I did not identify a realistic case that would trigger this in production.
If you think this is relevant, please verify this independently with a test first, I may be mistaken here. Also, feel free to acknowledge and disregard this as not relevant.
| return RetryConfig.from(retryConfig) | ||
| // Users must not be able to modify this value. | ||
| .ignoreExceptions(ResilientUserCertificateRevokedException.class) | ||
| .ignoreExceptions(ResilientUserCertificateRevokedException.class, ResilientUserCertificateUnknownStatusException.class) |
There was a problem hiding this comment.
Related to the getCircuitBreakerConfig() comment, could we also add UserCertificateOCSPException to ignoreExceptions() here? In the same test described in the getCircuitBreakerConfig() comment, nonce construction was attempted 3 times without sending any OCSP requests, causing unnecessary retries. These retries were not really checking the responder’s availability as intended and if retry delays are configured would make the caller wait longer for the same local failure.
This is a minor issue, feel free to acknowledge and disregard this as not relevant.
|
|
||
| private static RetryConfig getRetryConfig(RetryConfig retryConfig) { | ||
| return RetryConfig.from(retryConfig) | ||
| // Users must not be able to modify this value. |
There was a problem hiding this comment.
I'm not sure if it is good to completely overwrite caller-configured ignored exceptions. We could preserve the caller’s ignored exceptions, but still ensure that revoked and final unknown are never retried.
This is a minor issue, feel free to acknowledge and disregard this as not relevant.
AUT-2981
Signed-off-by: Madis Jaagup Laurson <madisjaagup.laurson@nortal.com>