Client address #3

New Issue

0xEAB · 2024-10-27T16:19:08Z

0xEAB commented

2024-10-27 16:19:08 +00:00

The ServerHttpRequest struct currently has no designated way to store the address of the remote client the request originated from.

This information would be crucial for auth logs (e.g. to be fed to security tools like Fail2Ban) or maybe web application firewalls, and handy for geo IP purposes.

Alternatives

Obviously, this information could be provided by a server to a request handler through alternative means (like a wrapper struct containing the ServerHttpRequest and additional info), though the naming might not be as adequate in this case.

Other implementations

PSR-7 handles this through the rather clunky ServerRequestInterface::getServerParams() method.

The [ServerHttpRequest](https://git.andrewlalis.com/Handy-Http/primitives/src/commit/23c74f82bedf662d3d6f8f2aed159a270f5a782d/source/handy_http_primitives/request.d#L13) struct currently has no designated way to store the address of the remote client the request originated from. This information would be crucial for auth logs (e.g. to be fed to security tools like Fail2Ban) or maybe web application firewalls, and handy for geo IP purposes. ### Alternatives Obviously, this information could be provided by a server to a request handler through alternative means (like a wrapper struct containing the ServerHttpRequest and additional info), though the naming might not be as adequate in this case. ### Other implementations PSR-7 handles this through the rather clunky `ServerRequestInterface::getServerParams()` method.

andrew commented

2024-10-28 14:02:35 +00:00

I am thinking of doing something like this, and adding const InternetAddress clientAddress; to the ServerHttpRequest:

/// The data representing a remote IPv4 internet address, available as an int or bytes.
union IPv4InternetAddress {
    const uint intValue;
    const ubyte[4] bytes;
}

/// The data representing a remote IPv6 internet address.
struct IPv6InternetAddress {
    const ubyte[16] bytes;
}

/// A remote internet address, which is either IPv4 or IPv6. Check `isIPv6`.
struct InternetAddress {
    /// True if this address is IPv6. False if this is an IPv4 address.
    const bool isIPv6;
    /// The port number assigned to the connecting client on this machine.
    const ushort port;
    union {
        IPv4InternetAddress ipv4Address;
        IPv6InternetAddress ipv6Address;
    }
}

It would mean that each request takes up an additional 16 bytes for the request, but I prefer this over the polymorphic class-based approach in Phobos.

I am thinking of doing something like this, and adding `const InternetAddress clientAddress;` to the `ServerHttpRequest`: ```d /// The data representing a remote IPv4 internet address, available as an int or bytes. union IPv4InternetAddress { const uint intValue; const ubyte[4] bytes; } /// The data representing a remote IPv6 internet address. struct IPv6InternetAddress { const ubyte[16] bytes; } /// A remote internet address, which is either IPv4 or IPv6. Check `isIPv6`. struct InternetAddress { /// True if this address is IPv6. False if this is an IPv4 address. const bool isIPv6; /// The port number assigned to the connecting client on this machine. const ushort port; union { IPv4InternetAddress ipv4Address; IPv6InternetAddress ipv6Address; } } ``` It would mean that each request takes up an additional 16 bytes for the request, but I prefer this over the polymorphic class-based approach in Phobos.

andrew commented

2024-10-28 14:11:26 +00:00

I'm going to go ahead and commit this along with the HTTP method update, and feel free to close if you're satisfied, or you are free to submit your own PR on it.

andrew referenced this issue from a commit

2024-10-28 14:12:08 +00:00

Addressed issues #3 (client address) and #1 (HTTP methods as strings)

0xEAB commented

2024-10-28 22:52:45 +00:00

It would mean that each request takes up an additional 16 bytes for the request, but I prefer this over the polymorphic class-based approach in Phobos.

Yeah, I don’t think we have to go with classes here either.

A relevant limitation if your union solution is, though, that it doesn’t support Unix domain sockets.
I’m not sure whether we should add them to the union as well (and replace isIPv6 with an enum-based type tag or something) or rather rethink the task as a whole.

(Another minor issue is that unions aren’t accessible in @safe code.)

Two ideas that come to mind are:

A server could add such information to a pseudo-header (pretty much like proxies do with X-Forwarded-For). The obvious limitation is that the value has to be either serialized to string or be a string in the first place.
A tagging mechanism that allows to add arbitrary attributes to HTTP messages.
PSR-7 has this and it works well in PHP through dynamic typing; in D however probably this requires some sort of clunk.
oceandrift-http uses std.variant.Variant[string] and usage isn’t too pretty (and @system).

Both could also be useful when implementing a middleware routing model.
Nevertheless, both also have apparent limitations/issues.

> It would mean that each request takes up an additional 16 bytes for the request, but I prefer this over the polymorphic class-based approach in Phobos. Yeah, I don’t think we have to go with classes here either. A relevant limitation if your union solution is, though, that it doesn’t support Unix domain sockets. I’m not sure whether we should add them to the union as well (and replace `isIPv6` with an enum-based type tag or something) or rather rethink the task as a whole. (Another minor issue is that unions aren’t accessible in `@safe` code.) Two ideas that come to mind are: - A server could add such information to a pseudo-header (pretty much like proxies do with `X-Forwarded-For`). The obvious limitation is that the value has to be either serialized to string or be a string in the first place. - A tagging mechanism that allows to add arbitrary attributes to HTTP messages. PSR-7 has this and it works well in PHP through dynamic typing; in D however probably this requires some sort of clunk. *oceandrift-http* uses [`std.variant.Variant[string]`](https://github.com/oceandrift/http/blob/v0.24.312/message/oceandrift/http/message/messages.d#L222) and [usage](https://github.com/oceandrift/http/blob/v0.24.312/microframework/oceandrift/http/microframework/httpauth.d#L60) isn’t too pretty ([and `@system`](https://github.com/oceandrift/http/blob/4cc7d92840617aeb44601c969d05abc367fc7023/microframework/oceandrift/http/microframework/cookies.d#L102)). Both could also be useful when implementing a middleware routing model. Nevertheless, both also have apparent limitations/issues.

0xEAB commented

2024-10-28 23:02:29 +00:00

(Another minor issue is that unions aren’t accessible in @safe code.)

Given that this library already uses std.typecons, would it be acceptable to use std.sumtype for the implementation here?

> (Another minor issue is that unions aren’t accessible in @safe code.) Given that this library already uses `std.typecons`, would it be acceptable to use `std.sumtype` for the implementation here?

andrew commented

2024-10-30 12:11:12 +00:00

I'd actually prefer to avoid using the existing standard library where at all possible (so that this code is largely compatible with Phobos3 or whatever).

In recent commits, I've removed the usage of std.typecons, and instead of std.sumtype, if you read the address.d file, you'll see that I make a barebones compound struct containing each possible address type. Realistically it's less than 32 bytes of total information, so I think it's still acceptable overhead.

I'd actually prefer to avoid using the existing standard library where at all possible (so that this code is largely compatible with Phobos3 or whatever). In recent commits, I've removed the usage of `std.typecons`, and instead of `std.sumtype`, if you read the `address.d` file, you'll see that I make a barebones compound struct containing each possible address type. Realistically it's less than 32 bytes of total information, so I think it's still acceptable overhead.

Sign in to join this conversation.

No Label

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Handy-Http/primitives#3