Skip to content

Added logging on Go server for failed agent registration scenarios.#4664

Merged
jyotisingh merged 1 commit intogocd:masterfrom
jyotisingh:error_logging_agent_registration
Apr 23, 2018
Merged

Added logging on Go server for failed agent registration scenarios.#4664
jyotisingh merged 1 commit intogocd:masterfrom
jyotisingh:error_logging_agent_registration

Conversation

@jyotisingh
Copy link
Copy Markdown
Contributor

No description provided.

@jyotisingh jyotisingh added this to the Release 18.4 milestone Apr 17, 2018
@arvindsv
Copy link
Copy Markdown
Member

There are also TLS requests which fail (at the jetty level) if the certs don't match etc. Is there anything we can do to log those?

@ketan
Copy link
Copy Markdown
Member

ketan commented Apr 17, 2018

There are also TLS requests which fail (at the jetty level) if the certs don't match etc. Is there anything we can do to log those?

Might be able to inject a custom trust manager that gets a callback to validate an incoming certificate.

@arvindsv
Copy link
Copy Markdown
Member

Yes, I think that's a good idea. Unless there's a special logger in jetty to log those (easier), adding a trust manager makes sense. Will need to check perf of that though. I think our connections are long-lived - so it might be ok.

@jyotisingh jyotisingh force-pushed the error_logging_agent_registration branch from ba1e2f2 to d6c9525 Compare April 18, 2018 05:27
@jyotisingh jyotisingh force-pushed the error_logging_agent_registration branch from d6c9525 to d630103 Compare April 19, 2018 10:29
@jyotisingh
Copy link
Copy Markdown
Contributor Author

jyotisingh commented Apr 20, 2018

@arvindsv , @ketan - Does something like this suffice? This is very specific to the agents having an invalid certificate.

From a2b03b3df29e6d082bfb634f6c4e309566e4d67a Mon Sep 17 00:00:00 2001
From: Jyoti Singh <jyotisingh7@gmail.com>
Date: Fri, 20 Apr 2018 15:24:17 +0530
Subject: [PATCH] Added a wrapper over the x509trustManager so we can log the
 certificate validation failures

---
 .../go/server/util/CustomSslContextFactory.java    | 76 ++++++++++++++++++++++
 .../go/server/util/GoSslSocketConnector.java       |  2 +-
 2 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 jetty9/src/main/java/com/thoughtworks/go/server/util/CustomSslContextFactory.java

diff --git a/jetty9/src/main/java/com/thoughtworks/go/server/util/CustomSslContextFactory.java b/jetty9/src/main/java/com/thoughtworks/go/server/util/CustomSslContextFactory.java
new file mode 100644
index 000000000..b1cf8fb90
--- /dev/null
+++ b/jetty9/src/main/java/com/thoughtworks/go/server/util/CustomSslContextFactory.java
@@ -0,0 +1,76 @@
+/*
+ * Copyright 2018 ThoughtWorks, 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 com.thoughtworks.go.server.util;
+
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.X509TrustManager;
+import java.security.KeyStore;
+import java.security.cert.CRL;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+public class CustomSslContextFactory extends SslContextFactory {
+    private static final org.slf4j.Logger LOGGER = LoggerFactory.getLogger(CustomSslContextFactory.class.getName());
+
+    protected TrustManager[] getTrustManagers(KeyStore trustStore, Collection<? extends CRL> crls) throws Exception {
+        TrustManager[] trustManagers = super.getTrustManagers(trustStore, crls);
+        List<TrustManager> managers = new ArrayList<>();
+        for (TrustManager trustManager : trustManagers) {
+            if (trustManager instanceof X509TrustManager) {
+                managers.add(new CustomX509TrustManager((X509TrustManager) trustManager));
+            } else {
+                managers.add(trustManager);
+            }
+        }
+        return managers.toArray(new TrustManager[managers.size()]);
+    }
+
+    private class CustomX509TrustManager implements X509TrustManager {
+        private X509TrustManager trustManager;
+
+        public CustomX509TrustManager(X509TrustManager trustManager) {
+            this.trustManager = trustManager;
+        }
+
+        @Override
+        public void checkClientTrusted(X509Certificate[] x509Certificates, String s) throws CertificateException {
+            try {
+                trustManager.checkClientTrusted(x509Certificates, s);
+            } catch (Exception e) {
+                String name = x509Certificates.length >= 1 ? x509Certificates[0].getSubjectDN().getName() : "NONAME";
+                LOGGER.error("Client certificate was found to be invalid. Subject: [{}] Error: [{}].", name, e.getMessage(), e);
+                throw e;
+            }
+        }
+
+        @Override
+        public void checkServerTrusted(X509Certificate[] x509Certificates, String s) throws CertificateException {
+            trustManager.checkServerTrusted(x509Certificates, s);
+        }
+
+        @Override
+        public X509Certificate[] getAcceptedIssuers() {
+            return trustManager.getAcceptedIssuers();
+        }
+    }
+}
diff --git a/jetty9/src/main/java/com/thoughtworks/go/server/util/GoSslSocketConnector.java b/jetty9/src/main/java/com/thoughtworks/go/server/util/GoSslSocketConnector.java
index 1918f2899..aa6f2478e 100644
--- a/jetty9/src/main/java/com/thoughtworks/go/server/util/GoSslSocketConnector.java
+++ b/jetty9/src/main/java/com/thoughtworks/go/server/util/GoSslSocketConnector.java
@@ -61,7 +61,7 @@ public class GoSslSocketConnector implements GoSocketConnector {
         httpsConfig.setSendServerVersion(false);
         httpsConfig.addCustomizer(new ForwardedRequestCustomizer());
 
-        SslContextFactory sslContextFactory = new SslContextFactory();
+        SslContextFactory sslContextFactory = new CustomSslContextFactory();
         sslContextFactory.setKeyStorePath(keystore.getPath());
         sslContextFactory.setKeyStorePassword(password);
         sslContextFactory.setKeyManagerPassword(password);
-- 
2.13.0

@ketan
Copy link
Copy Markdown
Member

ketan commented Apr 20, 2018

@arvindsv , @ketan - Does something like this suffice? This is very specific to the agents having an invalid certificate.

I recall seeing an error on the agent that indiciates connection failure because of bad ssl certs. Is that error not good enough to have to implement this — is this something we can do in a separate PR?

@ketan
Copy link
Copy Markdown
Member

ketan commented Apr 20, 2018

@arvindsv , @ketan - Does something like this suffice? This is very specific to the agents having an invalid certificate.

Also — that patch looks like what I was thinking about.

@jyotisingh
Copy link
Copy Markdown
Contributor Author

Created a separate PR for this => #4679.
Will merge this one.

@jyotisingh jyotisingh merged commit 7d9f540 into gocd:master Apr 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants