The first version of tls_inspector seems not to be good

[eao197]

There was a discussion after one of the articles on Habr.com (note: it's in Russian) about the necessity of such a thing as "tls_inspector" in addition to "ip_blocker" and "state_listener" (both added in RESTinio v.0.5). I've tried to implement such "tls_inspector". The first version can be found in "0.5-dev--tls-inspector" branch on BitBucket.

An example of very simple usage of tls_inspector can be found here. This example shows how to extract commonName from client's certificate and use this value as a user name to control access of the user to resources. A user "Alice" has access to all URLs, but user "Bob" has access only to /all and haven't access to /limited.

This implementation works, but I don't like it. It seems that several significant flaws should be fixed before "tls_inspector" will become a part of RESTinio.

The first moment I don't like is the return of inspection_result_t from tls_inspector's inspect method.

It seems logical that tls_inspector can check some client credentials or TLS parameters and allow or deny the connection. But, maybe Asio's set_verify_callback is more appropriate for that purpose?

The second moment can be seen in the implementation of a simple example of tls_inspector usage (this example was already mentioned above). If a tls_inspector has to store connection-related information somewhere (like in user_connections_t container in the example) then there should be a way to say tls_inspector that a connection is gone (closed or upgraded to WebSocket connection).

It seems that tls_inspector and state_listener will be a single thing most of the time. If so, why do we need a separate entity for tls_inspector? What if state_listener is enough and all that we need is the way to access TLS-socket from state_listener's state_changed method?

There can be another point of view on that issue. Maybe there should be a way to store some user-provided data inside a connection with a possibility to extract that via requiest_handle_t. It will allow doing things like:

class my_tls_inspector {
   ...
   restinio::tls_inspector::inspection_result_t
   inspect(restinio::tls_inspector::incoming_info_t & info) noexcept {
      auto name = extract_user_name(info);
      info.add_connection_user_data<user_name_t>(some_id, user_name_t{name});
   }
};
...
auto limited_resource_request_handler(
   restinio::request_handle_t & req ) {
   ...
   auto & name = req->query_connection_user_data<user_name_t>(some_id);
   ...
}

That approach has a benefit: a user data associated with a connection will automatically be destroyed when the connection is gone (closed or upgraded to WebSocket). So we can create a tls_inspector as a separate entity not related to state_listener.

That approach can also be used for different purposes. For example, if a client uses "keep-alive" for a connection and issues several requests via that connection, then connection_user_data allows storing some connection- or user-specific information without a need to implement own container for that purpose.

It'll be handy if someone can tell his or her thoughts on that topic. I hope someone finds time and desire to answer these questions or share their own opinion (any suggestions are welcome):

Does tls_inspector look like a valuable addition to RESTinio?

If does, should tls_inspector allow or deny new connections?

Is it a good idea to have something like connection-related user data associated with a connection?

[eao197]

@pvsur Thanks for your opinion.

But I have to ask you to write in English because there are non-Russian-speaking users of RESTinio. If it's a problem for you then we can speak via Habr's messaging systems.

[pvsur]

Answered in Habr

[eao197]

Yet another flaw in the current design: there is no guarantee that RESTinio calls state_listener's state_changed after the return from tls_inspector's inspect. If some exception is raised somewhere inside RESTinio internals between these calls then state_listener's state_changed won't be called. It means that if tls_inspector combined with state_listener and there is some resources cleanup in state_changed, then that cleanup won't be done.

The current implementation of RESTinio calls state_changed right after the return from inspect, so there is no place for exceptions for now. But it can be changed in some future version of RESTinio accidentally.

[eao197]

The current idea is the following:

1. There is no need in a separate thing named tls_inspector. A state_listener can inspect TLS-related parameters for a new connection.
2. There is no possibility to deny a connection from state_listener there is something wrong with TLS-parameters. For example, if a developer wants to check some values in the certificate of a client and deny a connection from that client, the developer should use Asio's set_verify_callback facility.

The class restinio::connection_state::notice_t can be extended by these additional methods:

class tls_accessor_t; // Will be defined in `restinio/tls.hpp`
   // So if you don't need TLS and don't include `restinio/tls.hpp`
   // then you won't see the details of tls_accessor_t.

class notice_t {
   ...
public:
   ...
   // Is this TLS-connection or not?
   bool is_tls_connection() const noexcept;

   // Calls lambda if this is a TLS-connection. Otherwise does nothing.
   template<typename Lambda>
   decltype(auto) try_inspect_tls(Lambda && lambda) const;

   // Calls lambda if this is a TLS-connection. Otherwise throws an exception.
   template<typename Lambda>
   decltype(auto) inspect_tls_or_throw(Lambda && lambda) const;

   // Calls lambda if this is a TLS-connection. Otherwise returns default_value.
   template<typename Lambda, typename T>
   T inspect_tls_or(Lambda && lambda, T && default_value) const;
};

These methods can be used in state_listener's state_changed method (but, probably, only when notice_t::cause() returns cause_t::accepted).

The tls_accessor_t class can have the following content:

class tls_accessor_t {
   tls_socket_t & m_socket;
public:
   ... // constructor's and destructor.

   auto native_handle() const noexcept { return m_socket->asio_ssl_stream().native_handle(); }
};

[binarytrails]

@eao197

2. There is no possibility to deny a connection from state_listener there is something wrong with TLS-parameters. For example, if a developer wants to check some values in the certificate of a client and deny a connection from that client, the developer should use Asio's set_verify_callback facility.

I don't think there is something wrong with TLS parameters, it's just that the SSL is implemented in a very particular way inside of Asio. You can still validate the SSL stream by wrapping the set_verify_callback callback while creating the SSL stream and later keep reusing the ssl context for your inspection and shutdown the ssl stream if something is not meeting your criteria in the tls_inspector:

ssl_socket_->asio_ssl_stream().set_verify_callback(
    [this, hostname](bool preverified, asio::ssl::verify_context& ctx) -> bool
    {
        // extract cert info prior to verification
        char subject_name[256];
        X509* cert = X509_STORE_CTX_get_current_cert(ctx.native_handle());
        X509_NAME_oneline(X509_get_subject_name(cert), subject_name, 256);
        if (logger_)
            logger_->d("[http:client]  [connection:%i] [ssl] verifying %s", id_, subject_name);
        // run the verification
        auto verifier = asio::ssl::rfc2818_verification(hostname);
        bool verified = verifier(preverified, ctx);
        // post verification, codes: https://www.openssl.org/docs/man1.0.2/man1/verify.html
        auto verify_ec = X509_STORE_CTX_get_error(ctx.native_handle());
        if (verify_ec == X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN /*19*/)
            verified = true;
        return verified;
    }
);

-- source code

[eao197]

@binarytrails That is one reason why I think that the first implementation of tls_inspector is not a good one. If you need to check something inside the certificate of a user, it is better to use set_verify_callback. So there is no need to return deny or allow from tls_inspector's inspect method.

But tls_inspector can be useful if a user wants to get some additional info from the certificate of a client and use that info later. In that case, there is no need to set up set_verify_callback, it's necessary to have access to TLS-socket from state_listener.