HTTP-119 - Fix bug where default Java TrustStore was not used when no cert-related properties were used. #123
Conversation
… cert-related properties were used. Signed-off-by: Krzysztof Chmielewski <[email protected]>
|
@grzegorz8 @davidradl could you take a look? @davidradl I Think this should solve the problem you reported. |
|
| @@ -18,6 +18,24 @@ | |||
|
|
|||
| public class ElasticSearchLiteQueryCreatorTest { | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
I think these new tests are not related to the issue HTTP-119? Could you please add them in a separate PR?
There was a problem hiding this comment.
Agreed - it would be good to have unit tests to test the various code paths around this change.
There was a problem hiding this comment.
The "new code" is already partially covered by existing tests, but I will try to add new ones to cover it fully.
The reason why I added those new tests was that we dropped with branch coverage below 90% and while inspecting test report I have found those two.
I will like to leave those tests in this PR, unless there will be a strong NO to it. The PR is small so IMO having those extra, unrelated tests is ok.
There was a problem hiding this comment.
@kristoffSC fine my me too - thanks for putting this together.
| && !selfSignedCert | ||
| && serverTrustedCerts.length == 0 | ||
| && StringUtils.isNullOrWhitespaceOnly(clientCert) | ||
| && StringUtils.isNullOrWhitespaceOnly(clientPrivateKey)) { |
There was a problem hiding this comment.
@kristoffSC What happens if we only have client certs. In this case, we want the trusted public server certs and the client certs. The current fix does not deal with that combination. Can we get hold of the default public server certs and add them like securityContext.addCertToTrustStore(cert); then process the client certs?
| } | ||
| } | ||
|
|
||
| SecurityContext securityContext = createSecurityContext(properties); |
There was a problem hiding this comment.
@kristoffSC Talking with our team around this, in addition to the above. It would be good to have the ability to:
1- run with public certs and any supplied private server certs. i.e. merge the supplied certs with existing public ones.
2- run only with private server certs (which is the current functionality when they are supplied in the config)
This would require a new config option maybe 'gid.connector.http.security.cert.server.includeDefaultCerts' with a default of true.
WDYT?
|
@kristoffSC Friendly nudge. I assume this one is going to be processed before #117. We are wanting to put 117 in, we have a circumvention for the public certs , as we can just supply them. |
Krzysztof seems to be less available recently. @davidradl Could you take over the PR and apply your suggestions? Or rather create a new PR. I'll be happy to review it. |
That sounds good. I will take this one over - unless I hear otherwise form @kristoffSC . I will do #117 first. I hope to look at both of these next week. |
|
@grzegorz8 @kristoffSC looking into this. |
|
@kristoffSC @grzegorz8 I have done quite a lot of testing and asking round our team what our requirements are. It looks like the best option for us is actually @kristoffSC s fix . I need to slightly amend it as it does not quite work as expected - due to the way serverTrustedCerts is handled- it defaults to "" which end up as an array of 1 "" element, so the default is not obtained when expected. I will amend the fix and put up for review. |
|
@kristoffSC @grzegorz8 I have coded this in #128. Sorry I did not manage to keep you original commit @kristoffSC -f this is an issue I can redo the pr - if not are we OK to close this PR? |
Description
Fix bug where default Java TrustStore was not used when no cert-related properties were used.
Resolves Public certificates should not have to be supplied as they should be picked up from the jvm
PR Checklist