Making impl ClientHelloPayload public ?

[dawid-nowak]

In our application we would like to make a decision based on contents of SNI in ClientHelloPayload without necessary affecting the how rustls flow. So we peek some bytes from the stream and initialize Reader and then use Reader to produce ClientHelloPayload. Everything works as expected and it is great that these two are part of the public API. Unfortunately, getting the SNI extension is not as straightforward since those APIs are not part of public API.

If would be also very helpful if other methods on ClientHelloPayload such as sni_extension, sni_extension, find_extension were added to public API. It seems that they were but now they visibility is only within the crate.

So the question really is if it makes sense to make impl ClientHelloPublic part of the public API from rustls perspective?

[cpu]

👋 Hi there,

In our application we would like to make a decision based on contents of SNI in ClientHelloPayload

Have you looked at using the Acceptor API? The Accepted::client_hello() fn's return type offers a server_name() fn for accesing the SNI value to make a decision.

If you look in the issue tracker I believe there have been previous discussions about exporting internal message types and this hasn't been something the project has been keen to support. In this case I'm not sure it's necessary at all given the Acceptor API.

[dawid-nowak]

Thanks for a quick response. I think our use case is slightly different.
I would like to check if the incoming TCP stream is TLS. If it is TLS then I can pass the stream to rustls for proper processing. If the stream is not then we would like to process it as TCP.

The main problem with using the Acceptor (I am actually using LazyConfigAcceptor ) is that we can't get back the consumed bytes.

That's why I resolved to peeking bytes from TCP stream and then deserializing ClientHelloPayload myself. In this respect if I can successfully deserialize ClientHelloPayload and get SNI, I can pass the stream to rustls acceptor and if I can't then I can treat it as a TCP stream.

Having ClientHelloPayload public as building block is a great simplification since I don't have to do my own parsing. Being able to retrieve extensions would simplify it even further.

[djc]

@dawid-nowak have a look at rustls/tokio-rustls#54.

[djc]

I also just submitted #1916, maybe it could help?

[dawid-nowak]

Super, i will need to dig deeper into it.
The use case here is similar to Envoy's TLS inspector .

My code looks pretty much as this snippet in rustls/tokio-rustls#54. We use something like below to figure out if the incoming stream is TCP/TLS.

So really, we are using public APIs anyway. The missing bit is to be able to get the extensions cleanly without having to re-writing/hacking at stuff that rustls is doing anyway :)

let mut reader = rustls::internal::msgs::codec::Reader::init(client_hello_buf);
        let chp = ClientHelloPayload::read(&mut reader);
        if let Ok(chp) = chp {
            for ext in chp.extensions {
                if let rustls::internal::msgs::handshake::ClientExtension::ServerName(ref req) = ext
                {
                    for s in req {
                        let encoded_server_name = s.get_encoding();
                        if let Ok(Some(name)) = Self::parse_rustls_sever_name(encoded_server_name) {
                            return Self {
                                server_name: Some(name),
                            };
                        }
                    }
                }
            }
        }
        Self { server_name: None }
    }

    fn parse_rustls_sever_name(server_name: Vec<u8>) -> Result<Option<String>> {
        // first skipped byte is rutls type server name type
        // first two bytes is the encoded length;
        let buf: [u8; 2] = server_name[1..3].try_into()?;
        let len = u16::from_be_bytes(buf) as usize;
        let range = &server_name[3..(3 + len)];
        let dns = DnsName::try_from(range);
        if let Ok(server_name) = dns {
            Ok(Some(server_name.as_ref().to_ascii_lowercase()))
        } else {
            Ok(None)
        }
    }

I don't like the parse_rustls_sever_name, the code is brittle since we need to guess rustls formats etc. it would be neater if there was an API available.

[djc]

Perhaps we could also add an into_bytes() method on Acceptor that takes out the receive buffer, if that helps.

[dawid-nowak]

If I understand it correctly with into_bytes(), I would still need to deserialize ClientHelloPayload?
I get that my use case is pretty awkward :) but if you wanted to make changes to Acceptor then I think you would need to expose some extra methods on the Acceptor to allow the application to peek at client hello before calling accept and/or to get back the full tcp stream if the information in the client hello doesn't match the needs.

[djc]

Okay, I see how that doesn't work.

So maybe we should move the deframer_buffer.buf into AcceptedAlert, to allow you to recover the received bytes from there? Then if it fails to parse a ClientHello you can get the bytes and turn them into a plaintext connection?

[dawid-nowak]

That could work... Still think that making impl ClientHelloPayload would be easier and less error prone. Could you help me understand the rationale here ?

[djc]

Why do you think it would be easier and less error prone?

[dawid-nowak]

Sure :) just for the conversations sake
I look at it from perspective of a user who is outside of the normal path and is happy to use low level building blocks provided to create something different.

ClientHelloPayload is public and also Reader is public so these two basic blocks are already available to the end user to start tinkering. The methods to find extensions are there but hidden. And since these methods are used internally, they are tested and verified. Exposing those methods is probably not a problem.

Yes, the application has a dependency on rustls low level building blocks which could change but at the same time it can trust that these blocks are sound.

Additionally, the normal flow of rustls is not affected. There are no changes to the APIs so the existing applications will work as normal.

That would be my thinking but I am happy to go with whichever way.

[djc]

Reader and ClientHelloPayload are only public (AFAICT) via internals:

Internal classes that are used in integration tests. The contents of this section DO NOT form part of the stable interface.

These are not intended to be used by downstream applications and are not bound by semver constraints.

[cpu]

I left a comment with a WIP branch on #1916 but I think it makes more sense to discuss here.

Using the acceptor API and the fns on an accepted rustls::server::ClientHello seems like it meets the need of the TLS side (mainly, dispatch by SNI/ALPN) without any new API surface/exports or cumbersome external deserialization.

I think what's lacking is a way to accept a plaintext connection in some cases. To solve that, what if instead of exposing the deframer buffer and having the caller need to figure out if its TLS we do the reverse and have the acceptor return the buffer when it has already decided it isn't TLS as part of a returned error?

I think that strikes a good balance. Existing TLS focused users can continue exactly as they do now, treating accept errors of any kind as fatal, sending an alert and closing. Users that want to dispatch non-TLS data can match on the new error type and unpack the data that Rustls couldn't deframe as the expected first TLS message. Does that seem workable?

[djc]

Your WIP branch makes sense to me, though I'm not sure I understand the need for OtherError(Arc::new(..)) wrapping the internal error. Could it just be a boxed Error? I think we could stick the buffer in AcceptedAlert instead of in Error, which seems simpler if a little bit weird. Also we should maybe file an issue for a simplifying revision the next time we decide to release a semver-incompatible API.

[ctz]

I think what's lacking is a way to accept a plaintext connection in some cases. To solve that, what if instead of exposing the deframer buffer and having the caller need to figure out if its TLS we do the reverse and have the acceptor return the buffer when it has already decided it isn't TLS as part of a returned error?

My feeling on this is: it is very clear within the first few bytes whether the conversation is TLS or not. It seems quite reasonable to have downstream code that wants to do plaintext and TLS on the same stream port to read the bytes and do the demultiplexing up-front?

[cpu]

Could it just be a boxed Error?

Yup! That's a better idea, not sure why I didn't think of that initially. I will change it out.

I think we could stick the buffer in AcceptedAlert instead of in Error, which seems simpler if a little bit weird.

I thought about this approach but it felt fairly unnatural. I'm not sure I understand what makes it simpler, except that it avoids adding a new variant to the Error enum that's specific to the Acceptor and not used elsewhere.

Also we should maybe file an issue for a simplifying revision the next time we decide to release a semver-incompatible API.

Yeah, that sounds like a good idea 👍

It seems quite reasonable to have downstream code that wants to do plaintext and TLS on the same stream port

@ctz Sorry, I'm having trouble mapping your response to an indicator of support/dissent for the proposal I have in mind. Are you thinking the demultiplexing shouldn't be done in Rustls and we should continue to expect it to be done donwstream ala the approach used in rustls/tokio-rustls#54 (comment) ?

[ctz]

Are you thinking the demultiplexing shouldn't be done in Rustls and we should continue to expect it to be done donwstream ala the approach used in rustls/tokio-rustls#54 (comment) ?

Yeah that's the sort of thing I had in mind.

[ctz]

wrong butan

[cpu]

Yeah that's the sort of thing I had in mind.

OK, I won't pursue turning the WIP branch into a PR in that case 👍

Should this issue be closed?

[dawid-nowak]

I am happy to close this for time being.

There seem to be two radically different approaches here and we can't get a consensus :

1. Let rustls try to figure out if the stream is the TLS stream and if it is not then return back the consumed data (or stream) so the user can re-store the stream.
2. Let the user try to figure out if the stream is TLS and then either use rustls or not.

I think Option 2 makes sense if for example the basic blocks to parse ClientHello are available as a public API. That would save people from having to re-invent the logic here and since those blocks are part of rustls then there is trust that the logic is good.

Option 1 is tightly integrated into rustls. Thing to consider here is that the new API would need to be exposed via higher level adapters such as tokio-rustls.

[cpu]

There seem to be two radically different approaches here and we can't get a consensus

From my read I think Option 3, the status quo, is where maintainer consensus is today.

Solutions outside of the crate are possible without change here (as demonstrated in the tokio-rustls issue). I think the parts required to do it reliably are already exposed under the internals API namespace. Option 2 feels like elevating those parts out of that namespace in order to offer explicit support & semver consideration. That's a fair request, but I don't think its likely at this time. I liked Option 1, but since that wasn't unanimous and it isn't a use case myself or the folks funding my work feel strongly about I was swayed towards inaction :-)