[
https://issues.apache.org/jira/browse/QPID-8367?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17019452#comment-17019452
]
ASF GitHub Bot commented on QPID-8367:
--------------------------------------
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]
> [Broker-J] Trusted CA revocation list
> -------------------------------------
>
> Key: QPID-8367
> URL: https://issues.apache.org/jira/browse/QPID-8367
> Project: Qpid
> Issue Type: Improvement
> Components: Broker-J
> Reporter: Tomas Vavricka
> Priority: Major
> Fix For: qpid-java-broker-8.0.0
>
>
> Qpid Broker-J supports custom CA. When in place clients then can connect with
> certificate signed by custom CA.
> However there is no way to reject compromised certificates. Implementation of
> revocation list for custom CA can solve this issue.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
