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

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

Open
g13013 opened this issue Jul 4, 2020 · 14 comments
Open

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

g13013 opened this issue Jul 4, 2020 · 14 comments

Comments

@g13013
Copy link

@g13013 g13013 commented Jul 4, 2020

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

@g13013 g13013 changed the title net/smtp: Add smt.SendMailWithDialer and make Client.localName public net/smtp: Add smt.SendMailWithClient and make Client.localName public Jul 4, 2020
@g13013 g13013 changed the title net/smtp: Add smt.SendMailWithClient and make Client.localName public proposal: net/smtp: Add smt.SendMailWithClient and make Client.localName public Jul 4, 2020
@gopherbot gopherbot added this to the Proposal milestone Jul 4, 2020
@gopherbot gopherbot added the Proposal label Jul 4, 2020
@ianlancetaylor ianlancetaylor changed the title proposal: net/smtp: Add smt.SendMailWithClient and make Client.localName public proposal: net/smtp: add SendMailWithClient and make Client.localName public Jul 4, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jul 4, 2020
@g13013 g13013 changed the title proposal: net/smtp: add SendMailWithClient and make Client.localName public proposal: net/smtp: add SendMailWithDialer and make Client.localName public Jul 4, 2020
@g13013 g13013 changed the title proposal: net/smtp: add SendMailWithDialer and make Client.localName public proposal: net/smtp: add SendMailWithDialer and make Client.WithLocalName Jul 5, 2020
@g13013 g13013 changed the title proposal: net/smtp: add SendMailWithDialer and make Client.WithLocalName proposal: net/smtp: add SendMailWithDialer Jul 5, 2020
@ianlancetaylor

This comment was marked as resolved.

@g13013

This comment was marked as resolved.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 8, 2020

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?

@rsc rsc moved this from Incoming to Active in Proposals Jul 8, 2020
@g13013
Copy link
Author

@g13013 g13013 commented Jul 10, 2020

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

@g13013
Copy link
Author

@g13013 g13013 commented Jul 10, 2020

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
Copy link

@gopherbot gopherbot commented Jul 10, 2020

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

@g13013
Copy link
Author

@g13013 g13013 commented Jul 10, 2020

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!

@g13013 g13013 changed the title proposal: net/smtp: add SendMailWithDialer proposal: net/smtp: allow for better control over Client Jul 10, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jul 15, 2020

@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
Copy link
Author

@g13013 g13013 commented Jul 20, 2020

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
Copy link
Contributor

@rsc rsc commented Jul 22, 2020

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 rsc changed the title proposal: net/smtp: allow for better control over Client proposal: net/smtp: add Client.TLSConfig field and Client.SendMail method Jul 22, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jul 22, 2020

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
Copy link
Author

@g13013 g13013 commented Jul 22, 2020

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
Copy link
Contributor

@rsc rsc commented Aug 5, 2020

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

@rsc rsc moved this from Active to Likely Accept in Proposals Aug 5, 2020
@dcormier
Copy link
Contributor

@dcormier dcormier commented Aug 6, 2020

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Likely Accept
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.