Use the correct alert depending on the CertificateException when usin…#6046
Use the correct alert depending on the CertificateException when usin…#6046normanmaurer wants to merge 1 commit into
Conversation
|
@Scottmitch PTAL |
…g OpenSslEngine Motivation: We tried to detect the correct alert to use depending on the CertificateException that is thrown by the TrustManager. This not worked all the time as depending on the TrustManager implementation it may also wrap a CertPathValidatorException. Modification: - Try to unwrap the CertificateException if needed and detect the right alert via the CertPathValidatorException. - Add unit to verify Result: Send the correct alert depending on the CertificateException when using OpenSslEngine.
normanmaurer
force-pushed
the
ssl_alert
branch
from
1141be1 to
c8efc04
Compare
| } | ||
| } | ||
|
|
||
| // Its a bit hacky to verify against the message that is part of the exception but there is no other way |
There was a problem hiding this comment.
agreed it is hacky ... maybe we can just validate the exception type for now and enhance later if JDK provides us with more granular exceptions, or a more reliable error code indicator?
There was a problem hiding this comment.
Nope as the exception type was the same before, so let us keep it
There was a problem hiding this comment.
this test may be JDK dependent though right? Also what about exception msg localization?
There was a problem hiding this comment.
Yeah... maybe just use assumeTrue?
There was a problem hiding this comment.
http://junit.sourceforge.net/javadoc/org/junit/Assume.html
A set of methods useful for stating assumptions about the conditions in which a test is meaningful. A failed assumption does not mean the code is broken, but that the test provides no useful information.
The test is still useful if we can't verify the exception text ... just that we can't verify the conditions at the granularity that we would like. If the text doesn't match we really don't know what is going on (e.g. text changed but still same logical condition). If its not reliable maybe we should just not check for it at all. I have also checked exception text in the past bcz of lack of more expressive exceptions ... but its not portable and I wouldn't mind taking it out now.
There was a problem hiding this comment.
I am on the fence here... as without the message check it passes also without the fix
There was a problem hiding this comment.
Ok your call. Unfortunate that we don't have a more reliable way to verify. Can we get the alert code from an OpenSSL API maybe?
There was a problem hiding this comment.
@Scottmitch let me just pull it in as it is then... there is no way atm.
2 participants
…g OpenSslEngine
Motivation:
We tried to detect the correct alert to use depending on the CertificateException that is thrown by the TrustManager. This not worked all the time as depending on the TrustManager implementation it may also wrap a CertPathValidatorException.
Modification:
Result:
Send the correct alert depending on the CertificateException when using OpenSslEngine.