Tr default cert by ajschmidt · Pull Request #3392 · apache/trafficcontrol

ajschmidt · 2019-03-07T21:09:12Z

Which issue is fixed by this PR? If not related to an existing issue, what does this PR do?

This PR sets and explicit self-signed certificate as the 'default' certificate which gets returned during an SSL handshake if the requesting host does not match any of the other valid certificates TR hosts. The default certificate will not match any hostname and should be rejected by the client.

Which TC components are affected by this PR?

What is the best way to verify this PR? Please include manual steps or automated tests.

(If no tests are part of this PR, please provide explanation as to why no tests are included.)

There are unit tests and integration tests built into the TR build which test this PR. To manually test:
T1. use curl -kv --resolve to connect with an SNI host that matches the domain of an existing DS, but not the regular expression. For instance try https://cdn.someds.somecdn.toplevel for a DS that responds to https://edge.someds.somecdn.toplevel
R1. Inspect the curl output and you should see that the TLS handshake was successful but the certificate returned was for domain : Invalid

Check all that apply

This PR includes tests
This PR includes documentation updates
This PR includes an update to CHANGELOG.md
This PR includes all required license headers
This PR includes a database migration (ensure that migration sequence is correct)
This PR fixes a serious security flaw. Read more: www.apache.org/security

asfgit · 2019-03-07T21:15:33Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3322/
Test PASSed.

rawlinp · 2019-03-11T23:35:13Z

is there other code that also synchronizes on this object?

Yes. The importCertificateData also synchronizes on the CertificateRegistry instance. Also, the getInstance() method could be called by multiple threads.

importCertificateDataList is a synchronized method, which I think is the same as if the method body was synchronized on CertificateRegistry.class, right?

Yes. I placed this Synchronize here to protect the data structure in the event that more than one thread calls the 'getInstance' method during startup. There are in fact three endpoints being initialized using the LanguidNioProtocol at startup. Once initialized this method never gets called again so they synchronized doesn't have any impact on performance.

The data structure would only be protected from race conditions if the other code that accesses it is also synchronized on the same object. synchronized (CertificateRegistryHolder.DELIVERY_SERVICE_CERTIFICATES) and synchronized (CertificateRegistry.class) are locking on two different things, so both of those methods could overlap with each other.

this is done in new commits

Ah yeah I just realized that getInstance is now static synchronized while importCertificateDataList is non-static synchronized. So those methods aren't actually locking on the same object yet (static locks on the class while non-static locks on a single instance).

So if one thread calls getInstance and another thread calls importCertificateDataList at the same time, both methods will interleave and mutate handshakeDataMap at the same time. That's what you're trying to prevent with the synchronization here, right?

I'm more concerned about simultaneous calls to 'getInstance' during startup. The poller that calls 'importCertificateDataList' launches after initialization has succeeded.

This discussion has caused me to realize that there is a bug in getInstance. I've just committed another change.

I committed one more change based on our discussion and just moved the default cert creation into the importCertificateData method.

rawlinp · 2019-03-11T23:36:37Z

should this check if the created value is null? what would happen if it ends up being null?

Yes. I've updated the PR. Now it returns a null CertificateRegistry if it can't create the cert.

rawlinp · 2019-03-12T16:11:51Z

Also, can you add a line in CHANGELOG.md about TR's new default cert behavior?

asfgit · 2019-03-13T00:12:59Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3340/
Test PASSed.

Improved descriptions in the default self-signed cert Improved warning messages in x509 certificate validation

asfgit · 2019-04-02T17:50:35Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3443/
Test PASSed.

rawlinp · 2019-04-04T18:07:43Z

+         */
+        @Override
+        protected void doRun() {
+	        final SocketWrapperBase<NioChannel> localWrapper = socketWrapper;


this line got mixed up w/ tabs and spaces

rawlinp · 2019-04-04T18:21:13Z


 import com.comcast.cdn.traffic_control.traffic_router.protocol.RouterNioEndpoint;
 import com.comcast.cdn.traffic_control.traffic_router.shared.CertificateData;
+//import com.fasterxml.jackson.core.type.TypeReference;


these imports can be removed if they're unused

rawlinp · 2019-04-05T16:09:47Z

+         */
+        @Override
+        protected void doRun() {
+			final SocketWrapperBase<NioChannel> localWrapper = socketWrapper;


this line is still way over-indented -- its using tabs and it looks like the other lines use spaces

Okay. It should be better now. There seems to be an issue with one of my code templates.

Thanks, it looks much better now.

asfgit · 2019-04-05T16:12:38Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3475/
Test FAILed.

asfgit · 2019-04-05T17:16:12Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3477/
Test FAILed.

asfgit · 2019-04-05T17:51:45Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3478/
Test PASSed.

asfgit · 2019-04-08T22:30:36Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3492/
Test PASSed.

rawlinp · 2019-04-09T22:20:42Z

+				if (defaultHd == null){
+					log.error("Failed to initialize the CertificateRegistry because of a problem with the 'default' " +
+							"certificate.  Returning the Certificate Registry without a default.");
+					return;


If this ends up being the case, would we still want to do handshakeDataMap = master before returning so that all the newly-processed certs would still be loaded (just without a default cert), or would it never make sense to keep the rest of the certs without the default cert in that case?

In both situation I think it will fail in either case when it goes to RouterNioEndpoint because there has to be a default SSLHost. This condition should only occur when TR is being initiated. I did perform a test and made sure that these lines only get called once. After that they are skipped because here is always a default cert.

In other words. I think this is the appropriate behavior. If it can't create the default certificate, something bigger is wrong and TR should not start.

Thanks, you make a fair point.

asfgit · 2019-04-09T23:55:52Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3502/
Test PASSed.

rawlinp · 2019-04-10T16:00:27Z

+         */
+        @Override
+        protected void doRun() {
+			final SocketWrapperBase<NioChannel> localWrapper = socketWrapper;


Thanks, it looks much better now.

rawlinp · 2019-04-10T16:02:30Z

+				if (defaultHd == null){
+					log.error("Failed to initialize the CertificateRegistry because of a problem with the 'default' " +
+							"certificate.  Returning the Certificate Registry without a default.");
+					return;


Thanks, you make a fair point.

…Tomcat

* Update maven-pmd-plugin to 3.14.0, which uses PMD 6.29.0 * Update Java PMD rule names to their PMD 6 equivalents * Unlist rules that are scheduled for removal in PMD 7 * Suppress PMD.CyclomaticComplexity and PMD.NPathComplexity * Fix PMD warnings * Set JAVA_HOME so Maven can find javac * Rebuild the Traffic Router builder image if its Dockerfile was changed * Use a fetch-depth of 4 to check files changed in the last 3 commits for the build-rpms GHA * Update Traffic Router Java version to 11 * Do not set UseSplitVerifier, SplitVerifier is required in Java 11 * Resolve new CloseResource PMD failure in Java 11 * Use RSAPrivateCrtKey, RSAPrivateCrtKeyImpl is not visible in Java 11 * Decrease NPath complexity of DeliveryService constructor * Do not escape '"' * Commant and unabbreviate commit comparison * Close resources in LetsEncryptDnsChallengeWatcher.readConfigFile() * Do not close socket before handling the request * Bind port to wildcard host for OpenJDK 11 compatibility * Store socketWrapper socket for use after super.doRun() * Fix PMD failures where all PMD failures were suppressed * Remove temporary hack from #3392 that resolved an SSL issue in Tomcat * Use OpenJDK 11 in Systemd service files * Use OpenJDK 11 for Ansible Java HOME

mitchell852 added Traffic Router related to Traffic Router new feature A new feature, capability or behavior labels Mar 8, 2019

rawlinp suggested changes Mar 12, 2019

View reviewed changes

rawlinp added this to the 3.0.1 milestone Mar 12, 2019

ASchmidt added 3 commits March 12, 2019 16:29

Initial commit of default cert changes

6539c1c

Changed error message for Certificate Expired

cd2b13d

corrections from PR Review

ac74234

ajschmidt force-pushed the tr-default-cert branch from dc7c08e to ac74234 Compare March 12, 2019 23:53

rawlinp removed this from the 3.0.1 milestone Mar 14, 2019

ASchmidt added 2 commits March 21, 2019 10:12

Added CertificateExtensions to default SSL cert

c38a291

Added fix for Tomcat bug related to error state of sockets

98617f9

Improved descriptions in the default self-signed cert Improved warning messages in x509 certificate validation

rawlinp reviewed Apr 4, 2019

View reviewed changes

fixed some spacing and made getInstance synchronized

98c10ca

rawlinp reviewed Apr 5, 2019

View reviewed changes

fixed some spacing and made getInstance synchronized

21f3806

added an if condition to 'getInstance' to prevent double initialization

db4e04c

Moved default certificate lifecycle into importCertificateDataList

cfbfc48

rawlinp reviewed Apr 9, 2019

View reviewed changes

Reformatted RouterNIOEndpoint

9fe3742

rawlinp approved these changes Apr 10, 2019

View reviewed changes

rawlinp merged commit 8900fcc into apache:master Apr 10, 2019

zrhoffman added a commit to zrhoffman/trafficcontrol that referenced this pull request Dec 3, 2021

Remove temporary hack from apache#3392 that resolved an SSL issue in …

0d2cc14

…Tomcat

Conversation

ajschmidt commented Mar 7, 2019

Which issue is fixed by this PR? If not related to an existing issue, what does this PR do?

Which TC components are affected by this PR?

What is the best way to verify this PR? Please include manual steps or automated tests.

(If no tests are part of this PR, please provide explanation as to why no tests are included.)

Check all that apply

Uh oh!

asfgit commented Mar 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rawlinp Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rawlinp commented Mar 12, 2019

Uh oh!

asfgit commented Mar 13, 2019

Uh oh!

asfgit commented Apr 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rawlinp Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Apr 5, 2019

Uh oh!

asfgit commented Apr 5, 2019

Uh oh!

asfgit commented Apr 5, 2019

Uh oh!

asfgit commented Apr 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Apr 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rawlinp Apr 4, 2019 •

edited

Loading

rawlinp Apr 4, 2019 •

edited

Loading