net/smtp: add Client.TLSConfig field and Client.SendMail method

[g13013]

smtp.SendMail actually does a great job, but the fact that it always uses smtp.Dial make it less flexible both for testing and for certain cases like ours, where we wanted to customize the Client.localName

In fact, in certain operations, Client.localName must correspond to the FQDN hostname sending the email (always localhost in the current implementation), this lack of flexibility led us to duplicate the entire SendMail (and tests) code just to address this issue.

I propose that we add a new public method smtp.SendMailWithDialer or smtp.SendMailWithDialerFunc and a new type Dialer/DialerFunc

type Dialer func() (*Client, error) // or DialerFunc ?

func SendMailWithDialer(dial Dialer, a Auth, from string, to []string, msg []byte) error

The change set required is very minimal and backward compatible, smtp.SendMail could use smtp.SendMailWithDialer just by passing smtp.Dial as the dialer:

func makeDialer(addr string) func() (*Client, error) {
    return func() (*Client, error) {
        return Dial(addr)
    }
}
func SendMail(addr string, a Auth, from string, to []string, msg []byte) error {
    return SendMailWithDialer(makeDialer(addr), a, from, to, msg)
}

This would allow for much more control both for testing and real uses cases with minimum code, for example:

func myDialer() (*smtp.Client, error) {
    cl, err := Dial("remoteSmtp:25")
    if err != nil { return nil, err }
    if err := cl.Hello("my_FQDN") {
        return nil, err
    }
    return cl, nil
}

smtp.SendMailWithDialer(myDialer, auth, from, to, msg)

I am willing to make a CL if it is accepted

Edit:

• At first the proposal was about adding SendMailWithClient, but it turned out that line validation would be duplicate both in SendMailWithClient and SendMail as the validation must occur before the dial

• The proposal included a proposal to make config.localName public, but since Client.Hello exists and SendMail uses Client.hello internally which does nothing if a Hello was called before this change is no longer required as the Dialer function could call Hello before passing to SendMailWithDialer

[ianlancetaylor]

@g13013 Please only send patches using Gerritt or as a GitHub pull request, as described at http://golang.org/doc/contribute.html. Please don't post patches on the issue tracker. That avoids any concerns about copyright. Thanks.

[g13013]

@ianlancetaylor oh didn't know about that, sorry ! comment removed

[rsc]

Originally, smtp.SendMail was a tiny little wrapper around the Client type, a convenience. It has gotten rather large as TLS and various validations were added.

@bradfitz points out maybe it would make sense to add a Client.SendMail method. Then you can make the Client c however you want and call c.SendMail. And it avoids a callback in favor of much more direct code.

Thoughts?

[g13013]

@rsc interesting, sounds good to me. I think it's even better than the original proposal.

[g13013]

Last thought,

I tend to think that adding Client.SendMail would solve the original issue but would not make smtp.Client as usable as it should be unless we include the Dial capability to initialize its state.

ex. : If a user wants to customize the TLS configuration to add a valid CA, a naive solution would be to extend Client and override Client.StartTLS, but since the Client.conn & Client.serverName are hidden and initialized only via smtp.Dial or smtp.NewClient which always return a smtp.Client struct, extending would not be possible. An example of this could be found in smtp.SendMail where the hook has been added for testing only and used here.

A solution to this would be to add Client.Dial in addition to Client.SendMail? which does pretty much what smtp.Dial and smtp.NewClient do all together, this would make the Client fully extensible and even make the Client instance reusable for further operations, if so, this would make smtp.Dial, smtp.NewClient and smtp.SendMail wrappers only for Client

what do you think ?

PS: We could even add Client.DialAndSend for convenience only!

[gopherbot]

Change https://golang.org/cl/242017 mentions this issue: net/smtp: allow more control on Client

[g13013]

I thought a CL with all possible changes would be better to visualize the needs. I am not sure I did well though, sorry for any inconvenience!

[rsc]

@g13013, thanks for sketching the API. Let's leave DialAndSend out, since that can be done as two separate calls.

I don't quite understand the need for c.Dial. A Client is an already connected connection. You get one by calling smtp.Dial or by using net.Dial + smtp.NewClient. It doesn't seem necessary to expand the API to contemplate a not-yet-dialed Client.

I think I followed what you said about wanting to modify the TLS config used by STARTTLS, but I don't see how any of the new API in the example CL enables that flexibility. If we need that flexibility, it seems like we could export the tls config as a field in the Client. Then you use smtp.Dial or net.Dial+smtp.NewClient to get a Client c, set up the TLS config, and call c.SendMail. The new exported field is the only change that seems to be necessary (along with c.SendMail as discussed earlier) to enable custom TLS configs.

Do I have this right?

[g13013]

I think I followed what you said about wanting to modify the TLS config used by STARTTLS, but I don't see how any of the new API in the example CL enables that flexibility

You are absolutely right, I've put all the problems that I see in the Client in one CL, and mislead you with my comment about TLS, it was obviously a poor argumentation.

It doesn't seem necessary to expand the API to contemplate a not-yet-dialed Client

You are probably right, but I wanted to suggest that Client.SendMail will have to do various checks before actually Dialing.

We can say that exporting a tls.Config field and moving SendMail to Client are the best choices we have.

One note about the change, before moving SendMail to Client, the validation occurs before the Dial, now, the validation of localName and recipients occur always after the Dial, . it could be harmful in the sense that the IP could get very poor reputation in case of an attack, is including an example to enforce best practices sufficient ? Also, it feels odd that the same validation in this case will occur multiple times, before the dial and after from within SendMail and Hello !

Or can we conside keeping the same signature of Client.SendMail, with addr and make it dial for a new connection if addr is not empty (no need to export Client.Dial), if we export also Client.LocalName, all the validation could occur before actually making any transaction with the remote system

cl := &smtp.Client{LocalName: "fqdn", TlsConfig: myTlsConf}
cl.SendMail(addr, auth, from, to, msg) // checks all, dials and send

[rsc]

Or can we conside keeping the same signature of Client.SendMail, with addr and make it dial for a new connection if addr is not empty

It seems a bit odd to do that - right now all Clients are pre-Dialed. You'd be introducing a new kind of Client, for not much win. The validation happening after the Dial doesn't seem like a huge problem. Connections get lost all the time, and how often does SendMail get called with invalid arguments?

[rsc]

I've retitled this to note that the changes are (1) add a TLSConfig *tls.Config field to smtp.Client, like in http.Server; and (2) add a SendMail method.

Does anyone object to this change?

[g13013]

and how often does SendMail get called with invalid arguments?

not often I guess, unless hackers involved

right now all Clients are pre-Dialed. You'd be introducing a new kind of Client

emm, that's a good point

ok then, I think everything have been said, if it's accepted I'll update the CL insha'Allah. thanks @rsc

[rsc]

Based on the discussion above, this (add Client.TLSConfig field and Client.SendMail method) seems like a likely accept.

[dcormier]

How would a new exposed field for *tls.Config on Client interact with the existing (*Client).StartTLS(*tls.Config) method that allows you to specify the *tls.Config to use for STARTTLS?

[rsc]

It seems like if you call StartTLS manually with a TLS config, that would override the thing in the struct.

[rsc]

No change in consensus, so accepted.

[g13013]

Sorry for the delay, I had a lot of work to finish these last days. I've updated the CL for the accepted changes.

As mentioned in the CL, Client.StartTLS was not checking if the connection is already using tls, for ex when created *tls.Conn with NewClient, so i've changed it to return an error in this case. Also Client.SendMail does not try to STARTTLS if already using a tls connection. I don't know if you are ok with this but I think it's necessary.

let me know of changes are required.

[g13013]

Is there anything new for the CL?