Introduce sslVerify configuration parameter
New sslVerify parameter was introduced to handle HTTPS SSL
certificate verification. It might be set either globally (in
gerrit.config) with default value 'false' (current behavior is
preserved) or locally per remote (when not specified it defaults to
global).
Implementation details:
1. once HttpClient is built one cannot change its
ConnectionSocketFactory to either verify or ignore SSL certificate
2. introduction of 'https+verify' schema was considered but it looked
somewhat hackish as it requires remote URL rewrite prior sending (worth
mentioning the fact that down the HttpClient implementation either
'https' or 'http' schema types are only assumed and I was not sure if
I could properly trace all places to be changed)
3. depending on effective remote configuration either verifying or
ignoring HttpClient is being used to perform post operation
Change-Id: I52447e670cc0c8c6b113a7234c35d1838ce7c45d
Signed-off-by: Jacek Centkowski <jcentkowski@collab.net>
diff --git a/src/main/java/com/googlesource/gerrit/plugins/webhooks/Configuration.java b/src/main/java/com/googlesource/gerrit/plugins/webhooks/Configuration.java
index eb1fb8c..3e6ba62 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/Configuration.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/Configuration.java
@@ -30,12 +30,14 @@
private static final int DEFAULT_MAX_TRIES = 5;
private static final int DEFAULT_RETRY_INTERVAL = 1000;
private static final int DEFAULT_THREAD_POOL_SIZE = 2;
+ private static final boolean DEFAULT_SSL_VERIFY = false;
private final int connectionTimeout;
private final int socketTimeout;
private final int maxTries;
private final int retryInterval;
private final int threadPoolSize;
+ private final boolean sslVerify;
@Inject
protected Configuration(PluginConfigFactory config, @PluginName String pluginName) {
@@ -45,18 +47,32 @@
maxTries = getInt(cfg, RemoteConfig.MAX_TRIES, DEFAULT_MAX_TRIES);
retryInterval = getInt(cfg, RemoteConfig.RETRY_INTERVAL, DEFAULT_RETRY_INTERVAL);
threadPoolSize = getInt(cfg, "threadPoolSize", DEFAULT_THREAD_POOL_SIZE);
+ sslVerify = getBoolean(cfg, RemoteConfig.SSL_VERIFY, DEFAULT_SSL_VERIFY);
+ }
+
+ protected boolean getBoolean(PluginConfig cfg, String name, boolean defaultValue) {
+ try {
+ return cfg.getBoolean(name, defaultValue);
+ } catch (IllegalArgumentException e) {
+ logError(name, "boolean", defaultValue, e);
+ return defaultValue;
+ }
}
protected int getInt(PluginConfig cfg, String name, int defaultValue) {
try {
return cfg.getInt(name, defaultValue);
} catch (IllegalArgumentException e) {
- log.error(String.format("invalid value for %s; using default value %d", name, defaultValue));
- log.debug("Failed retrieve integer value: " + e.getMessage(), e);
+ logError(name, "integer", defaultValue, e);
return defaultValue;
}
}
+ protected void logError(String name, String type, Object defaultValue, Exception e) {
+ log.error("invalid value for{}; using default value {}", name, defaultValue);
+ log.debug("Failed retrieve {} value: {}", type, e.getMessage(), e);
+ }
+
public int getConnectionTimeout() {
return connectionTimeout;
}
@@ -76,4 +92,8 @@
public int getThreadPoolSize() {
return threadPoolSize;
}
+
+ public boolean getSslVerify() {
+ return sslVerify;
+ }
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/webhooks/DefaultHttpClientProvider.java b/src/main/java/com/googlesource/gerrit/plugins/webhooks/DefaultHttpClientProvider.java
new file mode 100644
index 0000000..5dc7ff3
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/DefaultHttpClientProvider.java
@@ -0,0 +1,87 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// 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 com.googlesource.gerrit.plugins.webhooks;
+
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import java.security.KeyManagementException;
+import java.security.NoSuchAlgorithmException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.X509TrustManager;
+import org.apache.http.config.Registry;
+import org.apache.http.config.RegistryBuilder;
+import org.apache.http.conn.socket.ConnectionSocketFactory;
+import org.apache.http.conn.socket.PlainConnectionSocketFactory;
+import org.apache.http.conn.ssl.NoopHostnameVerifier;
+import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/* HTTP client that doesn't verify remote's SSL certificate */
+class DefaultHttpClientProvider extends HttpClientProvider {
+ private static final Logger log = LoggerFactory.getLogger(DefaultHttpClientProvider.class);
+
+ static final String DEFAULT = "default";
+
+ @Inject
+ protected DefaultHttpClientProvider(Configuration cfg) {
+ super(cfg, socketFactoryProvider());
+ }
+
+ private static Provider<Registry<ConnectionSocketFactory>> socketFactoryProvider() {
+ return new Provider<Registry<ConnectionSocketFactory>>() {
+ @Override
+ public Registry<ConnectionSocketFactory> get() {
+ return RegistryBuilder.<ConnectionSocketFactory>create()
+ .register(
+ "https",
+ new SSLConnectionSocketFactory(buildSslContext(), NoopHostnameVerifier.INSTANCE))
+ .register("http", PlainConnectionSocketFactory.INSTANCE)
+ .build();
+ }
+ };
+ }
+
+ private static SSLContext buildSslContext() {
+ try {
+ TrustManager[] trustAllCerts = new TrustManager[] {new DummyX509TrustManager()};
+ SSLContext context = SSLContext.getInstance("TLS");
+ context.init(null, trustAllCerts, null);
+ return context;
+ } catch (KeyManagementException | NoSuchAlgorithmException e) {
+ log.warn("Error building SSLContext object", e);
+ return null;
+ }
+ }
+
+ private static class DummyX509TrustManager implements X509TrustManager {
+ @Override
+ public X509Certificate[] getAcceptedIssuers() {
+ return new X509Certificate[0];
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] certs, String authType) {
+ // no check
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] certs, String authType) {
+ // no check
+ }
+ }
+}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpClientProvider.java b/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpClientProvider.java
index 7ba0a3a..aecb43a 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpClientProvider.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpClientProvider.java
@@ -14,24 +14,13 @@
package com.googlesource.gerrit.plugins.webhooks;
-import com.google.inject.Inject;
import com.google.inject.Provider;
-import java.security.KeyManagementException;
-import java.security.NoSuchAlgorithmException;
-import java.security.cert.X509Certificate;
-import javax.net.ssl.SSLContext;
-import javax.net.ssl.TrustManager;
-import javax.net.ssl.X509TrustManager;
import org.apache.http.HttpResponse;
import org.apache.http.client.ServiceUnavailableRetryStrategy;
import org.apache.http.client.config.RequestConfig;
import org.apache.http.config.Registry;
-import org.apache.http.config.RegistryBuilder;
import org.apache.http.conn.HttpClientConnectionManager;
import org.apache.http.conn.socket.ConnectionSocketFactory;
-import org.apache.http.conn.socket.PlainConnectionSocketFactory;
-import org.apache.http.conn.ssl.NoopHostnameVerifier;
-import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.HttpClients;
import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
@@ -40,7 +29,7 @@
import org.slf4j.LoggerFactory;
/** Provides an HTTP client with SSL capabilities. */
-class HttpClientProvider implements Provider<CloseableHttpClient> {
+abstract class HttpClientProvider implements Provider<CloseableHttpClient> {
private static final Logger log = LoggerFactory.getLogger(HttpClientProvider.class);
private static final int CONNECTIONS_PER_ROUTE = 100;
// Up to 2 target instances with the max number of connections per host:
@@ -49,16 +38,18 @@
private static final int MAX_CONNECTION_INACTIVITY = 10000;
private final Configuration cfg;
+ private final Provider<Registry<ConnectionSocketFactory>> socketMgr;
- @Inject
- HttpClientProvider(Configuration cfg) {
+ protected HttpClientProvider(
+ Configuration cfg, Provider<Registry<ConnectionSocketFactory>> socketMgr) {
this.cfg = cfg;
+ this.socketMgr = socketMgr;
}
@Override
public CloseableHttpClient get() {
return HttpClients.custom()
- .setConnectionManager(customConnectionManager())
+ .setConnectionManager(create(socketMgr.get()))
.setDefaultRequestConfig(customRequestConfig())
.setServiceUnavailableRetryStrategy(customServiceUnavailRetryStrategy())
.build();
@@ -104,12 +95,8 @@
}
}
- private HttpClientConnectionManager customConnectionManager() {
- Registry<ConnectionSocketFactory> socketFactoryRegistry =
- RegistryBuilder.<ConnectionSocketFactory>create()
- .register("https", buildSslSocketFactory())
- .register("http", PlainConnectionSocketFactory.INSTANCE)
- .build();
+ private HttpClientConnectionManager create(
+ Registry<ConnectionSocketFactory> socketFactoryRegistry) {
PoolingHttpClientConnectionManager connManager =
new PoolingHttpClientConnectionManager(socketFactoryRegistry);
connManager.setDefaultMaxPerRoute(CONNECTIONS_PER_ROUTE);
@@ -117,37 +104,4 @@
connManager.setValidateAfterInactivity(MAX_CONNECTION_INACTIVITY);
return connManager;
}
-
- private SSLConnectionSocketFactory buildSslSocketFactory() {
- return new SSLConnectionSocketFactory(buildSslContext(), NoopHostnameVerifier.INSTANCE);
- }
-
- private SSLContext buildSslContext() {
- try {
- TrustManager[] trustAllCerts = new TrustManager[] {new DummyX509TrustManager()};
- SSLContext context = SSLContext.getInstance("TLS");
- context.init(null, trustAllCerts, null);
- return context;
- } catch (KeyManagementException | NoSuchAlgorithmException e) {
- log.warn("Error building SSLContext object", e);
- return null;
- }
- }
-
- private static class DummyX509TrustManager implements X509TrustManager {
- @Override
- public X509Certificate[] getAcceptedIssuers() {
- return new X509Certificate[0];
- }
-
- @Override
- public void checkClientTrusted(X509Certificate[] certs, String authType) {
- // no check
- }
-
- @Override
- public void checkServerTrusted(X509Certificate[] certs, String authType) {
- // no check
- }
- }
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpSession.java b/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpSession.java
index 55696b6..fed95e7 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpSession.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpSession.java
@@ -14,8 +14,14 @@
package com.googlesource.gerrit.plugins.webhooks;
+import static com.googlesource.gerrit.plugins.webhooks.DefaultHttpClientProvider.DEFAULT;
+import static com.googlesource.gerrit.plugins.webhooks.SslVerifyingHttpClientProvider.SSL_VERIFY;
+
import com.google.common.net.MediaType;
import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.assistedinject.Assisted;
+import com.google.inject.name.Named;
import com.googlesource.gerrit.plugins.webhooks.HttpResponseHandler.HttpResult;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
@@ -25,11 +31,19 @@
import org.apache.http.impl.client.CloseableHttpClient;
class HttpSession {
+ interface Factory {
+ HttpSession create(RemoteConfig remote);
+ }
+
private final CloseableHttpClient httpClient;
@Inject
- HttpSession(CloseableHttpClient httpClient) {
- this.httpClient = httpClient;
+ HttpSession(
+ @Named(DEFAULT) Provider<CloseableHttpClient> defaultClientProvider,
+ @Named(SSL_VERIFY) Provider<CloseableHttpClient> sslVerifyingClientProvider,
+ @Assisted RemoteConfig remote) {
+ this.httpClient =
+ remote.getSslVerify() ? sslVerifyingClientProvider.get() : defaultClientProvider.get();
}
HttpResult post(RemoteConfig remote, EventProcessor.Request request) throws IOException {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/webhooks/Module.java b/src/main/java/com/googlesource/gerrit/plugins/webhooks/Module.java
index 00a6bfb..6b84862 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/Module.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/Module.java
@@ -14,11 +14,15 @@
package com.googlesource.gerrit.plugins.webhooks;
+import static com.googlesource.gerrit.plugins.webhooks.DefaultHttpClientProvider.DEFAULT;
+import static com.googlesource.gerrit.plugins.webhooks.SslVerifyingHttpClientProvider.SSL_VERIFY;
+
import com.google.gerrit.common.EventListener;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.inject.Inject;
import com.google.inject.Scopes;
+import com.google.inject.name.Names;
import java.util.concurrent.ScheduledExecutorService;
import org.apache.http.impl.client.CloseableHttpClient;
@@ -36,11 +40,22 @@
.annotatedWith(WebHooksExecutor.class)
.toProvider(ExecutorProvider.class)
.in(Scopes.SINGLETON);
- bind(CloseableHttpClient.class).toProvider(HttpClientProvider.class).in(Scopes.SINGLETON);
+
factory(PostTask.Factory.class);
factory(RemoteConfig.Factory.class);
+ factory(HttpSession.Factory.class);
+
DynamicSet.bind(binder(), EventListener.class).to(EventHandler.class);
+ bind(CloseableHttpClient.class)
+ .annotatedWith(Names.named(DEFAULT))
+ .toProvider(DefaultHttpClientProvider.class)
+ .in(Scopes.SINGLETON);
+ bind(CloseableHttpClient.class)
+ .annotatedWith(Names.named(SSL_VERIFY))
+ .toProvider(SslVerifyingHttpClientProvider.class)
+ .in(Scopes.SINGLETON);
+
install(processors);
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java b/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java
index 54e9249..f86703a 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java
@@ -17,7 +17,6 @@
import com.google.common.base.Supplier;
import com.google.common.base.Suppliers;
import com.google.gerrit.server.events.ProjectEvent;
-import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import com.googlesource.gerrit.plugins.webhooks.HttpResponseHandler.HttpResult;
@@ -45,7 +44,7 @@
@AssistedInject
public PostTask(
@WebHooksExecutor ScheduledExecutorService executor,
- Provider<HttpSession> session,
+ HttpSession.Factory session,
EventProcessor processor,
@Assisted ProjectEvent event,
@Assisted RemoteConfig remote) {
@@ -53,7 +52,7 @@
this.remote = remote;
// postpone creation of HttpSession so that it is obtained only when processor
// returns non-empty content
- this.session = Suppliers.memoize(() -> session.get());
+ this.session = Suppliers.memoize(() -> session.create(remote));
this.processor = Suppliers.memoize(() -> processor.process(event, remote));
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/webhooks/RemoteConfig.java b/src/main/java/com/googlesource/gerrit/plugins/webhooks/RemoteConfig.java
index cd11638..80e40d2 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/RemoteConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/RemoteConfig.java
@@ -29,6 +29,7 @@
static final String SOCKET_TIMEOUT = "socketTimeout";
static final String MAX_TRIES = "maxTries";
static final String RETRY_INTERVAL = "retryInterval";
+ static final String SSL_VERIFY = "sslVerify";
private final Configuration global;
private final Config config;
@@ -68,6 +69,10 @@
return config.getInt(REMOTE, name, RETRY_INTERVAL, global.getRetryInterval());
}
+ public boolean getSslVerify() {
+ return config.getBoolean(REMOTE, name, SSL_VERIFY, global.getSslVerify());
+ }
+
// methods were added in order to make configuration
// extensible in EvenptProcessor implementations
public Configuration getGlobal() {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/webhooks/SslVerifyingHttpClientProvider.java b/src/main/java/com/googlesource/gerrit/plugins/webhooks/SslVerifyingHttpClientProvider.java
new file mode 100644
index 0000000..57e25a2
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/SslVerifyingHttpClientProvider.java
@@ -0,0 +1,47 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// 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 com.googlesource.gerrit.plugins.webhooks;
+
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import org.apache.http.config.Registry;
+import org.apache.http.config.RegistryBuilder;
+import org.apache.http.conn.socket.ConnectionSocketFactory;
+import org.apache.http.conn.socket.PlainConnectionSocketFactory;
+import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
+
+/* HTTP client that verifies remote's SSL certificate */
+class SslVerifyingHttpClientProvider extends HttpClientProvider {
+ static final String SSL_VERIFY = "ssl_verify";
+
+ @Inject
+ protected SslVerifyingHttpClientProvider(Configuration cfg) {
+ super(cfg, socketFactoryProvider());
+ }
+
+ private static Provider<Registry<ConnectionSocketFactory>> socketFactoryProvider() {
+ return new Provider<Registry<ConnectionSocketFactory>>() {
+ @Override
+ public Registry<ConnectionSocketFactory> get() {
+ return RegistryBuilder.<ConnectionSocketFactory>create()
+ .register("https", SSLConnectionSocketFactory.getSocketFactory())
+ // the following registration is added for case when one enables SSL verification
+ // for HTTP remote
+ .register("http", PlainConnectionSocketFactory.INSTANCE)
+ .build();
+ }
+ };
+ }
+}
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md
index ec2ad91..95c098a 100644
--- a/src/main/resources/Documentation/config.md
+++ b/src/main/resources/Documentation/config.md
@@ -23,6 +23,7 @@
[remote "foo"]
url = https://foo.org/gerrit-events
maxTries = 3
+ sslVerify = true
[remote "bar"]
url = https://bar.org/
@@ -31,7 +32,7 @@
```
The configuration is inheritable. Connection parameters
-`connectionTimeout`, `socketTimeout`, `maxTries` and `retryInterval`
+`connectionTimeout`, `socketTimeout`, `maxTries`, `retryInterval` and `sslVerify`
can be fine-tuned at remote level.
File 'gerrit.config'
@@ -60,6 +61,11 @@
: Maximum number of threads used to send events to the target instance.
Defaults to 2.
+@PLUGIN@.sslVerify
+: When 'true' SSL certificate verification of all webhooks *is* performed
+ when payload is delivered.
+ Default value is 'false'.
+
File '@PLUGIN@.config'
----------------------
@@ -88,4 +94,8 @@
remote.NAME.retryInterval
: The interval of time in milliseconds between the subsequent auto-retries.
- When not specified, the default value is derrived from global configuration.
\ No newline at end of file
+ When not specified, the default value is derrived from global configuration.
+
+remote.NAME.sslVerify
+: When 'true' SSL certificate verification of remote url *is* performed
+ when payload is delivered, the default value is derived from global configuration.
\ No newline at end of file
diff --git a/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java b/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java
index 4529409..623fbb0 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java
@@ -21,7 +21,6 @@
import static org.mockito.Mockito.when;
import com.google.gerrit.server.events.ProjectCreatedEvent;
-import com.google.inject.util.Providers;
import com.googlesource.gerrit.plugins.webhooks.HttpResponseHandler.HttpResult;
import java.io.IOException;
import java.util.Optional;
@@ -53,6 +52,8 @@
@Mock private HttpSession session;
+ @Mock private HttpSession.Factory sessionFactory;
+
@Mock private ScheduledThreadPoolExecutor executor;
@Mock private EventProcessor processor;
@@ -67,7 +68,8 @@
when(remote.getMaxTries()).thenReturn(MAX_TRIES);
when(remote.getUrl()).thenReturn(WEBHOOK_URL);
when(processor.process(eq(projectCreated), eq(remote))).thenReturn(Optional.of(content));
- task = new PostTask(executor, Providers.of(session), processor, projectCreated, remote);
+ when(sessionFactory.create(eq(remote))).thenReturn(session);
+ task = new PostTask(executor, sessionFactory, processor, projectCreated, remote);
}
@Test
