Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expose socket ADDR in http #1947

Closed
wants to merge 1 commit into from

Conversation

ytti
Copy link

@ytti ytti commented Dec 12, 2015

Unsure if this is the rigth way, perhaps openssl should inherit IPSocket or TCPSocket instead? Dunno how to do specs, as the MemoryIO does not have addrs, and unsure if it's correct place to add them.
Use case for this is so that web framework can easily tell client's IP.

[ytti@ytti.fi ~/usr/git/sykemittari]% ./moi
response.addr = Socket::Addr(@family="AF_INET", @ip_port=53380, @ip_address="194.100.7.227")
response.peeraddr = Socket::Addr(@family="AF_INET", @ip_port=80, @ip_address="62.78.98.162")
request.addr = Socket::Addr(@family="AF_INET", @ip_port=40748, @ip_address="127.0.0.1")
request.peeraddr = Socket::Addr(@family="AF_INET", @ip_port=1122, @ip_address="127.0.0.1")
^C
[130 ytti@ytti.fi ~/usr/git/sykemittari]% cat moi.cr
require "http/server"
require "http/client"

client = HTTP::Client.new("google.com")
response = client.get("/")
pp response.addr
pp response.peeraddr

server = HTTP::Server.new(1122) do |request|
pp request.addr
pp request.peeraddr
HTTP::Response.ok "text/plain", "yaya"
end

server.listen
[ytti@ytti.fi ~/usr/git/sykemittari]%

Unsure if this is the rigth way, perhaps openssl should inherit IPSocket or TCPSocket instead?

[ytti@ytti.fi ~/usr/git/sykemittari]% ./moi
response.addr = Socket::Addr(@family="AF_INET", @ip_port=53380, @ip_address="194.100.7.227")
response.peeraddr = Socket::Addr(@family="AF_INET", @ip_port=80, @ip_address="62.78.98.162")
request.addr = Socket::Addr(@family="AF_INET", @ip_port=40748, @ip_address="127.0.0.1")
request.peeraddr = Socket::Addr(@family="AF_INET", @ip_port=1122, @ip_address="127.0.0.1")
^C
[130 ytti@ytti.fi ~/usr/git/sykemittari]% cat moi.cr
require "http/server"
require "http/client"

client = HTTP::Client.new("google.com")
response = client.get("/")
pp response.addr
pp response.peeraddr

server = HTTP::Server.new(1122) do |request|
  pp request.addr
  pp request.peeraddr
  HTTP::Response.ok "text/plain", "yaya"
end

server.listen
[ytti@ytti.fi ~/usr/git/sykemittari]%
@ytti
Copy link
Author

ytti commented Dec 12, 2015

Perhaps more generic solution would be just to expose 'io'.
request.io.peeraddr.address

However this still remains OpenSSL as special-case, as it does not expose peeraddr, and it's obviously not nice to do something like
request.io.io.peeraddr.address after verifying that io is actually OpenSSL socket.

So maybe two things for clean solution
a) more generic/shared OpenSSL and normal socket
b) expose the generic socket

@ysbaddaden
Copy link
Contributor

I'm not sure about this, because we are always requesting the addr and peeraddr, whatever happens, and we most likely will never need this information.

We won't expose the socket since request.io.addr would be fiddling with internal implementation details. We could memoize the socket then delegate addr and peeraddr to it... but we don't need to retain the socket once the request is built. Actually, we can build a Request that doesn't come from a socket at all, it can be manually built or parsed from a MemoryIO.

I'm not sure Request should expose the addr and peeraddr. You may retain the sockets and pass them along with the request if you really need them (or inherit/wrap HTTP::Request to expose these informations)

@ytti
Copy link
Author

ytti commented Dec 12, 2015

What is the downside of exposing them? If we just extract the IP, that's up-to 256bits of new data stored? Considering how long headers etc can be, and how much more useful SADDR of request is, than lot of the other data, isn't it justifiable?
I can understand why you might not want to expose objects, because they can't be GC'd, but what about exposing just the data?

@sdogruyol
Copy link
Member

I'm also thinking of doing something like this to retrieve to client's ip info though i was thinking of reading it from headers.

@sdogruyol
Copy link
Member

Any update on this?

@sdogruyol
Copy link
Member

Ping

@asterite
Copy link
Member

We'll eventually do this, but not by adding this info to the Request object

@asterite asterite closed this Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants