-
Notifications
You must be signed in to change notification settings - Fork 344
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
Traffic Ops client certificate authentication #7392
Conversation
…th main functions
Codecov Report
@@ Coverage Diff @@
## master #7392 +/- ##
============================================
+ Coverage 27.45% 29.97% +2.51%
Complexity 98 98
============================================
Files 685 785 +100
Lines 77786 82087 +4301
Branches 90 790 +700
============================================
+ Hits 21357 24604 +3247
- Misses 54417 55413 +996
- Partials 2012 2070 +58
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 100 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Some of these need conversation and some need updating.
traffic_ops/app/conf/cdn.conf
Outdated
} | ||
}, | ||
"client_certificate_authentication" : { | ||
"root_certificates_directory" : "/etc/pki/tls/certs/" |
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.
This is a problematic default. This will default to accepting certs signed by any of the default CAs, which is not what anyone is likely to want here. I would probably leave this undefined so the default "fails closed" with no acceptable roots.
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 a big fan of leaving it completely empty, because that make the feature less usable. Changed to /etc/pki/tls/traffic_ops
in 3e3873e8f0, would that work?
|
||
// LoginHandler first attempts to verify and parse user information from an optionally | ||
// provided client TLS certificate. If it fails at any point, it will fall back and | ||
// continue with the standard submitted form authentication. | ||
func LoginHandler(db *sqlx.DB, cfg config.Config) http.HandlerFunc { |
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.
This handles certificates as a replacement for uid/pass credentials, but not as a replacement for the mojo cookie. Given that the certificate can be seamlessly provided on every request, asking clients to use the login method to obtain a token that is then used instead of the certificate seems to create complexity where it isn't required. Additionally, certificates can ideally enable flows that do not use tokens, which prevents tokens from being stolen. The private key of a certificate isn't transmitted, so even if the certificate were leaked, it couldn't be used.
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.
This handles certificates as a replacement for uid/pass credentials, but not as a replacement for the mojo cookie. Given that the certificate can be seamlessly provided on every request, asking clients to use the login method to obtain a token that is then used instead of the certificate seems to create complexity where it isn't required. Additionally, certificates can ideally enable flows that do not use tokens, which prevents tokens from being stolen. The private key of a certificate isn't transmitted, so even if the certificate were leaked, it couldn't be used.
You're right that this is a good idea, I opened #7498 for it. I'll make sure we implement it, but it should not block #7392 being accepted, IMO.
c4f6d64
to
54b9c15
Compare
(Rebased) |
ba92b20
to
3e3873e
Compare
- Only fail when the UID is empty if no UID was found
409ecce
to
e9ad3c0
Compare
3aac7a2
to
bd0d7fd
Compare
Rebased to get #7489 |
bd0d7fd
to
73409b2
Compare
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.
This is good stuff. I see the open ticket to track the larger work getting the certs to work on pages other than the login page. That's important work, but it doesn't need to block this.
Overview
(Largely reposting @tcfdev's PR description from #7110):
This PR implements #6622 by adding the ability for a Traffic Ops instance to accept TLS certificates from a client request and verify them against specified Root CA's certificate as a form of login. This is not to be confused with mTLS, albeit has a similar design. Should a client not send a TLS certificate as part of the request login functionality will default to standard form authentication (current implementation).
Client
The client will provide a TLS certificate (and intermediate(s)) when hitting the
/user/login
endpoint.The client will need to attach a certificate (and intermediate(s)) that was provided by the Root CA. The client certificate will need to contain a UID Relative Distinguished Name in the DN for the x509 certificate. The object identifier for this field is:
0.9.2342.19200300.100.1.1
This will result in a subject that looks something like:
Traffic Ops
In
cnd.conf
there is a new section to define the location of the Root CA certificates that are used for verification.cdn.conf
Additionally, to enable a line must be added to the
traffic_ops_golang.tls_config
section if it exists. If this section is not present, ClientAuth will be enabled (but not required) by default.cdn.conf
Traffic Ops does not require the client TLS certificate to be present. And will not verify the client provided TLS certificates during the TLS handshake when establishing a connection. Only if the client sends a TLS certificate to the
/user/login
will it be processed. If the certificate (and intermediate(s)) provided by the client verifies correctly against the defined Root certificates, the UID field will be parsed. If there is more than 1 UID field, only the first one is accepted. Order is not guaranteed. The UID value will then be used for authentication.Should the TLS certificate authentication fail at any point, Traffic Ops will attempt to perform form value authentication (username and password) which is the current functionality. TLS client certificate authentication is not present on Oauth or Token login endpoints currently.
Which Traffic Control components are affected by this PR?
What is the best way to verify this PR?
Unit
Verify unit tests pass. Unit tests have been added to test TLS authentication. Existing unit tests ensure current default behavior continues to work as expected.
Manual
Test certificates can be created using the file located at
trafficcontrol/experimental/certificate_auth/generate_certs.go
. Runninggo run generate_certs.go
will produce private keys and certificates for Root, Intermediate, and Client (Server cert/key is also created, but can be ignored since they too are used only in tests). Place the Root certificate in the directory location specified in thecdn.conf
file.Launch a Traffic Ops instance.
Ensure a user exists with appropriate permissions with the name
userid
(This can be changed in thegenerate_certs.go
file if you want to use a different username).Send a request to the
user/login
Traffic Ops with the Client and Intermediate certs. For a Go client this would look something like:client.go A more complete version can be found at
trafficcontrol/experimental/certificate_auth/example/client.go
Upon success, a 200 OK status code will be returned along with the following body:
Additional tests may also be performed if desired, such as sending a POST request with a username/password body and no certificate (current behavior). Or a bad cert (either malformed or not signed by the correct Root CA) with or without username/password form.
PR submission checklist