expose on_connect v2

[robjtede]

PR Type

Feature

PR Checklist

• Tests for the changes have been added / updated.
  • Client certs can be accessed.
• Documentation comments have been added / updated.
• A changelog entry has been made for the appropriate packages.
• Format code with the latest stable rustfmt

Overview

Builds on #1482 (which will superceed if this is merged), which attempted to use the existing on_connect methods in -http to add data into the request-local extensions container. This was along the right lines, but I believe the approach in this PR is better for a couple of reasons:

• doesn't add a type parameter to HttpServer
• multiple types can be added to request extensions

I believe this will unblock #1727.

As you created the original PR @MikailBag, I'd appreciate your thoughts on this modifed approach.

[codecov-io]

Codecov Report

Merging #1754 into master will decrease coverage by 0.24%.
The diff coverage is 34.69%.

@@            Coverage Diff             @@
##           master    #1754      +/-   ##
==========================================
- Coverage   53.98%   53.73%   -0.25%     
==========================================
  Files         125      126       +1     
  Lines       12104    12145      +41     
==========================================
- Hits         6534     6526       -8     
- Misses       5570     5619      +49     

Impacted Files	Coverage Δ
actix-http/src/h2/dispatcher.rs	0.00% <0.00%> (ø)
actix-http/src/h2/service.rs	0.00% <0.00%> (ø)
actix-http/src/helpers.rs	70.37% <ø> (ø)
src/server.rs	37.00% <15.38%> (-3.30%)	⬇️
actix-http/src/builder.rs	46.47% <20.00%> (-3.53%)	⬇️
actix-http/src/service.rs	45.34% <50.00%> (+0.82%)	⬆️
actix-http/src/h1/service.rs	51.40% <92.30%> (+3.28%)	⬆️
actix-http/src/extensions.rs	61.53% <100.00%> (+3.20%)	⬆️
actix-http/src/h1/dispatcher.rs	56.37% <100.00%> (+0.22%)	⬆️
actix-http/src/header/common/mod.rs	10.00% <0.00%> (-6.67%)	⬇️
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4519db3...ca35027. Read the comment docs.

[MikailBag]

This is much better than my PR: ugly hack 1 (storing extention type in Server signature) successfully dropped!

Just as a reminder, there is also ugly hack 2: passing &dyn Any as handler input. I still don't have ideas how to make it more typed.

[robjtede]

I don't think there's a way to remove that unless we do one on connect method for each type like on_connect_openssl etc.

[robjtede]

heres the extract client certificate example, i'm going to move it to the examples repo and revert the in-repo example to the simpler one as before:

Example Code

use std::{any::Any, env, fs::File, io::BufReader};

use actix_tls::rustls::{ServerConfig, TlsStream};
use actix_web::{
    dev::Extensions, rt::net::TcpStream, web, App, HttpResponse, HttpServer, Responder,
};
use log::info;
use rust_tls::{
    internal::pemfile::{certs, pkcs8_private_keys},
    AllowAnyAnonymousOrAuthenticatedClient, Certificate, RootCertStore, Session,
};

const CA_CERT: &str = "examples/certs/rootCA.pem";
const SERVER_CERT: &str = "examples/certs/server-cert.pem";
const SERVER_KEY: &str = "examples/certs/server-key.pem";

#[derive(Debug, Clone)]
struct ConnectionInfo(String);

async fn route_whoami(
    conn_info: web::ReqData<ConnectionInfo>,
    client_cert: Option<web::ReqData<Certificate>>,
) -> impl Responder {
    if let Some(cert) = client_cert {
        HttpResponse::Ok().body(format!("{:?}\n\n{:?}", &conn_info, &cert))
    } else {
        HttpResponse::Unauthorized().body("No client certificate provided.")
    }
}

fn get_client_cert(connection: &dyn Any, data: &mut Extensions) {
    if let Some(tls_socket) = connection.downcast_ref::<TlsStream<TcpStream>>() {
        info!("TLS on_connect");

        let (socket, tls_session) = tls_socket.get_ref();

        let msg = format!(
            "local_addr={:?}; peer_addr={:?}",
            socket.local_addr(),
            socket.peer_addr()
        );

        data.insert(ConnectionInfo(msg));

        if let Some(mut certs) = tls_session.get_peer_certificates() {
            info!("client certificate found");

            // insert a `rustls::Certificate` into request data
            data.insert(certs.pop().unwrap());
        }
    } else if let Some(socket) = connection.downcast_ref::<TcpStream>() {
        info!("plaintext on_connect");

        let msg = format!(
            "local_addr={:?}; peer_addr={:?}",
            socket.local_addr(),
            socket.peer_addr()
        );

        data.insert(ConnectionInfo(msg));
    } else {
        unreachable!("socket should be TLS or plaintext");
    }
}

#[actix_web::main]
async fn main() -> std::io::Result<()> {
    if env::var("RUST_LOG").is_err() {
        env::set_var("RUST_LOG", "info");
    }

    env_logger::init();

    let ca_cert = &mut BufReader::new(File::open(CA_CERT)?);

    let mut cert_store = RootCertStore::empty();
    cert_store
        .add_pem_file(ca_cert)
        .expect("root CA not added to store");
    let client_auth = AllowAnyAnonymousOrAuthenticatedClient::new(cert_store);

    let mut config = ServerConfig::new(client_auth);

    let cert_file = &mut BufReader::new(File::open(SERVER_CERT)?);
    let key_file = &mut BufReader::new(File::open(SERVER_KEY)?);

    let cert_chain = certs(cert_file).unwrap();
    let mut keys = pkcs8_private_keys(key_file).unwrap();
    config.set_single_cert(cert_chain, keys.remove(0)).unwrap();

    HttpServer::new(|| App::new().route("/", web::get().to(route_whoami)))
        .on_connect(get_client_cert)
        .bind(("localhost", 8080))?
        .bind_rustls(("localhost", 8443), config)?
        .workers(1)
        .run()
        .await
}

Test with cURL or HTTPie:

curl https://127.0.0.1:8443/ --cacert examples/certs/rootCA.pem --cert examples/certs/client-cert.pem --key examples/certs/client-key.pem
# or
http https://127.0.0.1:8443/ --verify=false --cert=examples/certs/client-cert.pem --cert-key=examples/certs/client-key.pem

[fakeshadow]

The Any type here should be either TcpStream or UnixStream and their wrapper types. They both have actix_server::FromStream trait and technically we can add additional trait bound to HttpServer for it. The problem is we would kick the problem down the line to actix-http where we don't have matched generic trait method for something we want from these streams.(I could be wrong as I don't actually get it compiled as the trait origins in actix-net repo)

I believe it can be worked out later.

[fakeshadow]

lgtm.