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

Added proxy support #21

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Added proxy support #21

wants to merge 6 commits into from

Conversation

Techno-Fox
Copy link

Regarding the proxy support, this seems to work. Instead of complicating things with tor.nim, this seems WAY easier to maintain.

@Araq Araq requested a review from dom96 September 7, 2020 06:57
Copy link
Owner

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right approach but this isn't mergeable yet.

Can you implement it for async too?

src/irc.nim Outdated
Comment on lines 306 to 307
p[0] = cast[char](irc.port.uint16 shr 8)
p[1] = cast[char](irc.port)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to cast here, can't you do a simple type conversion? It's much safer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trying type conversion for line 307 results in

Error: unhandled exception: value out of range [RangeError]

casting seems to be the only way I can get it to work. Unless there's a way to convert it I'm not seeing?

src/irc.nim Outdated
Comment on lines 328 to 337

# proc connect*(s: ProxySocket, address: string, port: Port) =
# ## Connect by FQDN/hostname or IP address
# ## echo repr port.uint16.toHex().fromHex()
# var p = " "
# p[0] = cast[char](port.uint16 shr 8)
# p[1] = cast[char](port)

# s.inner.send("\x05\x01\x00\x03" & address.len.char & address & p)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. Will do

src/irc.nim Outdated
Comment on lines 352 to 353


Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

src/irc.nim Outdated
Comment on lines 311 to 312
irc.sock.send("\x05\x01\x00") # connect with no auth
let resp = irc.sock.recv(2, 1000)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this defined? Can you link to the spec/RFC? Reading 2 bytes looks suspicious to me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats getting the proxy response.

@Techno-Fox
Copy link
Author

waiting for review.

Copy link
Owner

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're still casting, and now have 4(!?) copies of the same code?

@Techno-Fox
Copy link
Author

I tried type conversion, but it doesn't work without it. With type conversion it will compile, but won't work for some reason, as in it wont connect.

@Techno-Fox
Copy link
Author

Techno-Fox commented Sep 17, 2020

I'll fix the 4 copies

@Techno-Fox
Copy link
Author

I fixed the copies, but the casting is confusing me. Why does type conversion not connect, but with casting it does?

Copy link
Owner

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the binary files as well.

Comment on lines +590 to +602
var p = " "
p[0] = char(irc.port.uint16 shr 8)
p[1] = cast[char](irc.port)

# connect to proxy with not authentication
await irc.sock.connect(irc.proxyAddr, irc.proxyPort)
await irc.sock.send("\x05\x01\x00") # connect with no auth
# get proxy response
let resp = await irc.sock.recv(2)
if resp != "\x05\x00": # check the proxy response
raise newException(Exception, "Unexpected proxy response: " & resp.toHex())

await irc.sock.send("\x05\x01\x00\x03" & irc.address.len.char & irc.address & p)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reduce duplication even further:

proc connectProxy(irc: Irc | AsyncIrc) {.multisync.} =
    var p = "  "
    p[0] = char(irc.port.uint16 shr 8)
    p[1] = cast[char](irc.port)

    # connect to proxy with not authentication
    await irc.sock.connect(irc.proxyAddr, irc.proxyPort)
    await irc.sock.send("\x05\x01\x00") # connect with no auth
    # get proxy response
    let resp = await irc.sock.recv(2)
    if resp != "\x05\x00": # check the proxy response
      raise newException(Exception, "Unexpected proxy response: " & resp.toHex())

    await irc.sock.send("\x05\x01\x00\x03" & irc.address.len.char & irc.address & p)

await connectProxy(irc) # and without await above.

Comment on lines +41 to +42
proxyAddr: string
proxyPort: Port
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I assume that this is a socks proxy, change all proxy to socksProxy. Not just here, but below as well.

Suggested change
proxyAddr: string
proxyPort: Port
socksProxyAddr: string
socksProxyPort: Port

@dom96
Copy link
Owner

dom96 commented Jul 4, 2021

ping

@dom96 dom96 mentioned this pull request Jul 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants