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

Enable LMTP over TCP #123

Closed
wants to merge 4 commits into from
Closed

Enable LMTP over TCP #123

wants to merge 4 commits into from

Conversation

patrick246
Copy link

Changes:

  • remove check that forbids LMTP over TCP
  • introduce Network config option, to choose between TCP and unix sockets
  • add a warning to the documentation that LMTP offers no authentication and should not be hosted unprotected

remove check that forbids LMTP over TCP
introduce Network config option, to choose between TCP and unix sockets
add a warning to the documentation that LMTP should not be used for WAN communication
test default Network config behavior
test error for empty lmtp address
test lmtp listening on tcp port
server.go Outdated Show resolved Hide resolved
s.Domain = "localhost"
s.AllowInsecureAuth = true

testPort := rand.Int31n(65535-1024) + 1024
Copy link
Owner

Choose a reason for hiding this comment

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

This is fragile because the port could be in use. Use port zero instead.

Copy link
Author

Choose a reason for hiding this comment

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

Totally agree, do you know a way to get the actually used port then? The other test code uses Serve with a custom listener, this here tests logic in ListenAndServe and can't use that

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actual listening port should be available via net.Listener.Addr().

server.go Outdated Show resolved Hide resolved
// Listen configures a listener according to the server configuration, without TLS
// This listener can then be passed to Serve. If access to the listener is not needed, then the
// combined function ListenAndServe can be used
func (s *Server) Listen() (net.Listener, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's not expose this

Copy link
Author

Choose a reason for hiding this comment

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

The other option would be to put the tests that test the listener setup inside the smtp package. Would that be okay?

// ListenTLS configures a listener according to the server configuration, with TLS
// This listener can then be passed to Serve. If access to the listener is not needed, then the
// combined function ListenAndServe can be used
func (s *Server) ListenTLS() (net.Listener, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto

server.go Show resolved Hide resolved

s.Network = ""
s.LMTP = true
s.Addr = "./testsocket"
Copy link
Owner

Choose a reason for hiding this comment

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

We can't do that, this will clutter the current working directory.

Copy link
Author

Choose a reason for hiding this comment

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

We could put it in os.TempDir()

@emersion
Copy link
Owner

emersion commented Nov 9, 2023

Superseded by #212

@emersion emersion closed this Nov 9, 2023
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.

None yet

3 participants