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

Add MTLS Support #657

Closed
wants to merge 24 commits into from
Closed

Add MTLS Support #657

wants to merge 24 commits into from

Conversation

Kmoneal
Copy link

@Kmoneal Kmoneal commented Jun 8, 2018

Requires hyper-sync-rustls pull request.

TLS requirements have to be met with an additional parameter ca_certs. ca_certs will take in a directory to generate a certificate store to be used for MTLS.

@SergioBenitez
Copy link
Member

SergioBenitez commented Jun 8, 2018

First of all, thanks so much for taking on this challenge! I'm really excited to have MTLS in Rocket, and I'd love to work with you to get this in.

At the moment, there are a few important issues that I can spot, besides the PR needing to be rebased on the latest master.

First, it looks like it's impossible to configure the server without MTLS enabled with this changeset. That isn't an acceptable compromise as the majority of users will not want to use MTLS. Please correct me if I've overlooked something here.

Second, it looks like with this implementation, MTLS is all-or-nothing: either the entire server (and all endpoints) require MTLS authentication, or none of the endpoints require MTLS authentication. In practice, many web services that use MTLS will want to authenticate with MTLS on some endpoints but not others. I do not feel comfortable merging a PR that is all-or-nothing.

Third, I don't see a mechanism to obtain the authenticated credentials submitted over MTLS. At the moment, it looks like you get half a bit of information: if you receive a request, then MTLS succeeded, otherwise, there were no connections or MTLS failed. While this may suffice for some applications, richer uses will require the information of the authenticated client.

In short:

  • MTLS must be an optional feature. TLS should be useable without MTLS.
  • MTLS must be configurable per-route.
  • MTLS should also be optionally enabled on the entire application.
  • The credentials of an authenticated user must be exposed to the application.

The way I had envisioned MTLS support being exposed in Rocket is via a request guard and, optionally, a fairing. After configuring MTLS in Rocket.toml:

[global.tls]
mutual_ca_certs = "path/to/CA/certs/dir/"

A MutualTlsUser request guard can be used on any route to require that the client present a valid certificate. The guard only succeeds if MTLS authentication succeeds. The guard exposes the information from the presented certificate:

#[get("/sensitive")]
fn sensitive(user: MutualTlsUser) -> String {
    // `MutualTlsUser` contains methods to access certificate data
    open_sensitive_data(user.name());
}

Alternatively, a fairing can be used to secure all of the application or subsets of it:

// secure the entire application
rocket::ignite().attach(MutualTls::fairing())...

// secure everything under '/secure/'; this functionality isn't required for this PR
rocket::ignite().attach(MutualTls::fairing().subset("/secure"))

I'd be more than willing to accept a PR that implements MTLS in this or a similar fashion.

@akuanti
Copy link

akuanti commented Jun 8, 2018

@SergioBenitez in the fairing case, would the routes still have access to the certificate information, or would each route that wanted to use that information also need to use a request guard?

@Kmoneal
Copy link
Author

Kmoneal commented Jun 11, 2018

The changes made allow MTLS or TLS based on if the certificate store directory is given or not.

I will start working on the implementing those changes. Thank you for the feedback!

@SergioBenitez
Copy link
Member

@akuanti Each route would have access to the certificate information, likely through a guard. This "doubling-up" should have no performance penalty once #655 lands.

@akuanti akuanti force-pushed the feature/mtls branch 2 times, most recently from 3a2bd4b to a2a4861 Compare June 25, 2018 22:28
@SergioBenitez
Copy link
Member

Please see the build failure.

@akuanti
Copy link

akuanti commented Jun 26, 2018

The build fails because it depends on changes in SergioBenitez/hyper-sync-rustls#4. Should I modify the Cargo.toml to point to that version?

@SergioBenitez
Copy link
Member

Yes. Please temporarily modify Cargo.toml to point to any version you'd like.

Copy link
Member

@SergioBenitez SergioBenitez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, this PR lacks the polish necessary to be pulled in. That being said, this is much more inline with something that would be pulled in, so please take these requests as encouragement!

Before the PR is pulled in, we'll also need some full-blown examples (in /examples).

/// # ; /*
/// .unwrap();
/// # */
/// ```
pub fn tls<C, K>(mut self, certs_path: C, key_path: K) -> Self
where C: Into<String>, K: Into<String>
pub fn tls<C, K, W>(mut self, certs_path: C, key_path: K, cert_store_path: W) -> Self
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cert_store_path isn't optional in this builder, but it should be.

@@ -21,7 +21,7 @@ pub struct ConfigBuilder {
/// The secret key.
pub secret_key: Option<String>,
/// TLS configuration (path to certificates file, path to private key file).
pub tls: Option<(String, String)>,
pub tls: Option<(String, String, String)>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the other TLS options can be specified without the mutual TLS option, then all three shouldn't be in a single Option. You should have another field just for mutual TLS that is an Option.

} else {
Err(conf.bad_type(name, "a table with missing entries",
"a table with `certs` and `key` entries"))
"a table with `certs`, `key`, and `ca_certs` entries"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably the ca_certs key isn't required.

@@ -312,3 +314,14 @@ impl<'a, 'r, T: FromRequest<'a, 'r>> FromRequest<'a, 'r> for Option<T> {
}
}

#[cfg(feature = "tls")]
impl <'a, 'r> FromRequest<'a, 'r> for MutualTlsUser {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation should be where the MutualTlsUser struct is defined.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to do this without having rocket_http depend on rocket? Or does that not matter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sure. That's pretty unfortunate. Okay, it's fine here.

@@ -27,6 +27,9 @@ pub mod uri;
#[cfg(feature = "tls")]
pub mod tls;

#[cfg(feature = "tls")]
pub mod mtls;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not put everything from mtls into the existing tls module?

use tls::Certificate;

#[derive(Debug)]
pub struct MutualTlsUser {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need docs. Also, we need nice accessor methods for all of the things that will be commonly desired.

self.tls = Some(TlsConfig { certs, key, ca_certs: None });
return Ok(());
};
let ca_cert_vector = util::load_cert_store_certs(self.root_relative(cert_store_path.unwrap()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will presumably panic if a certificate store isn't passed in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please follow the spacing/doc conventions from the code above.

Error::Io(e) => ConfigError::Io(e, "tls.ca_certs"),
_ => self.bad_type("tls", pem_err, "a valid certificate store directory")
})?;
let ca_certs = Some(util::generate_cert_store(ca_cert_vector)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad indentation.

@@ -227,8 +228,8 @@ pub fn log_level(conf: &Config,
pub fn tls_config<'v>(conf: &Config,
name: &str,
value: &'v Value,
) -> Result<(&'v str, &'v str)> {
let (mut certs_path, mut key_path) = (None, None);
) -> Result<(&'v str, &'v str, Option<&'v str>)> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here with separating tls configs and mtls configs.

@SergioBenitez
Copy link
Member

Ping.

@akuanti akuanti force-pushed the feature/mtls branch 2 times, most recently from 9806a9c to 932f5da Compare July 17, 2018 21:51
@SergioBenitez
Copy link
Member

@akuanti @Kmoneal Is this ready for another round of review?

@akuanti
Copy link

akuanti commented Jul 23, 2018

@SergioBenitez I'm not sure if we've properly addressed everything you mentioned, so yes, another review would be much appreciated.

Copy link
Collaborator

@jebrosen jebrosen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from some of the code style, mostly for handling the new Option value inside the config, I am concerned about the validation and the added dependencies. I would like to see an explanation for these implicit assumptions I currently see in the code:

  • Connections must come from a valid domain name that is mentioned in the client's certificate. I would like to know what specification recommends this, if any, or what the motivation is for this requirement.
  • The only information a route needs out of a certificate is a list containing the Common Name and all Subject Alternate Names, and that they are interchangeable.

I am also worried about the openssl dependency. I believe this can and will complicate compilation and deployment for some targets, especially on Windows and for targets such as musl where static linking is common. It introduces version constraints that have made ring difficult to deal with in the past. It's also only used for certificate parsing, which seems like something that another smaller library could provide.

dns-lookup = "0.9.1"
untrusted = "0.6.1"
webpki = "0.18.0-alpha4"
openssl = "0.10.10"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds several new dependencies, and openssl links a native library. These dependencies should be behind the "tls" feature, or a new "mtls" feature, or even dropped where possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rocket simply can't depend on openssl because of these issues. Whatever is being used here from openssl can almost certainly be found in ring, rustls, or another ring based library. We should use one of those instead.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it just the openssl dependency that is a problem, or are there other dependencies you don't like here?

Copy link
Collaborator

@jebrosen jebrosen Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

webpki is fair, and rustls already depends on it. As @SergioBenitez mentioned in his top-level comment, the code that uses webpki should probably be in hyper-sync-rustls and not here.

untrusted is needed to interface with webpki.

dns-lookup, or something similar, is apparently necessary to validate the client. webpki's documentation leads me to believe this should actually be done by rustls itself, but it currently isn't. I haven't looked at the code in dns-lookup, but its dependencies don't look too problematic.

pub use self::untrusted::Input;
pub use self::webpki::{EndEntityCert, DNSNameRef};

use self::openssl::x509::X509;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggests openssl is used only internally and only to parse X509 certificate data. webpki also parses the data to validate it, but I don't see anything other than validation in its API. Is there any way to avoid the openssl dependency?

@@ -21,7 +21,7 @@ pub struct ConfigBuilder {
/// The secret key.
pub secret_key: Option<String>,
/// TLS configuration (path to certificates file, path to private key file).
pub tls: Option<(String, String)>,
pub tls: Option<(String, String, Option<String>)>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new mtls configuration is still part of the tls config, which makes for some very awkward match constructs later in the code. On the other hand, keeping this path specified in the tls config section might look a bit nicer. I've suggested ways to clean up the matches assuming this field stays like this.

@SergioBenitez: did you want this split out into another field entirely, or just made optional?

// Load certs for clients.
if cert_store_path == None {
self.tls = Some(TlsConfig { certs, key, ca_certs: None });
return Ok(());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the added code in this function be restructured to avoid an early success return? Perhaps

if let Some(path) = cert_store_path {
    /* load certs... */
} else {
    self.tls = Some(TlsConfig { certs, key, ca_certs: None });
}
Ok(())

pub fn tls<C, K>(mut self, certs_path: C, key_path: K) -> Self
where C: Into<String>, K: Into<String>
pub fn tls<C, K, W>(mut self, certs_path: C, key_path: K, cert_store_path: Option<W>) -> Self
where C: Into<String>, K: Into<String>, W: Into<String>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If cert_store_path becomes W, where W is Into<Option<String>>, I think this match can be greatly simplified.

Alternatively, leave W as is and use cert_store.map(String::from) instead of the match.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

W: Into<Option<String>> sounds right to me.

config.set_tls(&certs_path, &key_path)?;
if let Some((certs_path, key_path, cert_store_path)) = self.tls {
match cert_store_path {
Some(cert_store) => config.set_tls(&certs_path, &key_path, Some(&cert_store))?,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, this can probably use cert_store_path.map(String::as_str) to avoid the match.

if let (Some(certs), Some(key), Some(ca_certs)) = (certs_path, key_path, cert_store_path) {
Ok((certs, key, Some(ca_certs)))
} else if let (Some(certs), Some(key), None) = (certs_path, key_path, cert_store_path) {
Ok((certs, key, None))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it can be a single if let.

};

// Compare certificate is valid for DNS name
let _verification = match end_entity.verify_is_valid_for_dns_name(common_name) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this is being done. The documentation for webpki explains how to use this and other functions to verify that a server's credentials are correct; this code appears to verify that a reverse lookup of the client's IP address matches the domain name specified on the client certificate.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that I had been reading old documentation for webpki. The up-to-date documentation does explain client verification, some of which is done by rustls, so it's clear to me why this is being done here.

};
return Success(mtls_user);
}
Forward(())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this whole thing can be simplified and shortened using Option/Result combinators instead of straight matches. I also suspect some of these ought to be returning Failures with a 4xx status code, instead of Forwards.

Copy link
Member

@SergioBenitez SergioBenitez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to everything from @jebrosen's review (Thanks, Jeb!), here are a few higher-level pieces of feedback:

  1. The additional crypto-ish dependencies likely should be elsewhere. That is, whatever functionality is being implemented using those libraries should likely be implemented in another library (maybe ring, or rustls or webpki or even hyper-sync-rustls) and exported as a high-level API. That API should then be used by Rocket.
  2. There's a lot of unnecessary clone() going on. In general, you want to let the user decide if they want an owned or borrowed version of a thing, and the only way to do that is to return the borrowed version and let them call .clone() or .into_owned() or what-have-you.
  3. As @jebrosen states, a lot of the code surrounding values of Option or Result types can be significantly simplified by using combinators (yay, almost monads) on these types as well as ?.

Functionally, this is stellar work. I'm super excited to have this in Rocket. Thank you for your time and effort!

impl MutualTlsUser {
pub fn new(peer_cert: Certificate) -> Option<MutualTlsUser> {
// Generate an x509 using the certificate provided
let x509 = match X509::from_der(peer_cert.as_ref()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use:

let x509 = X509::from_der(peer_cert.as_ref()).ok()?

Better yet, it would be nice if the error value could be propagated upwards. This would mean changing the signature to Result<MutalTlsUser, Error>.

};

// Retrieve alt names and store them into a Vec<String>
let alt_names = match x509.subject_alt_names() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

};
let mut common_names = Vec::new();
for name in alt_names {
let name = match name.dnsname() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire body is common_named.push(name.dnsname()?.to_string()).

#[derive(Debug)]
pub struct MutualTlsUser {
common_names: Vec<String>,
not_before: String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these being stored as strings? A String is not the appropriate type for time values because it doesn't support the operations we'd like to do on time values, in particular, comparison.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be appropriate here: chrono::DateTime, an integer, something else?

/// let cert_common_names = mtls.get_common_names();
/// }
/// ```
pub fn get_common_names(&self) -> Vec<String> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return a slice instead.

return Ok(());
};

let ca_cert_vector = util::load_cert_store_certs(self.root_relative(cert_store_path.unwrap()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's with this unwrap()?

@@ -124,6 +127,10 @@ impl Data {
// Set the read timeout to 5 seconds.
net_stream.set_read_timeout(Some(Duration::from_secs(5))).expect("timeout set");

// Grab the certificate info
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this tls stuff should be grouped together to avoid using so many feature(tls) attributes.

One idea is to have a block:

#[cfg(feature = "tls")]
{
    let mut data = Data::new(http_stream);
    if let Some(certs) = net_stream.get_peer_certificates() {
        data.set_peer_certificates(certs);
    }

    Ok(data)
}

#[cfg(not(feature = "tls"))]
Ok(Data::new(http_stream))

@@ -312,3 +314,14 @@ impl<'a, 'r, T: FromRequest<'a, 'r>> FromRequest<'a, 'r> for Option<T> {
}
}

#[cfg(feature = "tls")]
impl <'a, 'r> FromRequest<'a, 'r> for MutualTlsUser {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sure. That's pretty unfortunate. Okay, it's fine here.


fn from_request(request: &'a Request<'r>) -> Outcome<Self, Self::Error> {
// Get peer's IP address
let ip_addr = match request.client_ip() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can do something like:

let ip_addr = request.client_ip().or_forward(())?;

Use this here and everywhere else in this implementation.

dns-lookup = "0.9.1"
untrusted = "0.6.1"
webpki = "0.18.0-alpha4"
openssl = "0.10.10"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rocket simply can't depend on openssl because of these issues. Whatever is being used here from openssl can almost certainly be found in ring, rustls, or another ring based library. We should use one of those instead.

@akuanti
Copy link

akuanti commented Aug 31, 2018

@SergioBenitez @jebrosen
How would you prefer to approach certificate parsing? webpki parses the certificate partially, storing the parsed fields (e.g. subject) as untrusted::Input. However, that internal data is private. Ideally, we could just get that and decode it further. The options seem to be:

  1. make changes to webpki (particularly EndEntityCert) to decode those fields and provide getters for them
  2. decode the certificate again from the rustls::Certificate using functions from ring::der
  3. decode the certificate again using some other library, such as der-parser

(1) depends on getting a pull request into webpki or maintaining a fork of it, (2) and (3) may have performance implications. Any suggestions?

On a related note, do you need MutualTlsUser to be able to provide all information out of the certificate, or is there a specific list of fields?

@SergioBenitez
Copy link
Member

@akuanti I believe I've responded to each of those queries on IRC, is that right?

@akuanti
Copy link

akuanti commented Sep 10, 2018

@SergioBenitez I believe so.

I had one last one: how do you feel about splitting this PR into two: 1) config, client authentication, and MutualTlsUser request guard, and 2) exposing the certificate details. In 1) we can expose the name that successfully validated against the certificate as the subject name (even though it might have come from subject alt names).

Do you prefer for this branch to be rebased on master, or should we keep merging master into it to keep it up to date?

@SergioBenitez
Copy link
Member

I had one last one: how do you feel about splitting this PR into two: 1) config, client authentication, and MutualTlsUser request guard, and 2) exposing the certificate details. In 1) we can expose the name that successfully validated against the certificate as the subject name (even though it might have come from subject alt names).

This is a great idea! Let's do that.

Do you prefer for this branch to be rebased on master, or should we keep merging master into it to keep it up to date?

Yes, please rebase on the latest master.

Kmoneal and others added 8 commits September 11, 2018 14:58
* 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.
Kmoneal and others added 14 commits September 11, 2018 14:58
* 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.
@carlosdagos
Copy link

Hello! I'd actually really like this feature. What sort of issues are holding it back, and also are you looking for additional help? Thanks 👍

@SergioBenitez
Copy link
Member

I'm closing this due to inactivity. If anyone wants to take-over this work, I'd love to review a polished up version of this. Until then, let's track this in #17.

@MikailBag MikailBag mentioned this pull request Apr 9, 2020
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.

None yet

5 participants