Skip to content
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

Merged
merged 3 commits into from
Feb 3, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.jboss.shamrock.creator.phase.augment.AugmentOutcome;
import org.jboss.shamrock.creator.phase.runnerjar.RunnerJarOutcome;
import org.jboss.shamrock.creator.util.IoUtils;

import io.smallrye.config.SmallRyeConfigProviderResolver;

/**
Expand All @@ -57,6 +58,8 @@ public class NativeImagePhase implements AppCreationPhase<NativeImagePhase>, Nat

private static final String GRAALVM_HOME = "GRAALVM_HOME";

private static final String SHAMROCK_PREFIX = "shamrock.";

private Path outputDir;

private boolean reportErrorsAtRuntime;
Expand Down Expand Up @@ -290,12 +293,18 @@ public void provideOutcome(AppCreator ctx) throws AppCreatorException {
}
// TODO this is a temp hack
final Path propsFile = ctx.resolveOutcome(AugmentOutcome.class).getAppClassesDir().resolve("native-image.properties");

boolean enableSslNative = false;
if (Files.exists(propsFile)) {
final Properties properties = new Properties();
try (BufferedReader reader = Files.newBufferedReader(propsFile, StandardCharsets.UTF_8)) {
properties.load(reader);
}
for (String propertyName : properties.stringPropertyNames()) {
if (propertyName.startsWith(SHAMROCK_PREFIX)) {
continue;
}

final String propertyValue = properties.getProperty(propertyName);
// todo maybe just -D is better than -J-D in this case
if (propertyValue == null) {
Expand All @@ -304,13 +313,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;
Copy link
Member Author

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.

}
if(config != null) {
if(config.getOptionalValue("shamrock.ssl.native", Boolean.class).orElse(false)) {
enableHttpsUrlHandler = true;
enableJni = true;
enableAllSecurityServices = true;
}
if (enableSslNative) {
enableHttpsUrlHandler = true;
enableJni = true;
enableAllSecurityServices = true;
}
if (additionalBuildArgs != null) {
additionalBuildArgs.forEach(command::add);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
package org.jboss.shamrock.deployment;

import java.io.File;
import java.util.Optional;

import org.jboss.shamrock.deployment.annotations.BuildStep;
import org.jboss.shamrock.deployment.builditem.SystemPropertyBuildItem;
import org.jboss.shamrock.deployment.builditem.SslNativeConfigBuildItem;
import org.jboss.shamrock.runtime.annotations.ConfigGroup;
import org.jboss.shamrock.runtime.annotations.ConfigItem;

public class SslProcessor {
/**
* SSL configuration.
*/

Config ssl;

@ConfigGroup
Expand All @@ -19,21 +17,11 @@ static class Config {
* Enable native SSL support.
*/
@ConfigItem(name = "native")
boolean native_;
Optional<Boolean> native_;
}

@BuildStep
SystemPropertyBuildItem setupNativeSsl() {
String graalVmHome = System.getenv("GRAALVM_HOME");
if(ssl.native_) {
// I assume we only fail if we actually enable it, but perhaps there's a no-native called that we can't
// see here?

// FIXME: fail build? what sort of error here?
if(graalVmHome == null)
throw new RuntimeException("GRAALVM_HOME environment variable required");
return new SystemPropertyBuildItem("java.library.path", graalVmHome+File.separator+"jre"+File.separator+"lib"+File.separator+"amd64");
}
return null;
SslNativeConfigBuildItem setupNativeSsl() {
return new SslNativeConfigBuildItem(ssl.native_);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright 2019 Red Hat, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.jboss.shamrock.deployment.builditem;

import org.jboss.builder.item.MultiBuildItem;

public final class ExtensionSslNativeSupportBuildItem extends MultiBuildItem {
Copy link
Member Author

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.

Copy link
Member

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.


private String extension;

public ExtensionSslNativeSupportBuildItem(String extension) {
this.extension = extension;
}

public String getExtension() {
return extension;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
*/
public final class FeatureBuildItem extends MultiBuildItem {

public static final String AGROAL = "agroal";
public static final String CDI = "cdi";
public static final String TRANSACTIONS = "transactions";
public static final String WEBSOCKET = "websocket";
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright 2019 Red Hat, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.jboss.shamrock.deployment.builditem;

import java.util.Optional;

import org.jboss.builder.item.SimpleBuildItem;

public final class SslNativeConfigBuildItem extends SimpleBuildItem {

private Optional<Boolean> enableSslNativeConfig;

public SslNativeConfigBuildItem(Optional<Boolean> enableSslNativeConfig) {
this.enableSslNativeConfig = enableSslNativeConfig;
}

public Optional<Boolean> get() {
return enableSslNativeConfig;
}

public boolean isEnabled() {
// default is to disable the SSL native support
return enableSslNativeConfig.isPresent() && enableSslNativeConfig.get();
}

public boolean isExplicitlyDisabled() {
return enableSslNativeConfig.isPresent() && !enableSslNativeConfig.get();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,29 @@

import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import org.jboss.logging.Logger;
import org.jboss.shamrock.deployment.annotations.BuildProducer;
import org.jboss.shamrock.deployment.annotations.BuildStep;
import org.jboss.shamrock.deployment.builditem.substrate.SubstrateSystemPropertyBuildItem;
import org.jboss.shamrock.deployment.builditem.substrate.SubstrateProxyDefinitionBuildItem;
import org.jboss.shamrock.deployment.builditem.substrate.SubstrateResourceBundleBuildItem;
import org.jboss.shamrock.deployment.builditem.ExtensionSslNativeSupportBuildItem;
import org.jboss.shamrock.deployment.builditem.SslNativeConfigBuildItem;
import org.jboss.shamrock.deployment.builditem.substrate.RuntimeInitializedClassBuildItem;
import org.jboss.shamrock.deployment.builditem.substrate.RuntimeReinitializedClassBuildItem;
import org.jboss.shamrock.deployment.builditem.substrate.SubstrateConfigBuildItem;
import org.jboss.shamrock.deployment.builditem.substrate.SubstrateProxyDefinitionBuildItem;
import org.jboss.shamrock.deployment.builditem.substrate.SubstrateResourceBundleBuildItem;
import org.jboss.shamrock.deployment.builditem.substrate.SubstrateSystemPropertyBuildItem;

//TODO: this should go away, once we decide on which one of the API's we want
class SubstrateConfigBuildStep {

private static final Logger log = Logger.getLogger(SubstrateConfigBuildStep.class);

@BuildStep
void build(List<SubstrateConfigBuildItem> substrateConfigBuildItems,
SslNativeConfigBuildItem sslNativeConfig,
List<ExtensionSslNativeSupportBuildItem> extensionSslNativeSupport,
BuildProducer<SubstrateProxyDefinitionBuildItem> proxy,
BuildProducer<SubstrateResourceBundleBuildItem> resourceBundle,
BuildProducer<RuntimeInitializedClassBuildItem> runtimeInit,
Expand All @@ -55,5 +63,19 @@ void build(List<SubstrateConfigBuildItem> substrateConfigBuildItems,
proxy.produce(new SubstrateProxyDefinitionBuildItem(i));
}
}

if (sslNativeConfig.isEnabled()) {
nativeImage.produce(new SubstrateSystemPropertyBuildItem("shamrock.ssl.native", "true"));
} else if (!sslNativeConfig.isExplicitlyDisabled() && !extensionSslNativeSupport.isEmpty()) {
// we have extensions desiring the SSL support and it's not explicitly disabled
nativeImage.produce(new SubstrateSystemPropertyBuildItem("shamrock.ssl.native", "true"));

if (log.isDebugEnabled()) {
log.debugf("Native SSL support enabled due to extensions [%s] requiring it",
extensionSslNativeSupport.stream().map(s -> s.getExtension()).collect(Collectors.joining(", ")));
}
} else {
nativeImage.produce(new SubstrateSystemPropertyBuildItem("shamrock.ssl.native", "false"));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
import org.jboss.shamrock.deployment.annotations.BuildProducer;
import org.jboss.shamrock.deployment.annotations.BuildStep;
import org.jboss.shamrock.deployment.annotations.Record;
import org.jboss.shamrock.deployment.builditem.ExtensionSslNativeSupportBuildItem;
import org.jboss.shamrock.deployment.builditem.FeatureBuildItem;
import org.jboss.shamrock.deployment.builditem.SslNativeConfigBuildItem;
import org.jboss.shamrock.deployment.builditem.substrate.ReflectiveClassBuildItem;

class AgroalProcessor {
Expand All @@ -46,15 +49,19 @@ AdditionalBeanBuildItem registerBean() {
@Record(STATIC_INIT)
@BuildStep
BeanContainerListenerBuildItem build(
BuildProducer<FeatureBuildItem> feature,
BuildProducer<ReflectiveClassBuildItem> reflectiveClass,
BuildProducer<DataSourceDriverBuildItem> datasourceDriver,
SslNativeConfigBuildItem sslNativeConfig, BuildProducer<ExtensionSslNativeSupportBuildItem> sslNativeSupport,
DataSourceTemplate template
) throws Exception {
if (! datasource.url.isPresent() || ! datasource.driver.isPresent()) {
log.warn("Agroal extension was included in build however no data source URL and/or driver class has been defined");
return null;
}

feature.produce(new FeatureBuildItem(FeatureBuildItem.AGROAL));

reflectiveClass.produce(new ReflectiveClassBuildItem(false, false,
io.agroal.pool.ConnectionHandler[].class.getName(),
io.agroal.pool.ConnectionHandler.class.getName(),
Expand All @@ -63,14 +70,13 @@ BeanContainerListenerBuildItem build(
java.sql.ResultSet.class.getName(),
java.sql.ResultSet[].class.getName()
));

reflectiveClass.produce(new ReflectiveClassBuildItem(false, false, datasource.driver.get()));

if (datasource.driver.isPresent()) {
datasourceDriver.produce(new DataSourceDriverBuildItem(datasource.driver.get()));
}
datasourceDriver.produce(new DataSourceDriverBuildItem(datasource.driver.get()));

sslNativeSupport.produce(new ExtensionSslNativeSupportBuildItem(FeatureBuildItem.AGROAL));
Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.


return new BeanContainerListenerBuildItem(template.addDatasource(datasource));
return new BeanContainerListenerBuildItem(template.addDatasource(datasource, sslNativeConfig.isExplicitlyDisabled()));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public class DataSourceProducer {
private boolean xa;
private Integer minSize;
private Integer maxSize;
private boolean disableSslSupport = false;

private AgroalDataSource agroalDataSource;

Expand Down Expand Up @@ -118,6 +119,20 @@ public AgroalDataSource getDatasource() throws SQLException {
poolConfiguration.maxSize( DEFAULT_MAX_POOL_SIZE );
}

// SSL support: we should push the driver specific code to the driver extensions but it will have to do for now
if (disableSslSupport) {
switch (driver.getName()) {
case "org.postgresql.Driver":
poolConfiguration.connectionFactoryConfiguration().jdbcProperty("sslmode", "disable");
break;
case "com.mysql.jdbc.Driver":
poolConfiguration.connectionFactoryConfiguration().jdbcProperty("useSSL", "false");
break;
Copy link
Member Author

@gsmet gsmet Jan 31, 2019

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.

Copy link
Member

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 ;) ).

default:
log.warning("Agroal does not support disabling SSL for driver " + driver.getName());
Copy link
Member Author

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.

}
}

//Explicit reference to bypass reflection need of the ServiceLoader used by AgroalDataSource#from
agroalDataSource = new io.agroal.pool.DataSource(dataSourceConfiguration.get());
log.log(Level.INFO, "Started data source " + url);
Expand Down Expand Up @@ -222,4 +237,8 @@ public Integer getMaxSize() {
public void setMaxSize(Integer maxSize) {
this.maxSize = maxSize;
}

public void disableSslSupport() {
this.disableSslSupport = true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
@Template
public class DataSourceTemplate {

public BeanContainerListener addDatasource(DataSourceConfig config) {
public BeanContainerListener addDatasource(DataSourceConfig config, boolean disableSslSupport) {
return new BeanContainerListener() {
@Override
public void created(BeanContainer beanContainer) {
Expand All @@ -42,6 +42,9 @@ public void created(BeanContainer beanContainer) {
}
producer.setMinSize(config.minSize);
producer.setMaxSize(config.maxSize);
if (disableSslSupport) {
producer.disableSslSupport();
}
}
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
package org.jboss.shamrock.jpa.runtime.boot;

import java.io.Serializable;
import java.security.NoSuchAlgorithmException;
import java.util.Map;

import javax.persistence.EntityManagerFactory;
import javax.persistence.EntityNotFoundException;
import javax.persistence.PersistenceException;
Expand Down Expand Up @@ -105,6 +107,17 @@ public void generateSchema() {
}

private PersistenceException persistenceException(String message, Exception cause) {
// Provide a comprehensible message if there is an issue with SSL support
Throwable t = cause;
while (t != null) {
if (t instanceof NoSuchAlgorithmException) {
message += "Unable to enable SSL support. You might be in the case where you used the `shamrock.ssl.native=false` configuration"
+ " and SSL was not disabled automatically for your driver.";
break;
}
t = t.getCause();
}

return new PersistenceException(getExceptionHeader() + message, cause);
}

Expand Down
Loading