Add SSL Certificate Validation to Traffic Router by ajschmidt · Pull Request #3380 · apache/trafficcontrol

ajschmidt · 2019-03-06T17:56:49Z

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

When TR obtains SSL certificates it now checks each cert before accepting it. A cert could get rejected for being expired, a mismatch between the Delivery Service URL and the subjects of the cert or a mismatch between the modulus of the certificate's private key and the public keys. Rejected certs are just dropped and not used but TR continues to operate using the valid certs it can find.

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.)

Unit tests have been added to the build process which test valid certs and examples of certs triggering each of the failure conditions.

T1. This can be manually tested by adding invalid certificates to a test CDN through Traffic Portal
R1. Watch the error logs of Traffic Router when it next retrieves the SSL certs for the expected error logs.
T2. After seeing the expected errors in the logs then attempt to make and SSL connection with TR for the offending Delivery Service URL. (Use curl with the --resolve option)
R2. The handshake will return the 'default' certificate.

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

…eRegistry

asfgit · 2019-03-06T18:31:00Z

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

ocket8888 · 2019-03-06T23:21:40Z


 start() {
-	systemctl start traffic_router
+	chmod 777 starttr.sh


this should probably just be done once, in the Dockerfile, not every time TR starts.

Done in new commits.

ocket8888 · 2019-03-06T23:35:31Z

+		if (chopped != null && chopped.length > 1) {
+			chop = chopped[1];
+		}
+		if (chop != null) {


Can't the body of this if be inside the body of the previous if (i.e. right under chop = chopped[1];)? chop can only not be null if that first if's condition is true. For that matter, you could also just declare chop inside of that as String chop = chopped[1]; since it's not going to be used outside of the if block.

You are correct. It turns out that the split actually returns an empty string if there is no token. So in the new commit I have also added and extra check to make sure 'chop' has length.

ocket8888 · 2019-03-06T23:52:00Z

      "certificate": {
        "comment" : "The following is just a self signed certificate and key to use for testing this is NOT private data from a CA",
-        "key": "LS0tLS1CRUdJTiBSU0EgUFJJVkFURSBLRVktLS0tLQpNSUlFb2dJQkFBS0NBUUVBc2Y0NnV5OGJ2\nQk5rMGhCaEVsbHdGT0dqREh6M1hJY1hteDRVNThNZG9Fa1JId0VTCjVONnd3NFV6bDAvRDcyMlJV\nODlMeHB4bldvclJmdVZNQldnOGVFcXBUb2NUS2NOZHhtZmdEUWZTcTZ1ODNTWkUKTmFCZFArK2g5\nYTJJRFZXWGFldVRhcVA3Q3lVVG52Sld5Mm1JalJWZkRGQWRWWHNhU1M4RGRYUWdibEJTelJ6NwpL\nMXFHVWt1RlZQc0R0ODZBYVF3TnN5R2ZDN3ltcUkzNU1FQ3hTdzNPd2lXSlAyZTg3U2E5UG9Pdjcr\nZUs2NVJnCmM3dzNkSXQxZUlyS3B6OWpQV1RPTkJOK0JhWFdvcHNXZ3UvdVd4Q1pnUk9qaXBWVUFK\nNHhrNFRGMjd0S28zRDUKSHd4RVZOeStlck9FTGI3VWxuaHMxaG1LdVI2RTZwREpHSyt1UFFJREFR\nQUJBb0lCQURkb1RYNFJkdysvOWNXUwpoYlZCbEN0YjVmYXdQQXhZblZlVE1Lekl4ME1yRzZKTUlr\nYU9yL1hkVkNjSkZKUkd4bE9SbHlRWGNmRDBmNnlCCjBuMW9hbEtENDFwbm4rYURRNERNdXlrelF6\naGZlS1piRUNhbGFnSEtKZCtsaWxHa1VFTVBxMDhxQnE3OGRyUW0KK2kvT0JVenQxeTJ0RHNTYVVw\nOXZmQ01tNkNXT1pKOHN6eWZrSk1ZZVVLdXptOWwzMGNNeW84anF0bi9LQjZWWApEVkZmNGpqU1FM\nTzBYbUo1M012TW1xUzlGaVRUdklpQm9iNnkxMEtWZjZpWk5MZnVnVExoWFhQaDFDNTJTUTVvCjZj\nVWdqanh5NFhmYUdub0Y1NlF4M0FIZWRpd0Z6eEFpWGlKOCswUFB3bWI2Q1FGOU1jSzV0QjQzcVZx\nNWNKNFcKTVF1SWpsMENnWUVBMkM3SFh6OHppODQ0anJHa1VvUmF3NE9QcitBdnFIVGY1NEJ2Ujhz\nTzc2NkljRzEvUytmdQoxbXQ2RnBRQVZGeHJvWmxEWHkrWGtTQ0toaEM3cmh6cHQzaVJuRkhkWFpT\nM25MM2ZjWnIzUCtFSXNyb3hLeVFRCmg3dy8xYnEzd3JwUnNOWjZpdDNVSHJvVlRLU2tZQzZvOUor\nYzRPYjVQRzVGcFRvZk81dmRWWHNDZ1lFQTBzYkYKd3pjYzEvcTdEVXpkTkVTaWtNMFdMMkFZanBH\nOFpEYTY0eUcrZ1JkWjhjWmpJUzlkVFpWWVpUODNFSnp2T2dyWgo1NmFYTVN2QVc2ak1oY1VDR05K\nN3JaaGl1Uk9HNVdWM3FzMnFkbmtkQlgrdGhrUE9HT3lNYnhsN1ltUlZXTG11CjBFV3NMQ3VHVFov\neDV5NG02WmNGQldRZUkreVMxanZTTXVkelVhY0NnWUJvVG1ISjlnK2o1Qk5yM2hCZjlCWnIKQVY1\naHlMU1YvaFpPZDZ5NW9pTUp5RmR5ajVKOUNHSTN1TkhHZFJDWG82UVc2NEVUT3o1Uk9yYzdxblV6\ndENXYwpiYU1zSGwvRm1Fempac1daNWVCb1JPYlNmWDNkeDkvbDdoR0t5VFdDMGkwNk9ySVRzS1o1\nVU9XWC9sU0ZSOTRqCmNhUGE2L2JUam8weUJKSXZTNndHWXdLQmdDN045dkpmbGFjY1JWY3h2MWt3\nK0l5QkRqRWMvTGNFQTdxWk1LenAKUEYxOEt2djJXdUx1bXFCMHpubEZMVndpRFRsdFdYQUlYVUNN\nLzUwYkFiZWV4TlZ3UUFpUGN6UzM4bGVVVFp0LwpLaUEreXNRQzB5eWlkK3l1OG94bE16SHBKODZa\nQlFtNHZ2L2I5bW5jWDZJL2JHS29wM1BJQksxamhrUE9hdUhrCjVZVzNBb0dBVEtVZlhkWTNad0hM\nWlUxSm1lRldKb29kOW10SHNrM1QyT2hCdDBwNUpSU3hVYWxOY0tkZEcvMVgKMFFCN1E1NWxMZEIz\nT1k2b1NDaEtuK1dUMWNwQ1N3d09PQlJNTmZPOVR1c252NkU2ZlNRWWVwdGNmV2lOb3VBWQpYaDVn\nQTRORzJHeUM5TW9tZUxQbUY0NmJsdU5UYzFxREMzeVZVcVQzdDkxY1RwNytWems9Ci0tLS0tRU5E\nIFJTQSBQUklWQVRFIEtFWS0tLS0tCg==",
+        "key": "LS0tLS1CRUdJTiBSU0EgUFJJVkFURSBLRVktLS0tLQpNSUlFcEFJQkFBS0NBUUVBMHBBWXlmamRz\nZ0NSL0ZXeEQvWE1vbDNwYjRWazlHSFdqTnR4bkdvWHA3OGxxM2p1CitLd012ZlU2ZDVScmdXbHp6\nMlJjZnFhMHdjbkYrNU1abkdkbzRGaGwycnhnT3ZBV2NSb3ZXS3BUaXNUUkcrQXcKbnFneUZTWjNT\nalhUcE9YUjV4dUJJOWI1c3ZIN3RhbzdBcWFLR2I0V3d1TnA3cTZzcUtRYlRxaUlXcE9JQWtFKwpS\nVnJXblBVdHRvaHlTV08yL2dIbDQ2NlU1S0czdC9TU3lqZVRPZ1ZFU0xoQUhlWlk2dExyTGd1YmdM\nanlLVWE5CkpDcWJLa1laZ2UrdWlhKzVVMzZ6alhYRUdBUEdwWWNrM2Zqb1pYN01zM3hkUzV6Zit6\nUDQzSnVVWUtheE5KbVMKRXMvaU1Dc1VPK1htZEtQTFhmWGdPWDNoM0NQdC83U05HR3VlS1FJREFR\nQUJBb0lCQUF1REEwZnZpamRVSHFjYwpERDBpSkJqd1ozWElaamVTTGNldnE2dHdoWENQVzhEZk1M\nbDV0b3lnSHAweENSdWZKMHk4WU80dnNRd3pPdGJCCk9SSTYrUm5pMjFhMUc5RzlGSTBFY0hnNWY2\nM0RpdWNxUDU0ODlkZ0FMVjlxUi9Mbythdlg3aHlHZ1VwT1BvTzEKRmRyVVBoS2dPT0JZekk3WEQr\ndDhaVjNNaXYvZ25aWUphR2dYRDYrODFGaU4veXNXaE5SMEVYYW5Vck1wR2ZtdQpSVm1tYnlvRjNr\nWVV1d3V2SUZIMkdPVFo0VjViR3JGQUJUQUowSWt2MkdmcU9TR08xa1NiWVMxclZjb29uMyswClRz\ncGxLejRUT3NVblp5VUk1UVBpbEI5ak93cjVlbGFRM1lndnZCM0k5TUdvQjRTLy9XWkFVRGFQTjlE\nZXYyU1UKUm1jb2dqMENnWUVBN3oxaEh0c21QZmVOOTA3VWV4clVwMGRMOEJtbmJ5V1k0bkYrbFhp\nalRLQ0xpTjdtNGFSNQpTTHhtZ21nVkhrQ0dOZkEybXFVWHA2dTlNL1BXOGx4SjRjNExLRldCbTVJ\nYjFRdlRZM3hOWDNKWVZYZkhTWkNlCitvaDMwaStmcm9oYmNnZzRpaG9QeDU2aGhpbEdlSXZGSS9l\nMDg0M0Z0dU9mWkgrQ0s1dHRKOU1DZ1lFQTRWQnEKTTNUUlhzbUU3ZHQvSFFza2ladGFjTVdkQldx\nOVRXVlBIb3hBK3VjalFoandGVFhBZzhkZGFsbE9TS1hWNjVGawoxMlphU1lTbTd5VnplS3lVRk4w\nazBxNXpRQkRZQ1FNYVA2YjZYbGxiL2VMaWJQQTMxbU5uRGl2WjdkVWM3UUVqClhUUy9nclFLc2Zl\nenRVMXUwNG1SMTZ5THhZSHh3MW5IL3NselFKTUNnWUVBMHF6S2lkR1NxNThFZFhRRnlTS24KZ1dk\nWGgrZ1BlZUV4OExiaE1kODZicEF5VUNWNlM5bjZ0QUswZ3NJRlZzNmJZWVJYa1hjd2pZYSs1ckVq\nNStrOQpab3Q3Wjlsa2VRc0JWMnRDaTZrNnVZS0lKenVEVTFUM3FzZmlQRVdUNks1TFdPL0VXbGo0\nN0dEVS9MLzhQc3RXCm40WFM0MmRGWlBpdHRHSlV6dkhmL2VFQ2dZQXVpR3dXaW5hL0s4RmZXbWly\nTitUbzRvUFFMSS9jVVlvZEZPSTkKUGR3aHRXREx3dGk2bUtwVXpQVFhCUEN0QWtybTV0VTd3ekM4\nWkVBUnZkdFdQZFlyWk95NDhqeHRLODFpTnhqUgpzb3VjdHJuUCttNm03d21wSmtoZlhlRVpSRjAv\nK1c4elRiU0xxdUZXbGdDd1hmaVlpWjNzTy85MTMvdHRTL3FJCi9WUG5Md0tCZ1FDNGQ4OXNUL3E3\nNjZMREt6bEhySzVEUkE2TTRENEhMbjEwTWJQVExNdWNweEJlQkY1YnhvQkEKeHRFcVhBRCtyaWVr\nT3IybjJSRHlxeGt0MS9FY2JhcTI5bmNXVFFwZjZ2NkV3cTFGT3JhUDlpdTJFbERBcUcyMApXbmdG\nOUttM0VETUpBQkd5VHhieFgxZTZMRG1LaFcwWFFxRWptVmpGQi9uMFVQOGxJd2F2aGc9PQotLS0t\nLUVORCBSU0EgUFJJVkFURSBLRVktLS0tLQo=",


What's the purpose of these key replacements? Was there something wrong with the old values? Seems odd that the key would change but not the cert.

These keys have been replaced specifically to cause the modulus validation check to fail in the integration tests. It sounds negative, but its actually a positive test.

ocket8888 · 2019-03-06T23:56:20Z

 ADD $RPM /
 ADD $TC_REPO /etc/yum.repos.d/
+ADD starttr.sh /
+ADD shutdowntr.sh /


How come? Are you docker execing into the container to start and stop Traffic Router? What do you gain by doing that without making any changes?

systemd has been disabled in the centos 7 docker container so we can no longer use the unit files to start/stop processes. The starttr.sh gets called by the run.sh, and yes, I do docker exec to run the shutdown. I also copy in new TR rpms so I can test code changes without having to rebuild the whole Docker image. We have done something similar in CIAB for the TR and ATS containers.

Well typically you're not supposed to do that with containers, but I'd like the opinion of someone who actually uses these things - @rob05c , thoughts?

I think this goes back to the whole "containers as VMs" vs "one process per container" thing, which really depends on how the container was designed. These scripts make it seem more like a "container as a VM". That said, I don't really see a problem with adding these scripts to this TR container, but it probably should've been done in a different PR since they don't seem to be required for adding TR SSL cert validation.

ocket8888 · 2019-03-07T00:00:28Z

 	private PrivateKeyDecoder privateKeyDecoder = new PrivateKeyDecoder();
 	private CertificateDecoder certificateDecoder = new CertificateDecoder();

+	@SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.AvoidDeeplyNestedIfStmts", "PMD.NPathComplexity"})


Again, I'm not a Java guy, but this strikes me as a potential codesmell (especially since it's done at the class and method levels - does the class-level one not apply to the methods?). If the cyclomatic complexity is high enough to issue a warning, maybe it should be reduced by breaking the method into parts?

I moved this down to where it belongs. I accidentally left it at the top while I was developing the solution.

Set the startup/shutdown scripts for the TR docker container to executable

asfgit · 2019-03-09T02:05:52Z

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

rawlinp · 2019-03-11T22:20:39Z

 ADD $RPM /
 ADD $TC_REPO /etc/yum.repos.d/
+ADD starttr.sh /
+ADD shutdowntr.sh /


I think this goes back to the whole "containers as VMs" vs "one process per container" thing, which really depends on how the container was designed. These scripts make it seem more like a "container as a VM". That said, I don't really see a problem with adding these scripts to this TR container, but it probably should've been done in a different PR since they don't seem to be required for adding TR SSL cert validation.

rawlinp · 2019-03-11T22:21:16Z

 import com.comcast.cdn.traffic_control.traffic_router.secure.HandshakeData;
 import com.comcast.cdn.traffic_control.traffic_router.secure.KeyManager;
 import org.apache.log4j.Logger;
+//import org.apache.tomcat.util.modeler.Registry;


this comment can be removed

done in latest commit

rawlinp · 2019-03-11T22:31:04Z

+        if (replace) {
+            previous = sslHostConfigs.get(key);
+        }
+        super.addSslHostConfig( sslHostConfig, replace);


there is an unnecessary space after the opening (

rawlinp · 2019-03-11T22:38:12Z

+			final List<X509Certificate> x509Chain = new ArrayList<>();
+			boolean hostMatch = false;
+			boolean modMatch = false;
+			for ( final String encodedCertificate : encodedCertificates) {


there is an unnecessary space after the opening (

rawlinp · 2019-03-11T22:41:45Z

+				}
+				x509Chain.add(certificate);
+			}
+			if ( hostMatch && modMatch) {


there is an unnecessary space after the opening (

rawlinp · 2019-03-11T22:48:55Z

+				log.warn("Service name doesn't match the subject of the certificate = "+certificateData.getHostname());
+			}
+			else if (!modMatch) {
+				log.error("Modulus not == for host: "+certificateData.getHostname());


This error message isn't super clear. How about "private key modulus does not match public key modulus for certificate host: " + ...

asfgit · 2019-03-12T06:12:33Z

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

rawlinp · 2019-03-12T16:05:20Z

I approved the code but still want to pull this down to test a little before merging.

rawlinp

Actually, do you mind adding a line to CHANGELOG.md for this change?

asfgit · 2019-03-12T19:42:24Z

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

* updated tr docker config so it would work * added start and stop scripts for TR docker containers * fixed bug in Tomcat which wasn't unregistering old SslHostConfigs * Added validation checks of SSL certs while being loaded by CertificateRegistry * Changed error message for Certificate Expired * Corrected a parsing error in certificate validation checking Set the startup/shutdown scripts for the TR docker container to executable * Corrected some code formatting and one NullPointerException * Added info to CHANGELOG.md (cherry picked from commit 0d2560d)

* Make SSL Cert lookup case-insensitive (#3331) * Fix case-sensitive SSL cert lookup * Set SSL certificate key names to lower case * Set requested SNI to lower case (cherry picked from commit 6504e7f) * Add SSL Certificate Validation to Traffic Router (#3380) * updated tr docker config so it would work * added start and stop scripts for TR docker containers * fixed bug in Tomcat which wasn't unregistering old SslHostConfigs * Added validation checks of SSL certs while being loaded by CertificateRegistry * Changed error message for Certificate Expired * Corrected a parsing error in certificate validation checking Set the startup/shutdown scripts for the TR docker container to executable * Corrected some code formatting and one NullPointerException * Added info to CHANGELOG.md (cherry picked from commit 0d2560d) * Corrected CHANGELOG * Another CHANGELOG correction

ASchmidt added 5 commits March 5, 2019 14:39

updated tr docker config so it would work

ea34df8

added start and stop scripts for TR docker containers

ce6ed87

fixed bug in Tomcat which wasn't unregistering old SslHostConfigs

7ae40c1

Added validation checks of SSL certs while being loaded by Certificat…

a13a769

…eRegistry

Changed error message for Certificate Expired

da25f4e

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

ocket8888 suggested changes Mar 6, 2019

View reviewed changes

ocket8888 suggested changes Mar 7, 2019

View reviewed changes

Corrected a parsing error in certificate validation checking

5165287

Set the startup/shutdown scripts for the TR docker container to executable

rawlinp suggested changes Mar 11, 2019

View reviewed changes

Corrected some code formatting and one NullPointerException

b1d89f0

rawlinp approved these changes Mar 12, 2019

View reviewed changes

rawlinp added this to the 3.0.1 milestone Mar 12, 2019

rawlinp suggested changes Mar 12, 2019

View reviewed changes

Added info to CHANGELOG.md

6d2da08

rawlinp approved these changes Mar 12, 2019

View reviewed changes

rawlinp merged commit 0d2560d into apache:master Mar 12, 2019

ghost mentioned this pull request May 22, 2019

Mixed case XML ID and Routing Name results in failed TLS handshake #3234

Closed

Conversation

ajschmidt commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Mar 9, 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!

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!

Uh oh!

Uh oh!

Uh oh!

asfgit commented Mar 12, 2019

Uh oh!

rawlinp commented Mar 12, 2019

Uh oh!

rawlinp left a comment

Choose a reason for hiding this comment

Uh oh!

asfgit commented Mar 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ajschmidt commented Mar 6, 2019 •

edited

Loading