-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve user experience with SSL #698
Conversation
@@ -302,13 +312,14 @@ public void provideOutcome(AppCreator ctx) throws AppCreatorException { | |||
command.add("-J-D" + propertyName + "=" + propertyValue); | |||
} | |||
} | |||
|
|||
enableSslNative = properties.getProperty("shamrock.ssl.native") != null ? Boolean.parseBoolean(properties.getProperty("shamrock.ssl.native")) | |||
: false; |
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 configuration is pushed here via native-image.properties
.
|
||
import org.jboss.builder.item.MultiBuildItem; | ||
|
||
public final class ExtensionSslNativeSupportBuildItem extends MultiBuildItem { |
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 name could be improved, suggestions, welcome.
Obviously, if we end up using a separate extension to push that, we won't need the extension name anymore.
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.
This is fine for now.
poolConfiguration.connectionFactoryConfiguration().jdbcProperty("useSSL", "false"); | ||
break; | ||
default: | ||
log.warning("Agroal does not support disabling SSL for driver " + driver.getName()); |
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.
This class does not use JBoss Logging, we need to fix that. Will create a separate issue.
break; | ||
case "com.mysql.jdbc.Driver": | ||
poolConfiguration.connectionFactoryConfiguration().jdbcProperty("useSSL", "false"); | ||
break; |
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.
We should have the JDBC drivers push some support for that via build items instead of having specific code here.
I have some plans for that but it will have to wait.
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.
I agree, we can do this later (not a lot later though ;) ).
|
||
// Indicates that this extension would like the SSL support to be enabled | ||
sslNativeSupport.produce(new ExtensionSslNativeSupportBuildItem(FeatureBuildItem.AGROAL)); |
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.
I push a build item here. @dmlloyd apparently, you preferred having a separate extension do that. I thought having the extensions requiring this could be helpful.
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.
Do all drivers support 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.
I think most of the TCP based drivers do, probably all of them now that SSL is becoming the norm.
In the end, I think I would make it something coming from the driver extension but that's for another day. My plan is to have a build item for JDBC drivers that would be consumed by the Agroal extension but that's for another day.
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.
This is OK for now. We can come back to this later and come up with something fancy that can handle JDK, NSS, OpenSSL, etc.
LGTM. |
Rebased. |
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.
(deleted)
|
||
import org.jboss.builder.item.MultiBuildItem; | ||
|
||
public final class ExtensionSslNativeSupportBuildItem extends MultiBuildItem { |
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.
This is fine for now.
|
||
// Indicates that this extension would like the SSL support to be enabled | ||
sslNativeSupport.produce(new ExtensionSslNativeSupportBuildItem(FeatureBuildItem.AGROAL)); |
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.
This is OK for now. We can come back to this later and come up with something fancy that can handle JDK, NSS, OpenSSL, etc.
break; | ||
case "com.mysql.jdbc.Driver": | ||
poolConfiguration.connectionFactoryConfiguration().jdbcProperty("useSSL", "false"); | ||
break; |
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.
I agree, we can do this later (not a lot later though ;) ).
Ah, nevermind, carry on :) |
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
Is there a guide on how we now can connect to Postgres with SSL with quarkus? Im using the vertx quickstart for this. |
This PR is on top of #691 so this one should be merged first.
Only the last 3 commits should be reviewed.
I'm also pretty sure it will conflict with David's config stuff so I'll rebase accordingly once it's in.
Fixes #454 .