Warp-TLS API change on "OnInsecure"

[liamzee]

I just closed my pull request on Warp-TLS: #1032

There is very limited functionality responding to OnInsecure requests; while standard procedure seems to be to run Warp behind a reverse proxy, with the reverse proxy handling TLS connections, some more security-minded people might wish to have Warp-TLS running behind the proxy and some convenience-oriented people might wish to have Warp-TLS standing on its own.

I do think another option in OnInsecure that can provide a broader response than simply returning a hard-coded response header with a custom message might be good, but what would be a good interface for that? Would a Request -> Response interface from WAI be better? How about Request -> IO Response if there's a desire to call to the filesystem, load up a file in IORef / MVar / TVar, or to log an action?

If the maintainers feel this is a good idea, what kind of interface would they suggest?

[Vlix]

Giving the user more options sounds like a fine idea. I don't have too much experience with TLS internals, though, so I'm not quite certain what's a good idea or even possible.

That said, if I look at the code, the TLS handling starts before the request is actually accepted (which makes sense, don't start sending anything before both parties agree it's safe/ok to do so). So a WAI Request -> Response interface might not even be an option.
It also looks like the DenyInsecure could be implemented using DenyInsecureWithAction, so that could be changed into a Pattern at some point.

I'd opt for the action just being a lazy ByteString, though. No reason to force the header to be a separate strict ByteString (can just be the first chunk of the lazy ByteString).

I'd like to know what @kazu-yamamoto thinks about this. He knows more about TLS handling, I feel.

[kazu-yamamoto]

Sorry for the delay.
I'm OK to add a new constructor.

@Vlix's observation looks reasonable to me. I guess the following is the most generic:

| DenyInsecureWithAction (S.ByteString -> IO ())

@liamzee However, if you find a way to create Request, please show us your code.

[Vlix]

| DenyInsecureWithAction (S.ByteString -> IO ())

At the least, we'd have to supply the Socket as well, since the S.ByteString is only the first chunk.
But if we call it Deny... the user will probably expect the socket to be closed automatically? So we'd have to make sure the socket is closed when the handler exits AND on any exception thrown 🤔

Also, the documentation should state that the handler will have the responsibility to send a correctly formatted response with the provided first chunk of the request and Socket

[kazu-yamamoto]

@vix You are right.

Maybe S.ByteString -> IO Response?

[Vlix]

We'll need to at least provide something like S.ByteString -> Socket -> IO Response.
It's that, or provide a getRequestBodyChunk-like function like IO S.ByteString -> IO Response.

Otherwise the user can't inspect the entire request, right?

[liamzee]

Is this feature creep, though? I can understand wanting to expose the socket, but the more complex it becomes, the harder it is to secure.

Still here, a bit distracted on other projects ATM.

[Vlix]

I don't feel like this is feature creep. More like scoping the feature request and deciding the best API.

I'm feeling more for IO ByteString -> IO Response, where the IO ByteString argument is functionally getRequestBodyChunk and the given response would be used to send HTTP status, headers and body over the socket back, after which warp will close the socket and throw the InsecureConnectionDenied.