server: enable holding back acceptor alerts

[djc]

@howardjohn please take a look if this can address your use case. If so, could you add some test coverage for it?

(I do wonder if at this point it would be easier to just address the non-TLS input case more directly...)

Fixes #146.

[ctz]

I don't mind adopting this as a possibility, but definitely needs to be defend with tests. (And, ideally the referenced example code is executed to ensure it behaves as desired.)

[howardjohn]

Thanks @djc !! I added tests and a bit more in #148

[djc]

Pretty sure this is wrong. In case of a partial write, just calling alert.write() again is wrong, since it'll duplicate the parts of the alert that were sent previously.

(Presumably it would be exceedingly rare to get a partial write, but still...)

[howardjohn]

I believe this works and is tested. The test uses a duplex stream with a capacity of 2 to ensure we cannot write it in one chunk. This is the same approach used in another test:

async fn lazy_config_acceptor_alert() {
    // Intentionally small so that we have to call alert.write several times

Looking at the alert.write() function, it does seem to consume the data it wrote such that multiple calls to the same alert.write() are valid IIUC

[howardjohn]

Hmm, for some reason it worked on my branch but not the PR now. let me see what changed.

[howardjohn]

Suggested change

	Ok(_) => continue,
	Ok(_) => {
	this.alert = Some(alert);
	continue;
	}

This fixes the issue and aligns with how LazyConfigAcceptor writes the alert

[djc]

@howardjohn you added a bunch of timeouts here which don't really seem necessary? This is a pretty controlled environment, so it feels excessive?

[howardjohn]

these were just copied from other cases, we can remove if preferred.

[djc]

This was previously attached to the wrong method:

@howardjohn what is the purpose of this? It doesn't make sense to me. If send_alert is true, we've sent the alert before this is reachable. If send_alert is false, I think you can only call this without taking out either the io or the alert, and there's no way you can actually check what the server received, I think?

[howardjohn]

The purpose is, when send_alert is false, to send any stored alerts. The alternative would be to take the io and alert and do it yourself, but then you need to go make your own sync write adapter, etc.

The original example only shows unconditionally sending an HTTP error. However, a more appropriate solution would be to send an http error if the incoming request was http (or "looks like" http) and otherwise send the actual alert down.

The new test case covers both paths - sending http error and sending alert

[djc]

But the test cases don't use this API. I understand the idea, but as implemented I don't think it works, because you can only call this method unconditionally (as in, without reviewing whether there's an alert).

[howardjohn]

lazy_config_acceptor_manual_alert does on L383.

The idea of the full end to end flow is:

let acceptor = tokio_rustls::LazyConfigAcceptor::new(rustls::server::Acceptor::default(), stream);
pin_mut!(acceptor);
let mut start = match acceptor.as_mut().await {
  Ok(start) => start,
  Err(e) => {
    if is_https
        && let Some(io) = acceptor.take_io()
        && let Some(data) = io.buffered()
        && tls_looks_like_http(data) {
      let _ = io.write_all(b"HTTP/1.0 400 Bad Request\r\n\r\nclient sent an HTTP request to an HTTPS listener\n").await;
      let _ = io.shutdown().await;
    } else if let Some(mut alert) = accepter.write_alert() {
        let _ = accept.await;
    }
    return Err(e);
  },
};
let ch = start.client_hello();
// .. rest of handshake

[djc]

But write_alert() will not actually send the alert if you've called take_io() before that.

[howardjohn]

ah... right. good call. Maybe we should rework this to return an alert that can be passed in its own io (generally from take_io) to do the writing. I can work on that

[howardjohn]

I sent #150 with this as well as the fix for #147 (comment)

[djc]

Where does your io.buffered() method come from?

[howardjohn]

Sorry, here is the concrete code: https://github.com/agentgateway/agentgateway/blob/485a299d2199cf7f792a8f7e55677f1a4898013b/crates/agentgateway/src/proxy/gateway.rs#L512.

In this case, my IO is a wrapper on the underlying TCPStream that allows me to buffer up a copy of data: https://github.com/agentgateway/agentgateway/blob/485a299d2199cf7f792a8f7e55677f1a4898013b/crates/agentgateway/src/transport/rewind.rs#L14. So we buffer up the initial TLS handshake. Then, on error, we can peak what the original data was.

Note the golang approach to the same problem works differently; they inspect the first few bytes to see if it looks like a TLS record and then send to the TLS stack. Both are viable and could work in Rust; we already used the RewindSocket for unrelated purposes in our application, so went with the "look back" approach.