Adds option to supply root SSL certificate for client

[lavafroth]

Fixes #870.

This PR adds three flags:

• --server-certs
• --client-cert
• --client-key

The --server-certs flag can be supplied multiple .pem or .der files which get added to the requesting client as a root certificate and can thus be used for endpoints with self-signed certificates.

For mutual authentication, we use the last two flags. The --client-key flag needs to be supplied a .pem file which has the PKCS #8 PEM encoded private key and the --client-cert flag must be supplied a PEM encoded certificate for the client. An additional root CA certificate may need to be supplied through --server-certs as a .pem or .der file.

This means, your command should look like:

cargo run -- --url https://localhost:8001 \
--client-key client1-key.pem \
--client-cert client1-crt.pem \
--server-certs ca-crt.pem

Progress

@epi052 has kindly provided this list which tracks the progress of this PR.

[epi052]

ayyyy, thanks for taking the time to put this together! 🎉

I should be able to review in the next day or two. Some initial thoughts though:

it's nice to be able to add a root cert, but folks can already interact with targets with certs they don't know about with -k|--insecure. On the other hand, users have no way to connect to a target that authenticates its clients. Adding a client certificate would be a totally new feature, and, in my opinion, should be added to this PR (or at least planned for in this PR with regards to naming etc). See my note here.

i think ultimately, the cli options should be

• --server-certs CERT[, CERT...]
• --client-certs CERT[, CERT...]

I'll start a review for what's currently here soon. Let me know if you want to work on the client cert as well, or if we should split of a cert branch that both PRs can merge into before a final merge to main. Thanks again!

[lavafroth]

As far as I can tell, the code I have written does the --server-certs part. For mutual authentication though, we would not need the server certs. Instead, the client would need a private key from a pem file to prove its identity. Correct me if I was wrong anywhere but if this is how it works, I'll begin adding commits for the --client-certs feature.

[lavafroth]

@AkechiShiro, it'd be great if you could test out this branch against your mutual authentication local server, now that I've added the --client-cert flag.

[epi052]

howdy! starting looking at this today.

just trying to get a local server setup correctly to test things. I have something that works with curl, but doesnt with ferox. This PR may take a little more time than normal to get reviewed, as a heads up. On top of setting up a representative server, I have some life things going on over the next 2 weeks. I'll work on this as time permits though.

Thanks again for putting this together!

[lavafroth]

Sure thing! Take your time reviewing this since it is quite a large change anyway. We would need to think about refactoring client::initialize because clippy screams at me due to the sheer number of arguments supplied to it 😅.

[epi052]

an update on this:

I was able to get this working on a local mutual auth test server, but required some changes (shown as diff below). I kept getting Bad certificate errors during the ssl handshake when using rustls-tls :: from_pem. I suspect I wasn't constructing the certificate properly, but idk what exactly i was doing wrong (format, intermediate certs, etc...). The steps I used for creating the certificates are here

Another problem I ran into is that rustls-tls couldn't renegotiate down to tls1.2 (which is what my local server is using). It would start with tls1.3, server would ask to drop down to 1.2, and the client would error out. I was able to fix this in code by limiting the maximum tls version, but that's not a good solution imo. I didn't run into this problem with native-tls.

I'm including my Caddyfile (used with caddyserver that I'm using for testing below the diff. Any additional testing toward a solid final solution would be super.

Some other commands that may prove useful for debugging/testing are included as well.

----8<----

native-tls changes

diff --git a/Cargo.toml b/Cargo.toml
index 7c1bb77..1ac4a85 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -35,7 +35,7 @@ tokio = { version = "1.26", features = ["full"] }
 tokio-util = { version = "0.7", features = ["codec"] }
 log = "0.4"
 env_logger = "0.10"
-reqwest = { version = "0.11", features = ["socks", "rustls-tls"] }
+reqwest = { version = "0.11", features = ["socks", "native-tls"] }
 # uses feature unification to add 'serde' to reqwest::Url
 url = { version = "2.2", features = ["serde"] }
 serde_regex = "1.1"
diff --git a/src/client.rs b/src/client.rs
index a59694e..946be45 100644
--- a/src/client.rs
+++ b/src/client.rs
@@ -88,8 +88,13 @@ pub fn initialize(
         // in either case, add the root certificate to the client
         if let Some(extension) = cert_path.extension() {
             if "pem" == extension.to_str().unwrap_or_default() {
-                let cert = reqwest::tls::Identity::from_pem(&buf)?;
+                // let cert = reqwest::tls::Identity::from_pem(&buf)?;
+                let cert = reqwest::tls::Identity::from_pkcs8_pem(
+                    &std::fs::read("client1-crt.pem")?,
+                    &std::fs::read("client1-key.pem")?,
+                )?;
                 client = client.identity(cert);
+                // .max_tls_version(reqwest::tls::Version::TLS_1_2);
             } else {
                 // if it is not a "pem" file or we cannot determine the extension
                 // TODO: spew an error

Caddyfile

(mTLS) {
    tls {
        client_auth {
            mode require_and_verify
            trusted_ca_cert_file ca-crt.pem
        }
    }
}

https://localhost:8001 {
    import mTLS
    log

    handle / {
        file_server browse
    }
}

curl with client cert/key

curl --cert client1-crt.pem --key client1-key.pem https://localhost:8001 -v

openssl client

openssl s_client -connect 127.0.0.1:8001 -cert client1-crt.pem -key client1-key.pem -CAfile ca-crt.pem -debug -state -tls1_2

openssl server

openssl s_server -CAfile ca-crt.pem -cert server-crt.pem -key server-key.pem

[lavafroth]

I made the changes suggested above. The code now uses the native-tls feature. The following invocation should connect to the mTLS endpoint successfully:

cargo run -- --url https://localhost:8001 \
--client-key client1-key.pem \
--client-cert client1-crt.pem \
--server-cert ca-crt.pem

[epi052]

After looking more closely at rustls-tls, it explicitly doesn't support renegotiation / downgrading and a bunch of other stuff.

https://docs.rs/rustls/latest/rustls/index.html#non-features

That's fine for projects with controlled usage, but there's tons of old stuff on the internet, so probably not a great library for ferox.

[epi052]

Good morning! I started a review, but haven't given you the checklist below to help hitting all the places you'd need for adding a new command line option.

I'm not done reviewing, so don't worry about making changes yet. Just wanted to keep you up to date

• Add new command line option checklist
  • config/container.rs
    • add value to Configuration struct
    • add value to Configuration::default
    • add value to Configuration::parse_cli_args
    • add value to Configuration::merge_config
  • config/tests.rs
    • update Configuration::tests::setup_config_test with new value
    • update Configuration::tests::default_configuration with new value
    • add test to check config parsing
  • ferox-config.toml.example
    • add new value
  • banner/container.rs
    • add new BannerEntry to Banner struct
    • update Banner::new with a BannerEntry
    • update print_to with whether to print or not
  • tests/test_banner.rs
    • add new test so that the new BannerEntry is displayed and examined under test
  • Cargo.toml
    • bump version - minor release +1 - patch release = 0
  • parser.rs
    • add the option
  • scan_manager/tests.rs
    • add serialized version of new option to the feroxstates_feroxserialize_implementation test
  • wherever.rs
    • implement the new feature

[lavafroth]

Hi, the idea for multiple server certificates looks awesome! I just changed it so that we don't use an Option for it. In case of an empty iterator for the server_certs, it will yield None anyway. Also, I did a cargo fmt to keep the checks happy.

[epi052]

Hi, the idea for multiple server certificates looks awesome! I just changed it so that we don't use an Option for it. In case of an empty iterator for the server_certs, it will yield None anyway. Also, I did a cargo fmt to keep the checks happy.

haha, ty! I saw the changes, they look good!

I finished up adding tests and things, should be merged today. Thanks for your work on this!