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

server: XCLIENT implementation #114

Closed
wants to merge 1 commit into from
Closed

Conversation

foxcpp
Copy link
Collaborator

@foxcpp foxcpp commented Jul 15, 2020

Some interface details probably need discussion.

@emersion
Copy link
Owner

Hmm, this backend API is a little bit unfortunate. Would there be a way to only have a new Session method?

@foxcpp
Copy link
Collaborator Author

foxcpp commented Jul 24, 2020

Hmm, this backend API is a little bit unfortunate. Would there be a way to only have a new Session method?

It is due to the fact XCLIENT is valid both before and after AUTH and may work differently depending on authentication status. I initially thought about avoiding two methods by creating a session using anonymous callback but it may be problematic since backend may permit XCLIENT but not unauthenticated transactions and permit them after XCLIENT. ( On a second thought that setup does not really make a lot of sense, not sure... )

If I had the liberty of doing a major interface change I would ditch authentication-centered Session creation and add Auth() method to Session, create one session per connection and be done with it (we already have one session per connection but the way it is initially created is rather convoluted).

@emersion
Copy link
Owner

If I had the liberty of doing a major interface change I would ditch authentication-centered Session creation and add Auth() method to Session, create one session per connection and be done with it (we already have one session per connection but the way it is initially created is rather convoluted).

Agreed. Do you think it's worth it to break the API this way?

@foxcpp
Copy link
Collaborator Author

foxcpp commented Jul 7, 2021

I believe HAProxy's PROXY protocol is more widely supported therefore it is a good idea to implement it first.

@foxcpp
Copy link
Collaborator Author

foxcpp commented Jul 7, 2021

It is also much more simple than XCLIENT which is SMTP-focused. It might be a good idea to support it too if we take into account SMTP-aware proxies.

@kayrus
Copy link
Contributor

kayrus commented Jul 8, 2021

HAProxy's PROXY protocol is easy to implement using the https://github.com/pires/go-proxyproto package:

                        // Wrap listener in a proxyproto listener
                        proxyListener := &proxyproto.Listener{Listener: smtpListener}
                        err := smtpServer.Serve(proxyListener)

@foxcpp
Copy link
Collaborator Author

foxcpp commented Jul 8, 2021

Closing as I do not intend to continue work on XCLIENT in foreseeable future.

@foxcpp
Copy link
Collaborator Author

foxcpp commented Jul 16, 2021

After a more careful research I made the decision it is useful to support both PROXY and XCLIENT since XCLIENT is more flexible and is compatible with connection pooling while PROXY is not.

@xdkaka
Copy link

xdkaka commented Jun 16, 2022

HAProxy's PROXY protocol is easy to implement using the https://github.com/pires/go-proxyproto package:

                        // Wrap listener in a proxyproto listener
                        proxyListener := &proxyproto.Listener{Listener: smtpListener}
                        err := smtpServer.Serve(proxyListener)

In this way, is the proxy server able to forge the user's real IP?

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.

4 participants