alex-rufous commented on issue #44: QPID-8367 [Broker-J] Trusted CA revocation list URL: https://github.com/apache/qpid-broker-j/pull/44#issuecomment-576264339 Hi Tomas, I looked through the proposed changes. Please find below my review comments. I like the idea of generation test resources on running the tests rather than having them pre-created in advance and regenerating CA, certificates, keystores, truststores, etc periodically. In fact, I started to use in my recent tests `SSLUtil#generateSelfSignedCertificate` for certificate generation in order to avoid relying on static test resources. My intention was to replace keystore/truststore test resources with auto-generated ones where it is possible. Though, java API does not allow to generate all required test resources programmatically. Thus, the external utilities like openssl or NSS certutil has to be used. I am OK with your changes to switch from NSS certutil to openssl. Though, I still would like to keep the ability to run the unit and system tests on Windows platforms. Thus, I think, that we need to check the platform/environment and skip the tests relying on certificate generation with openssl. Potentially, we can try running the generation script and on successful execution we can write the generation status in some status file. When generation is not successful, the unit and system tests can check the file and set the flag to skip tests relying on generated resources. What do you think about it? I have not run the tests with the changes. How long does it take to generate the test resources with openssl? If it a matter of seconds, that should be acceptable. Otherwise, we might need a mechanism to reduce the number of ssl resource generations. Taking that you started running certificate generation script from qpid-test-utils/pom.xml, I think it make sense to move all required ssl test resources (including generation script) into `qpid-test-utils` and drop test_resources/ssl in order to have test dependencies in one module. Potentially, the test certificate, keystores, truststores can be generated in corresponding module "target" folder and the file locations can be either relative to target folder or exposed to the unit/system tests in some way (for example, via status file). The script is executed in maven phase `generate-resources`. I believe, that the phase should be changed to `generate-test-resources`. As for the implementation changes, please find my code review comments below. The self descriptive attribute names are preferred over abbreviations and acronyms. Thus, I would suggest to rename the new attributes as follows: - revocation -> certificateRevocationCheckEnabled - crlUrl -> certificateRevocationListUrl - onlyEndEntity -> certificateRevocationCheckOfEndEntityCertificates or checkRevocationStatusOfEndEntityCertificates - preferCrls -> preferCertificateRevocationList[ForCertificateRevocationCheck] (I am not sure about this name) - noFallback -> certificateRevocationCheckWithNoFallback - softFail -> certificateRevocationCheckWithIgnoringSoftFailures Please feel free to change the attribute name to more user friendlier names. I am not sure that my names suggested above are that good. The crlUrl seems only allows file URLs. Why data URLs are not allowed? Is it due to security consideration? Could you please expand on it? AuthenticationTest comments: AuthenticationTest uses "com.sun.net.httpserver.HttpServer". Why jetty http server cannot be used instead? I am not big fan of hardcoding of the test ports, as they could be occupied on the environment where tests are run, but, I see the reason why it was done. Perhaps, in order to avoid seeing failed tests due to port being occupied by other process, I might make sense to skip the test if http server fails to bind to the pre-defined port. Another way to workaround this issue is by starting http server first, and than generating the ssl test resources against the http server port. For example, you can implement "org.junit.rules.ExternalResource" where you can start/stop the http server and execute the script after server startup by using java API `java.lang.ProcessBuilder`. That would eliminate the need to execute the generation script from maven. You can have a look into implementation of `EmbeddedKdcResource` which I added for KerberosAuthenticationManagerTest, etc. A new base class KeyStoreTestBase does not bring much value. I think it would be better to introduce utility class KeyStoreTestUtils with some static methods like checkExceptionThrownDuringKeyStoreCreation and call the utility static methods from other test classes. TlsTest & TestSSLConstants Unix file separator is explicitly hard-coded into test constant. I prefer environment neutral implementation. Are you planning to add documentation for the new functionality into Qpid broker docbook module? Kind Regards, Alex
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
