flashmob / go-guerrilla

Mini SMTP server written in golang
MIT License
2.79k stars 366 forks source link

go-guerrilla doesn’t support “RCPT TO: <Postmaster>”. #201

Closed issuefiler closed 4 years ago

issuefiler commented 5 years ago

RFC 5321

   Any system that includes an SMTP server supporting mail relaying or
   delivery MUST support the reserved mailbox "postmaster" as a case-
   insensitive local name.  This postmaster address is not strictly
   necessary if the server always returns 554 on connection opening (as
   described in Section 3.1).  The requirement to accept mail for
   postmaster implies that RCPT commands that specify a mailbox for
   postmaster at any of the domains for which the SMTP server provides
   mail service, as well as the special case of "RCPT TO:<Postmaster>"
   (with no domain specification), MUST be supported.
   o  The reserved mailbox name "postmaster" may be used in a RCPT
      command without domain qualification (see Section 4.1.1.3) and
      MUST be accepted if so used.
   Syntax:

      rcpt = "RCPT TO:" ( "<Postmaster@" Domain ">" / "<Postmaster>" /
                  Forward-path ) [SP Rcpt-parameters] CRLF

                  Note that, in a departure from the usual rules for
                  local-parts, the "Postmaster" string shown above is
                  treated as case-insensitive.

Test

Preset

"allowed_hosts": ["a.com"]

Expectation

Case # 1

RCPT TO: <postMaStER@a.com>
250 2.1.5 OK

Case # 2

RCPT TO: <Postmaster@not-a.com>
454 4.1.1 Error: Relay access denied: not-a.com

Case # 3

RCPT TO: <poSTmAsteR>
250 2.1.5 OK

Case # 4

EDITED

RCPT TO: <"po\ST\mAs\t\eR">
250 2.1.5 OK

Current

Case # 1 ✅

RCPT TO: <postMaStER@a.com>
250 2.1.5 OK

Case # 2 ✅

RCPT TO: <Postmaster@not-a.com>
454 4.1.1 Error: Relay access denied: not-a.com

Case # 3 ❌

RCPT TO: <poSTmAsteR>
454 4.1.1 Error: Relay access denied: 

Case # 4 ❌ ~✅~ EDITED

RCPT TO: <"po\ST\mAs\t\eR">
501 5.5.4 Invalid address

~Fix (not tested)~

  1. ~Validate the syntax per the ABNF. This must be done prior to the step 2, or it would break on the case # 4.~
  2. ~If the Host is empty and the User is case-insensitively Postmaster, say OK.~

https://github.com/flashmob/go-guerrilla/blob/51f7dda326b1e9878e5f679ccb34a134127951b0/server.go#L492-L503

                if s.allowsHost(to.Host) || (to.Host == "" && strings.toLower(to.User) == "postmaster") {
                    client.PushRcpt(to)
                    rcptError := s.backend().ValidateRcpt(client.Envelope)
                    if rcptError != nil {
                        client.PopRcpt()
                        client.sendResponse(r.FailRcptCmd, " ", rcptError.Error())
                    } else {
                        client.sendResponse(r.SuccessRcptCmd)
                    }
                } else {
                    client.sendResponse(r.ErrorRelayDenied, " ", to.Host)
                }
flashmob commented 5 years ago

The ABNF has already been parsed in parse.go

TODO:

There's a question - what Address.host should we default to? Should we fill any value there? Leave blank, or add a new config option, or just use 'localhost' ?

issuefiler commented 5 years ago

The RFC seems to be a bit unclear for "po\ST\mAs\t\eR" and "po\ST\mAs\t\eR"@example.com inputs, but what you’ve said would be the right path to go, I guess; edited.

And for the Address.Host of <Postmaster>: we could simply leave the Host blank (as is) and modify the stringifying steps for use by processors (#199).

flashmob commented 4 years ago

Hi @issuefiler, Please see PR #202 - it's ready for your acceptance test :-)

It works a little different from what was specified above (https://github.com/flashmob/go-guerrilla/issues/201#issuecomment-557957171) It simply captures quoted local-parts without the escape characters, and then the String() function knows if it was quoted or not. So if the local-part is "test\" " then it will be stored as "test " ". So, allowsHost didn't need any modification after all.

When testing:

Note that if <postmaster> is used in the RCPT TO (without a host), then new functionality was added to assume that the host is assumed to be the Hostname setting for the Server. Therefore, we must ensure that the Hostname setting also exists in the AllowedHosts config setting. (The server will log a warning if it detects this case)