-
Notifications
You must be signed in to change notification settings - Fork 150
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
remove SSL Config and NR certs #245
Conversation
AITS look good - https://javaagent-build.pdx.vm.datanerd.us/job/JavaAgent_PR_AIT/56/ |
if (caBundlePath != null) { | ||
Agent.LOG.log(Level.INFO, "use_private_ssl configuration setting has been removed."); | ||
} else { | ||
Agent.LOG.log(Level.SEVERE, "use_private_ssl configuration setting has been removed. Please use ca_bundle_path instead."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that we always want to recommend that customers use ca_bundle_path
in the case where use_private_ssl
is set and ca_bundle_path
is not specified. I would think that in most cases we would prefer that the customer simply removes use_private_ssl
and lets the agent use the default JVM truststore.
I would rather this read something like:
The use_private_ssl configuration setting has been removed and will be ignored. The agent will use the JVM/JRE truststore by default unless you configure ca_bundle_path to use a different truststore.
@@ -125,6 +56,9 @@ private static KeyStore getKeyStore(String caBundlePath) | |||
while (is.available() > 0) { | |||
try { | |||
caCerts.add((X509Certificate) cf.generateCertificate(is)); | |||
} catch (CertificateException e) { | |||
Agent.LOG.log(Level.SEVERE, "Failed to generate ca_bundle_path certificate. Verify the certificate format. For instance, is it this issue JDK-8208602 ?", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get rid of this line:
For instance, is it this issue JDK-8208602 ?
It's not going to mean anything to most customers and there's no guarantee that it will always be searchable.
@@ -125,6 +56,9 @@ private static KeyStore getKeyStore(String caBundlePath) | |||
while (is.available() > 0) { | |||
try { | |||
caCerts.add((X509Certificate) cf.generateCertificate(is)); | |||
} catch (CertificateException e) { | |||
Agent.LOG.log(Level.SEVERE, "Failed to generate ca_bundle_path certificate. Verify the certificate format. For instance, is it this issue JDK-8208602 ?", e); | |||
break; | |||
} catch (Throwable t) { | |||
Agent.LOG.log(Level.SEVERE, | |||
"Unable to generate ca_bundle_path certificate. Will not process further certs.", t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the new catch block above will prevent the code from getting into this catch block and logging Unable to generate ca_bundle_path certificate. Will not process further certs
since the exception that was making it here was CertificateException
.
2021-03-11T15:51:17,781-0800 [89136 1] com.newrelic ERROR: Unable to generate ca_bundle_path certificate. Will not process further certs.
java.security.cert.CertificateException: Could not parse certificate: java.io.IOException: Illegal header: -----BEGIN CERTIFICATE-----
Why don't we get rid of the new catch and just merge the two log lines together in the old catch block:
} catch (Throwable t) {
Agent.LOG.log(Level.SEVERE,
"Unable to generate ca_bundle_path certificate. Please verify the certificate format. Will not process further certs.", t);
}
In general it's a terrible idea to catch (Throwable t)
and catching a more specific exception (like you were doing with CertificateException
) would be preferred but I don't know if Throwable
is used here for a specific reason or not and I'd prefer to change as little behavior as possible around this logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nitpick about her newline character otherwise looks good to go.
@@ -356,7 +356,7 @@ private String initSSLConfig() { | |||
if (caBundlePath != null) { | |||
Agent.LOG.log(Level.INFO, "use_private_ssl configuration setting has been removed."); | |||
} else { | |||
Agent.LOG.log(Level.SEVERE, "use_private_ssl configuration setting has been removed. Please use ca_bundle_path instead."); | |||
Agent.LOG.log(Level.SEVERE, "The use_private_ssl configuration setting has been removed and will be ignored. The agent will use the JVM/JRE truststore by default unless you configure ca_bundle_path to use a different truststore.\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the newline character (\n
) needed here or is it just a copy & paste artifact? Let's remove it if it isn't intentional.
For #244
use_private_ssl
ca_bundle_path
is usedManually tested against this internal endpoint with the following settings:
As expected, agent successfully negotiated SSL handshake and logged use_private_ssl has been removed.
As expected, agent was unsuccessful in negotiating SSL handshake. Also,
Using ca_bundle_path: ....
was logged as well.