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 a56a8c9..42e5607 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/Configuration.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/Configuration.java
@@ -31,12 +31,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,
@@ -47,19 +49,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;
}
@@ -79,4 +94,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..9c9e660
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/DefaultHttpClientProvider.java
@@ -0,0 +1,90 @@
+// 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.NoopHostnameVerifier;
+import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.security.cert.X509Certificate;
+import java.security.KeyManagementException;
+import java.security.NoSuchAlgorithmException;
+
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.X509TrustManager;
+
+/* 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 e96f5ed..d49df2d 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,14 @@
package com.googlesource.gerrit.plugins.webhooks;
-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 com.google.inject.Provider;
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;
@@ -39,15 +29,11 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import com.google.inject.Inject;
-import com.google.inject.Provider;
-
/**
* Provides an HTTP client with SSL capabilities.
*/
-class HttpClientProvider implements Provider<CloseableHttpClient> {
- private static final Logger log = LoggerFactory
- .getLogger(HttpClientProvider.class);
+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:
private static final int MAX_CONNECTIONS = 2 * CONNECTIONS_PER_ROUTE;
@@ -55,16 +41,17 @@
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();
@@ -106,12 +93,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);
@@ -119,39 +102,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 01f906a..3e4a2ab 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpSession.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpSession.java
@@ -14,25 +14,38 @@
package com.googlesource.gerrit.plugins.webhooks;
-import java.io.IOException;
-import java.nio.charset.StandardCharsets;
-import java.util.Map.Entry;
+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 org.apache.http.client.config.RequestConfig;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.entity.StringEntity;
import org.apache.http.impl.client.CloseableHttpClient;
-import com.google.common.net.MediaType;
-import com.google.inject.Inject;
-import com.googlesource.gerrit.plugins.webhooks.HttpResponseHandler.HttpResult;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.Map.Entry;
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 f8f5114..4d0f579 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/Module.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/Module.java
@@ -14,6 +14,9 @@
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 java.util.concurrent.ScheduledThreadPoolExecutor;
import org.apache.http.impl.client.CloseableHttpClient;
@@ -23,6 +26,7 @@
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.inject.Inject;
import com.google.inject.Scopes;
+import com.google.inject.name.Names;
public class Module extends FactoryModule {
private final ProcessorModule processors;
@@ -38,13 +42,18 @@
.annotatedWith(WebHooksExecutor.class)
.toProvider(ExecutorProvider.class)
.in(Scopes.SINGLETON);
- bind(Configuration.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 51d00ae..87d003d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java
@@ -27,7 +27,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;
@@ -48,7 +47,7 @@
@AssistedInject
public PostTask(@WebHooksExecutor ScheduledThreadPoolExecutor executor,
- final Provider<HttpSession> session,
+ final HttpSession.Factory session,
final EventProcessor processor,
@Assisted final ProjectEvent event,
@Assisted final RemoteConfig remote) {
@@ -59,7 +58,7 @@
this.session = Suppliers.memoize(new Supplier<HttpSession>() {
@Override
public HttpSession get() {
- return session.get();
+ return session.create(remote);
}
});
this.processor = Suppliers.memoize(new Supplier<Optional<EventProcessor.Request>>() {
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 18fa868..d69edba 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/RemoteConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/RemoteConfig.java
@@ -30,6 +30,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;
@@ -69,6 +70,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..70d71bd
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/SslVerifyingHttpClientProvider.java
@@ -0,0 +1,48 @@
+// 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..c23f1bd 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/
@@ -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 72dd96d..80d548e 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java
@@ -22,7 +22,7 @@
import com.google.common.base.Optional;
import com.google.gerrit.server.events.ProjectCreatedEvent;
-import com.google.inject.util.Providers;
+
import java.io.IOException;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
@@ -64,6 +64,9 @@
private ScheduledThreadPoolExecutor executor;
@Mock
+ private HttpSession.Factory sessionFactory;
+
+ @Mock
private EventProcessor processor;
@Mock private EventProcessor.Request content;
@@ -76,7 +79,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
