Skip to content
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

Mutual TLS support #1271

Closed
wants to merge 24 commits into from
Closed

Mutual TLS support #1271

wants to merge 24 commits into from

Conversation

MikailBag
Copy link
Contributor

This is #657 (by @Kmoneal) rebased on master.
Currently, it compiles and passed cargo test.
As a next step, I will apply all suggestions from reviews on that PR.

Kmoneal and others added 24 commits April 9, 2020 15:06
* Add an optional configuration for Rocket.toml, ca_certs, to take
  in a directory and use it for MTLS.
* Update Cargo.toml to point to fork of hyper-sync-rustls with updates
  for MTLS.
* Save peer certificates from network stream to Data
* Add peer_certs field to Request
* Move certificates from Data to Request
* Lookup domain name associated with client's IP Address.
* Verify that the domain name match the certificate common name.
* Clean up code.
* Added better comments.
* Make cert_store_path optional.
* Modify code sample to reflect changes.
* Move mtls.rs contents into tls.rs.
* Parse certificat into MutualTlsUser.
* Create getter methods for MutualTlsUser.
* Generate MutualTlsUser from first accepted certificate from
  array the client provides.
* Add more comments explaining sections of code.
* Add documentation and examples.
* Remove public key and signature from MutualTlsUser.
* Improve error handling to not panic when generating a new
  MutualTlsUser.
* Replace unwraps with exceptions to specify what failed.
* Combine lines of code that can be together and simplify
  get_not_before and get_not_after method names.
* Remove methods referenced in comments that are no longer
  implemented.
* Use combinators instead of explicit matches where possible
* Remove some clone() calls
* Return references instead of copies
This keeps the internals of the name validation out of the
`from_request` logic for `MutualTlsUser`, which is currently still in
the core rocket lib to avoid circular dependencies.
Even without exposing the certificate details, MutualTlsUser provides a
guard that only allows authenticated clients to connect. Removing the
certificate parsing will allow this functionality to be added before all
the details of parsing the certificates have been figured out.

* Remove all fields and methods from MutualTlsUser
* Remove openssl dependency
* Update tests
This is not necessarily the value stored in the subject name of the
certificate, but it is the name for which the provided certifcate was
validated.
@MikailBag
Copy link
Contributor Author

I don't know why mtls example is failing on Windows. It doesn't seem to contain platforms-specific code.

@MikailBag
Copy link
Contributor Author

Taking look on https://github.com/SergioBenitez/Rocket/blob/c0a0e46d7e9b007ce60d2e2ba818990771e3a0d8/core/lib/src/request/from_request.rs#L478
I am not sure this code is secure.
It seems to me that only validation in that code is verify_is_valid_for_dns_name. According to webpki docs, verify_is_valid_tls_client_cert and verify_signature are required too.

It is also possible I am wrong, and verify_is_valid_for_dns_name is enough, so I would like to get some clarification on this.

@Kmoneal
Copy link

Kmoneal commented Apr 14, 2020

I will need to look into this again. I don't see it hurting by adding the signature verification even if it is not needed.

Just a clarification of what was the original roadblock since I don't think this was every put on github. There was originally a dilemma of the cert list being used and not having a specific source of what DNS name the other server was providing. I don't know now, but that was a problem stemming from the library for certificates not specifying the DNS name and instead sending through a list of names provided in the certificate.

I have very limited time right now but would like to help as best I can.

@SergioBenitez SergioBenitez marked this pull request as draft June 5, 2020 01:33
@SergioBenitez
Copy link
Member

Closing due to inactivity, but I'd still love to see this implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants