Allow to verify if an error code is valid for CertificateVerifier#260
Allow to verify if an error code is valid for CertificateVerifier#260normanmaurer wants to merge 1 commit intomasterfrom
Conversation
|
@Scottmitch PTAL... I will check if I can add a unit test for this easily here |
openssl-static/pom.xml
Outdated
| <maven.deploy.skip>true</maven.deploy.skip> | ||
|
|
||
| <!-- Skip tests for now as there is something wrong with the static build for openssl when running on macOS --> | ||
| <skipTests>true</skipTests> |
There was a problem hiding this comment.
@Scottmitch I will investigate in this as a separate issue. Not super urgent tho as we not publish static builds for openssl anyway
| </activation> | ||
| <properties> | ||
| <!-- Skip tests on windows as its not easy to load the openssl library we link against in the tests --> | ||
| <skipTests>true</skipTests> |
There was a problem hiding this comment.
@Scottmitch there may be a way to fix this, but I am a bit short on time and would love to merge this first. We can do a followup if needed
Scottmitch
left a comment
There was a problem hiding this comment.
2 comments on the unit test then lgtm
| do { | ||
| errorCode++; | ||
| Assert.assertTrue(errorCode < Integer.MAX_VALUE); | ||
| } while (CertificateVerifier.isValid(errorCode)); |
There was a problem hiding this comment.
this seems like a roundabout way to do:
assertFalse(CertificateVerifier.isValid(Integer.MIN_VALUE));Can we simplify this test?
There was a problem hiding this comment.
sure... just let us hope this one will never be valid ;)
There was a problem hiding this comment.
What is the intention of this test? To prove that at least 1 negative value is invalid? Seems strange in general. If we are just trying to demonstrate there is at least 1 error code that is not valid we have a few options:
- create some private markers to indicate "highest" and "lowest" valid value and use these in the test. Negative of this approach is we have to worry about updating these just for testing.
- make the set package private, sort all the values, and pick a value not in the set.
- randomly choose values until we find one which is not in the set and make sure its not valid.
There was a problem hiding this comment.
yeah that it not always return true. Maybe using Integer.MIN_VALUE is good enough as you said.
| for (Field field : fields) { | ||
| if (field.isAccessible()) { | ||
| int errorCode = field.getInt(null); | ||
| Assert.assertTrue(CertificateVerifier.isValid(errorCode)); |
There was a problem hiding this comment.
add a string description which shows what number errorCode is missing.
| Set<Integer> errors = new HashSet<Integer>(); | ||
| errors.add(X509_V_OK); | ||
| errors.add(X509_V_ERR_UNSPECIFIED); | ||
| errors.add(X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT); |
There was a problem hiding this comment.
How about just using reflection to get the fields start with X509_V_ERR (and X509_V_OK)?
There was a problem hiding this comment.
@trustin I was considering this but did not do because of jdk9 and secruitymanagers. Reflection may be disallowed. This is why I added the test-case which will fail if we miss to at stuff to the map.
There was a problem hiding this comment.
jdk9 and secruitymanagers
Really? I thought there's no security restriction on accessing the fields that belong to itself reflectively?
There was a problem hiding this comment.
I am not sure :) Better be safe then sorry and like I said we will catch it with the unit test anyway.
There was a problem hiding this comment.
I'm fine with that although I still believe it's gonna be fine. :-)
Motivation: CertificateVerifier allows to return an errorCode. We should have a static method to validate that the error code that may be returned is valid. Modifications: - Make CertificateVerifier an abstract class and add method to check if errorCode is valid. Result: Easier way to validate if errorCode is valid.
|
Cherry-picked into master after addressing @Scottmitch s comments |
Motivation:
CertificateVerifier allows to return an errorCode. We should have a static method to validate that the error code that may be returned is valid.
Modifications:
Result:
Easier way to validate if errorCode is valid.