-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
ISSUE 608 : Support one way SSL in schema registry client. #609
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.... a few nits.
|
||
if (sslConfigurations.containsKey(SSL_TRUST_STORE_PATH)) { | ||
sslConfigurator.trustStoreType(sslConfigurations.get("trustStoreType")) | ||
.trustStoreFile(sslConfigurations.get("trustStorePath")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can replace this with SSL_TRUST_STORE_PATH
return Arrays.asList(SchemaRegistryTestProfileType.DEFAULT, SchemaRegistryTestProfileType.SSL); | ||
return Arrays.asList(SchemaRegistryTestProfileType.DEFAULT, | ||
SchemaRegistryTestProfileType.SSL, | ||
SchemaRegistryTestProfileType.SSL_WITH_SERVER_AUTH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: SSL_WITH_SERVER_AUTH sounds a bit confusing to me. Do you want to change to it to ONE_WAY_SSL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The profile I introduced requires server authentication but client doesn't and I wanted to add another profile where client requires authentication but server doesn't. If you think its ok to only have the former we can change it to ONE_WAY_SSL, if not I will add SSL_WITH_CLIENT_AUTH profile.
Fixes #608