Add enhanced X509 Certificate/Private RSA Key validation to Traffic Ops (AddSSLKeys Endpoint) · Pull Request #3382 · apache/trafficcontrol

ghost · 2019-03-06T19:39:15Z

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

Partially addresses #3267

All imported x509 certificates / private key pairs will be validated with the following criteria:

All PEM encoded strings contained in the CRT and KEY fields must be decodable to DER binary format via pem.Decode()
A single PEM encoded private RSA key must be decodable from KEY payload. KEY format must be PKCS update README.md - JIRA link #1 or PKCS experimental TOv2 UI - changes to handle ints (postgres) instead of strings.. #8
The import server X509 certificate in the CRT payload is checked for the appropriate Public Key Algorithm (x509.RSA is only supported currently)
Private RSA Key modulus (value N) is compared against the x509 cert RSA Public Key modulus value N. If both values do not match, x509 certificate and key are rejected with httpStatus 4XX.
Original certificate Chain validation functionality has been left in-place.
Ability to import x509v3 Certificates without SKI/AKI has been left in-place.

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

Tests to perform for validation:

Import x509 certificate with mismatched RSA Private Key

Expected Result: x509 certificate and key are rejected and not saved in traffic vault.

Import x509 certificate with encrypted RSA Private Key

Expected Result: x509 certificate and key are rejected and not saved in traffic vault.

Attempt to import ECC x509 certificate and ECDSA private key

Expected Result: x509 certificate and key are rejected and . not saved in traffic vault.

Import self-signed certificate for a delivery service with matching Private RSA key.
Optional: Startup CiaB with this commit cherry-picked in.
Run the unit tests in deliveryservice/keys_test.go

=== RUN   TestDecodePrivateKey_PKCS8_RSA_2048
--- PASS: TestDecodePrivateKey_PKCS8_RSA_2048 (0.00s)
=== RUN   TestDecodePrivateKey_PKCS1_RSA_2048
--- PASS: TestDecodePrivateKey_PKCS1_RSA_2048 (0.00s)
=== RUN   TestDecodePrivateKey_RSA_Encrypted_And_BadData
--- PASS: TestDecodePrivateKey_RSA_Encrypted_And_BadData (0.00s)
=== RUN   TestVerifyAndEncodeCertificate_BadData
--- PASS: TestVerifyAndEncodeCertificate_BadData (0.00s)
=== RUN   TestVerifyAndEncodeCertificate_SelfSigned_NO_SKIAKI_CertKeyPair
--- PASS: TestVerifyAndEncodeCertificate_SelfSigned_NO_SKIAKI_CertKeyPair (0.15s)
=== RUN   TestVerifyAndEncodeCertificate_SelfSignedCertKeyPair
--- PASS: TestVerifyAndEncodeCertificate_SelfSignedCertKeyPair (0.00s)
=== RUN   TestVerifyAndEncodeCertificate_SelfSignedCertKeyPair_MisMatched_PrivateKey
--- PASS: TestVerifyAndEncodeCertificate_SelfSignedCertKeyPair_MisMatched_PrivateKey (0.00s)
=== RUN   TestVerifyAndEncodeCertificate_CASignedCertKeyPair
--- PASS: TestVerifyAndEncodeCertificate_CASignedCertKeyPair (0.00s)
=== RUN   TestVerifyAndEncodeCertificate_CASigned_NO_SKIAKI_CertKeyPair
--- PASS: TestVerifyAndEncodeCertificate_CASigned_NO_SKIAKI_CertKeyPair (0.00s)
=== RUN   TestVerifyAndEncodeCertificate_CASigned_NO_SKIAKI_CertKeyPair_With_RootCA
--- PASS: TestVerifyAndEncodeCertificate_CASigned_NO_SKIAKI_CertKeyPair_With_RootCA (0.00s)
=== RUN   TestVerifyAndEncodeCertificate_CASignedCertKeyPair_With_RootCA
--- PASS: TestVerifyAndEncodeCertificate_CASignedCertKeyPair_With_RootCA (0.00s)
=== RUN   TestVerifyAndEncodeCertificate_ECC_CertificateKeyPair
--- PASS: TestVerifyAndEncodeCertificate_ECC_CertificateKeyPair (0.00s)
PASS
ok  	command-line-arguments	(cached)

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-06T20:47:31Z

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

ocket8888 · 2019-03-07T00:02:30Z

If this PR only partially fixes that issue, you should change "Fixes" to e.g. "Addressed" - As-is, GitHub's going to auto-close the issue when this gets merged.

ocket8888

It'd be nice if you could add something to the deliveryservices/sslkeys/add API endpoint docs about supported algorithms/chains/whatever else you think is appropriate.

rob05c

Looks good, only things are making sure EC certs are still allowed, and language idioms.

Tested in a CiaB, works as expected, except that EC certs aren't allowed.

~~We do support EC and DSA certs in TC~~, this needs to check and allow those as well. Other than that, mostly just language idiom and format things, if you don't mind fixing those.

That said, most of our code in TC allows any cert the libraries we're using support (Go, Java, Perl, etc). It does seem like we might be making things harder for ourselves in the future, by whitelisting instead of blacklisting. If we just check for invalid things we know about, we'll support new cert types that our libraries add for free. If you could make this allow types that we don't know about, and just return errors for known bad things, I think that would be even better.

Once the idioms and EC support are added, I'll test EC certs in the CIAB again, and it should be good to merge.

EDIT: Hm, looks like I might be mistaken about EC support. It works on Edges, but it looks like TR might have trouble with them. Still, we should support them, they're smaller and more secure. How would you feel about adding a warning, but allowing them? So when TR is fixed, we don't have to also fix TO?

ghost · 2019-03-08T17:30:58Z

Thanks for reviewing this PR @ocket8888 @rob05c. I'm working on getting these requested changes in today.

asfgit · 2019-03-08T23:18:18Z

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

asfgit · 2019-03-08T23:23:57Z

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

ghost · 2019-03-09T00:08:33Z

I'm adding some extra logic at the end of the certificate unit tests that check for correct number of certs that should be in the cert chain. Unit tests for non-SKIAKI certificates do not have the correct number of items in the returned certificate bundle.

asfgit · 2019-03-09T01:10:04Z

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

asfgit · 2019-03-09T01:16:51Z

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

asfgit · 2019-03-11T19:55:31Z

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

Sometimes, the Certificate.Verify method will remove elements from a valid certificate chain. Instead of using the output of that method, always use the user input certificate and return a warning if the input certificate does not match the output of the Certificate.Verify method. Fixes #3398 (cherry picked from commit dde742e)

… services

ghost · 2019-03-29T20:39:39Z

I have forced pushed a branch that combines PR #3382 with PR #3417 and adds ECDSA support for DNS delivery services.
TODO:

Add in the SQL query that limits ECDSA certs to DNS delivery services only.
Fix the unit tests for the ECC certificate/key pair import use case.
Fix up the docs to be 'go doc' format.

asfgit · 2019-03-29T20:45:52Z

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

asfgit · 2019-03-30T02:56:58Z

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

asfgit · 2019-03-30T03:19:51Z

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

asfgit · 2019-03-30T03:32:52Z

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

…em block

asfgit · 2019-03-30T04:12:04Z

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

Add unit test for encrypted ECDSA private key

asfgit · 2019-03-30T04:44:50Z

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

asfgit · 2019-03-30T05:04:38Z

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

Add ECDSA mismatched cert/key unit test Update error messages to be more meaningful

asfgit · 2019-03-30T19:24:56Z

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

Missing RSA keyEncipherment unit test Missing ECDSA digitalSignature unit test Missing serverAuth extendedKeyUsage (x509v3 only) unit test

asfgit · 2019-03-30T20:26:37Z

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

asfgit · 2019-03-31T04:14:47Z

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

asfgit · 2019-03-31T18:44:59Z

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

asfgit · 2019-03-31T21:24:40Z

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

asfgit · 2019-03-31T22:29:46Z

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

ghost · 2019-04-01T17:27:22Z

Latest branch provides the following changes:

Merge in PR CLOSED - Fix add sslkeys endpoint to always use the input certificate #3417
Adds ECDSA support for only DNS* delivery service types.
Verification that both RSA and ECDSA private keys match the corresponding submitted x509 certificate. RSA verification ensures the public modulus N value matches the modulus N value in the x509 certificate. ECDSA verification ensures the curve set {name, public-X, public-Y} curve values are equal to those in the x509 certificate.
There are 27 unit tests that verify both x509v3 and x509v1 certificate/key combinations (look at keys_test.go). Yes I'm aware the documentation is lacking, but it doesn't make sense for me to write a ton of docs if this PR gets rejected. I can update the docs once this PR has been tested and the solution is accepted.
Verification that ECDSA keys are only permitted on DNS* delivery services explicitly.
Reject all x509 + DSA certificates because no modern HTTPS client supports it

rawlinp

Nice work on this PR!

For comments I've prefixed with nit: feel free to tell me you'd like to leave it how it is if you feel strongly about it. The other few comments I believe should be addressed before this is merged.

Minor changes to error message formatting and string comparison Re-sort go import statements

ghost · 2019-04-02T18:06:07Z

@rawlinp I pushed four commits that resolve all comments you raised from yesterday's review.

asfgit · 2019-04-02T18:09:50Z

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

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

rob05c self-assigned this Mar 6, 2019

ocket8888 suggested changes Mar 7, 2019

View reviewed changes

rob05c suggested changes Mar 7, 2019

View reviewed changes

rawlinp mentioned this pull request Mar 18, 2019

CLOSED - Fix add sslkeys endpoint to always use the input certificate #3417

Closed

17 tasks

Rawlin Peters and others added 2 commits March 29, 2019 11:37

Combine PR #3382 with PR #3417 and add ECDSA support for DNS delivery…

8e26731

… services

Update unit tests to fail faster

d3162c0

Update comments related to RSA/ECDSA key mismatch verification

d8d902c

Make decoding ECDSA PEM blocks more readable

303581a

Add Unit tests for ECDSA privateKey decoding with and without param p…

f6aa5e0

…em block

Rename Unit test methods.

0b0d877

Add unit test for encrypted ECDSA private key

Add unit test to verify DSA signed x509 certificates are rejected

15a8a41

Gofmt

6bd4977

Fix ECDSA unit tests

b892971

Add ECDSA mismatched cert/key unit test Update error messages to be more meaningful

Add more unit tests related to critical x509 cert/key validation

b4da1d8

Missing RSA keyEncipherment unit test Missing ECDSA digitalSignature unit test Missing serverAuth extendedKeyUsage (x509v3 only) unit test

Jeffrey Bevill added 2 commits March 30, 2019 22:04

Add self-signed x509v1 server certificate unit test.

9ce939e

Remove redundant error message header.

8c78afe

Revert change, gofmt

25c407e

Use getDSType() to permit ECDSA keys for only DNS* DS types.

6e89291

rawlinp suggested changes Apr 1, 2019

View reviewed changes

Jeffrey Bevill added 3 commits April 2, 2019 10:55

Update tc.DSType.IsDNS() method to only include DNS DS types.

318dfd5

Use updated tc.DSType.isDNS() method to determine if DS is a DNS type.

e6e80d5

Check errors returned from rsa/ecdsa private key decoding methods

fc09c42

Minor changes to error message formatting and string comparison Re-sort go import statements

rawlinp approved these changes Apr 3, 2019

View reviewed changes

rawlinp merged commit 55690d7 into apache:master Apr 3, 2019

This was referenced May 22, 2019

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

Closed

TO API - DS ADDSSLKeys X509 Certificate/Key Import Validation Enhancements. #3267

Closed

Conversation

ghost commented Mar 6, 2019 • edited by ghost 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!

ocket8888 commented Mar 7, 2019

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rob05c left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Mar 8, 2019

Uh oh!

asfgit commented Mar 8, 2019

Uh oh!

asfgit commented Mar 8, 2019

Uh oh!

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

Uh oh!

Uh oh!

asfgit commented Mar 9, 2019

Uh oh!

asfgit commented Mar 9, 2019

Uh oh!

asfgit commented Mar 11, 2019

Uh oh!

ghost commented Mar 29, 2019

Uh oh!

asfgit commented Mar 29, 2019

Uh oh!

asfgit commented Mar 30, 2019

Uh oh!

asfgit commented Mar 30, 2019

Uh oh!

asfgit commented Mar 30, 2019

Uh oh!

asfgit commented Mar 30, 2019

Uh oh!

asfgit commented Mar 30, 2019

Uh oh!

asfgit commented Mar 30, 2019

Uh oh!

asfgit commented Mar 30, 2019

Uh oh!

asfgit commented Mar 30, 2019

Uh oh!

asfgit commented Mar 31, 2019

Uh oh!

asfgit commented Mar 31, 2019

Uh oh!

asfgit commented Mar 31, 2019

Uh oh!

asfgit commented Mar 31, 2019

Uh oh!

ghost commented Apr 1, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ghost commented Mar 6, 2019 •

edited by ghost

Loading

rob05c left a comment •

edited

Loading

ghost commented Mar 9, 2019 •

edited by ghost

Loading

ghost commented Apr 1, 2019 •

edited by ghost

Loading