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.
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.
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.
| 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.
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.
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.
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.
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.
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.
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. |
gsmet
changed the title
|
|
||
| import org.jboss.builder.item.MultiBuildItem; | ||
|
|
||
| public final class ExtensionSslNativeSupportBuildItem extends MultiBuildItem { |
|
|
||
| // 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.
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.
I agree, we can do this later (not a lot later though ;) ).
|
Ah, nevermind, carry on :) |
|
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 .