code review 2052042: smtp: new package

smtp: new package

[eds, 2010-08-29 17:21:46]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[eds, 2010-08-30 17:45:23]

On Sun, Aug 29, 2010 at 12:21 PM,  <chickencha@gmail.com> wrote:
> Reviewers: rsc,
>
> Message:
> Hello rsc (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> smtp: new package
>
> Please review this at http://codereview.appspot.com/2052042/
>
> Affected files:
>  A src/pkg/smtp/Makefile
>  A src/pkg/smtp/smtp.go
>  A src/pkg/smtp/smtp_test.go
>
>
>

FYI, in case you've looked at this some already, I reworked Auth and
its implementors a little bit so they're not so SMTP-specific.

- Evan

[rsc1, 2010-09-08 15:54:27]

Looks like a good start.
I want to think more about the Auth interface.
Are there multistage auth protocols?
It would be good to implement one to 
see how well it works.

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go
File src/pkg/smtp/smtp.go (right):

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode32
src/pkg/smtp/smtp.go:32: extensions map[string]string
s/extensions/ext/

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode36
src/pkg/smtp/smtp.go:36: func Dial(addr string) (*Client, int, string, os.Error)
{
Expected (*Client, os.Error).
Saving the greeting text is fine but it should
be in the struct.

All the methods that return int, string, os.Error
should return os.Error.  If the code indicates an
error, the os.Error will describe it (it's a *textproto.ProtocolError).
And if the code doesn't indicate an error, it's very
uncommon for the message to matter.  When it does,
return just that, only when err == nil.

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode44
src/pkg/smtp/smtp.go:44: // NewClient returns a new Client using an existing
connection.
Expected return (*Client, os.Error)

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode66
src/pkg/smtp/smtp.go:66: // Helo sends the HELO greeting to the server. Helo
should be used only when the
Can the client figure this out?
A nicer API would be

// Helo sends the HELO (or, if the server supports it, the EHLO) greeting to the
server.

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode115
src/pkg/smtp/smtp.go:115: func (c *Client) Verify(addr string) (int, string,
os.Error) {
For example, this should be Verify(addr string) os.Error
and do

_, _, err := c.cmd(250, "VRFY %s", addr)
return err

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode120
src/pkg/smtp/smtp.go:120: // TODO(eds): Should Auth be in a separate package? It
could be useful for other protocols.
No.  SMTP should define what it needs, and if other
packages use the same interface, great.  That's the
nice thing about interfaces.

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode120
src/pkg/smtp/smtp.go:120: // TODO(eds): Should Auth be in a separate package? It
could be useful for other protocols.
Please do move the auth implementatons to a separate auth.go.

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode234
src/pkg/smtp/smtp.go:234: return c.cmd(250, cmdStr, from)
same sort of thing
_, _, err := c.cmd(250, cmdStr, from)
return err

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode240
src/pkg/smtp/smtp.go:240: func (c *Client) Rcpt(to string) (int, string,
os.Error) {
same

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode244
src/pkg/smtp/smtp.go:244: // Data issues a DATA command to the server using the
provided data.
Data should probably return an io.WriteCloser and let the caller
write the data directly.  Then it doesn't have to be in []byte form.

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode246
src/pkg/smtp/smtp.go:246: func (c *Client) Data(data []byte) (code int, msg
string, err os.Error) {
same
func (c *Client) Data() (io.WriteCloser, os.Error)

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode262
src/pkg/smtp/smtp.go:262: c, _, _, err := Dial(addr)
All the _ in this function prove the point.  :-)

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode299
src/pkg/smtp/smtp.go:299: // The extension name is case-insensitive. If the
extension is support,
s/support/supported/
can probably drop Info (s/ExtensionInfo/Extension/)

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode313
src/pkg/smtp/smtp.go:313: func (c *Client) Reset() (int, string, os.Error) {
same

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode318
src/pkg/smtp/smtp.go:318: func (c *Client) Quit() os.Error {
lgtm

[eds, 2010-09-08 16:39:33]

Thanks for the review.

The LOGIN mechanism is multi-stage. The server issues two challenges: one for
user name and one for password.

The Auth interface models an SASL authentication mechanism, which allows for any
number of challenges, but I haven't seen a mechanism with more than two. I'm not
an expert, though. It would be nice to see some more complicated Auth
implementations, but most SMTP servers don't even support mechanisms beyond
PLAIN and LOGIN, so I've found it difficult to test.

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go
File src/pkg/smtp/smtp.go (right):

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode66
src/pkg/smtp/smtp.go:66: // Helo sends the HELO greeting to the server. Helo
should be used only when the
On 2010/09/08 15:54:28, rsc1 wrote:
> Can the client figure this out?

The only way is to first send EHLO, and if the server doesn't accept it, fall
back to HELO. The client can do this right now, but the package could do it
instead.

http://codereview.appspot.com/2052042/diff/8002/src/pkg/smtp/smtp.go#newcode120
src/pkg/smtp/smtp.go:120: // TODO(eds): Should Auth be in a separate package? It
could be useful for other protocols.
On 2010/09/08 15:54:28, rsc1 wrote:
> No.  SMTP should define what it needs, and if other
> packages use the same interface, great.  That's the
> nice thing about interfaces.
> 

Sure, but it still seems odd to me to duplicate the same interface across
several packages. You don't see duplicates of io.Reader running around
everywhere. If there's ever a general SASL package, I think this interface and
its implementers belong there.

Anyway, it's a topic for another day. I'll remove the todo and create auth.go.

[rsc, 2010-09-08 17:11:56]

>> Can the client figure this out?
>
> The only way is to first send EHLO, and if the server doesn't accept it,
> fall back to HELO. The client can do this right now, but the package
> could do it instead.

I thought that you could send EHLO if the server greeting said ESMTP?

Russ

[agl1, 2010-09-08 17:16:37]

On Wed, Sep 8, 2010 at 1:11 PM, Russ Cox <rsc@golang.org> wrote:
> I thought that you could send EHLO if the server greeting said ESMTP?

The specified version negotiation involves an opportunistic EHLO:

http://www.ietf.org/rfc/rfc1869.txt (see section 4).

Although using the server's status line would have been a better idea, I agree.


AGL

[iant2, 2010-09-08 17:27:47]

Russ Cox <rsc@golang.org> writes:

>>> Can the client figure this out?
>>
>> The only way is to first send EHLO, and if the server doesn't accept it,
>> fall back to HELO. The client can do this right now, but the package
>> could do it instead.
>
> I thought that you could send EHLO if the server greeting said ESMTP?

Technically you can send EHLO regardless, and fall back to HELO if EHLO
gets an error response.  But I believe sendmail only sends EHLO if the
string ESMTP appears somewhere in the server's greeting.

Ian

[iant2, 2010-09-08 17:35:29]

Russ Cox <rsc@golang.org> writes:

>>> Can the client figure this out?
>>
>> The only way is to first send EHLO, and if the server doesn't accept it,
>> fall back to HELO. The client can do this right now, but the package
>> could do it instead.
>
> I thought that you could send EHLO if the server greeting said ESMTP?

Technically you can send EHLO regardless, and fall back to HELO if EHLO
gets an error response.  But I believe sendmail only sends EHLO if the
string ESMTP appears somewhere in the server's greeting.

Ian

[eds, 2010-09-09 04:05:08]

PTAL

Helo and Ehlo are still separate for now. In practice it does seem true that
servers supporting EHLO will have "ESMTP" somewhere in their greeting. I'd
prefer to do things by the book, though. If we can find anywhere that says it's
okay to look for ESMTP in the greeting, that's fine with me. I do seem to recall
reading that somewhere, but I can't find it now.

[rsc, 2010-09-09 16:50:36]

> Helo and Ehlo are still separate for now. In practice it does seem true
> that servers supporting EHLO will have "ESMTP" somewhere in their
> greeting. I'd prefer to do things by the book, though. If we can find
> anywhere that says it's okay to look for ESMTP in the greeting, that's
> fine with me. I do seem to recall reading that somewhere, but I can't
> find it now.

Both sendmail and qmail agree that you should use EHLO
if the greeting says ESMTP, HELO otherwise.  Even if it's
not in the RFC, the installed base means that it's what
servers expect of the clients, and we should do the same.

Also, this logic should be part of Dial.  That is, Dial should
do the initial EHLO instead of having a separate method,
and Starttls should do the second EHLO.

That should simplify SendMail and clients like it.

Russ

[rsc1, 2010-09-09 18:44:30]

http://codereview.appspot.com/2052042/diff/26001/src/pkg/smtp/auth.go
File src/pkg/smtp/auth.go (right):

http://codereview.appspot.com/2052042/diff/26001/src/pkg/smtp/auth.go#newcode12
src/pkg/smtp/auth.go:12: // Auth represents an authentication mechanism.
This interface does not allow the one-message authentication
demonstrated in the RFCs.  The extra round trip may be significant
on a long-distance connection, and some servers might not
support it because it's not common.  So we should make it 
possible to specify the AUTH data.

It's easy to believe that the choice of protocol could depend
on the server name, and it definitely depends on whether TLS
negotiation succeeded.  (You don't want to use plain-text login
over an unsecured connection.)

It's not clear to me that the authentication protocol always
knows when it's going to be done: more is a property of the
server response, not necessarily the client.

String is typically intended for printing.  Making it have a
semantic meaning here is a bit restrictive.  (An auth object
might want to print itself as "LOGIN:rsc@golang.org" for example.)
A better name would be Protocol or Name, but it can be
rolled into the start message.

I'd suggest something along the lines of this:

// Auth is implemented by an SMTP authentication mechanism.
type Auth interface {
    // Start begins an authentication with the named server.
    // It returns the name of the authentication protocol
    // and optionally data to include in the initial AUTH message
    // sent to the server.  It can return proto == "" to indicate
    // that authentication should be skipped.
    // If it returns a non-nil os.Error, the SMTP client aborts
    // the authentication attempt and hangs up connection.
    Start(server string, usingTLS bool) (proto string, toServer []byte, err
os.Error)

    // Next continues the authentication.  The server has just sent
    // the fromServer data.  If more is true, the server expects a
    // response, which next should return as toServer; otherwise
    // Next should return toServer == nil.
    // If Next returns a non-nil os.Error, the SMTP client aborts
    // the authentication attempt and hangs up connection.
    Next(fromServer []byte, more bool) (toServer []byte, err os.Error)
}

Reset is probably unnecessary: it is implied by Start.
The more flag could be done away with by having 

    Next(fromServer []byte) (toServer []byte, os.Error)
    Finish(fromServer []byte) os.Error

but I'm not sure it would be simpler.

Russ

http://codereview.appspot.com/2052042/diff/26001/src/pkg/smtp/smtp.go
File src/pkg/smtp/smtp.go (right):

http://codereview.appspot.com/2052042/diff/26001/src/pkg/smtp/smtp.go#newcode8
src/pkg/smtp/smtp.go:8: //	AUTH	RFC 2554
align with others
(use spaces instead of tabs here; tabs are
only ever at the beginning of lines)

http://codereview.appspot.com/2052042/diff/26001/src/pkg/smtp/smtp.go#newcode123
src/pkg/smtp/smtp.go:123: expect := func(more bool) int {
might as well inline below

http://codereview.appspot.com/2052042/diff/26001/src/pkg/smtp/smtp.go#newcode130
src/pkg/smtp/smtp.go:130: more := true
A little awkward.
I'd suggest dropping this, changing the loop to for err == nil {,
and adding if !more { break } at the end of the loop.
Then you can use := on the a.Challenge line too,
dropping the declaration of resp

[eds, 2010-09-12 15:15:08]

On Thu, Sep 9, 2010 at 1:44 PM,  <rsc@google.com> wrote:
> // Auth is implemented by an SMTP authentication mechanism.
> type Auth interface {
>    // Start begins an authentication with the named server.
>    // It returns the name of the authentication protocol
>    // and optionally data to include in the initial AUTH message
>    // sent to the server.  It can return proto == "" to indicate
>    // that authentication should be skipped.
>    // If it returns a non-nil os.Error, the SMTP client aborts
>    // the authentication attempt and hangs up connection.
>    Start(server string, usingTLS bool) (proto string, toServer []byte,
> err os.Error)

How about passing server-supported auth mechanisms (as a slice of
strings?) instead of server name? That seems more generally useful to
me.

- Evan

[rsc, 2010-09-12 15:47:06]

On Sun, Sep 12, 2010 at 11:15, Evan Shaw <chickencha@gmail.com> wrote:
> On Thu, Sep 9, 2010 at 1:44 PM,  <rsc@google.com> wrote:
>> // Auth is implemented by an SMTP authentication mechanism.
>> type Auth interface {
>>    // Start begins an authentication with the named server.
>>    // It returns the name of the authentication protocol
>>    // and optionally data to include in the initial AUTH message
>>    // sent to the server.  It can return proto == "" to indicate
>>    // that authentication should be skipped.
>>    // If it returns a non-nil os.Error, the SMTP client aborts
>>    // the authentication attempt and hangs up connection.
>>    Start(server string, usingTLS bool) (proto string, toServer []byte,
>> err os.Error)
>
> How about passing server-supported auth mechanisms (as a slice of
> strings?) instead of server name? That seems more generally useful to
> me.

Really?  You don't want to know the TLS-verified server
name before you send it a password?  I sure do.

I agree that the auth mechanisms are useful too.
Probably it should be a struct so that things can
be added:

Start(server ServerInfo) (proto string, toServer []byte, err os.Error)

type ServerInfo struct {
    Name string  // name used in greeting, verified with TLS
    TLS bool  // started TLS
    Auth []string  // advertised authentication mechanisms
}

Russ

[eds, 2010-09-16 13:52:59]

On Thu, Sep 9, 2010 at 1:44 PM,  <rsc@google.com> wrote:
>    // Next continues the authentication.  The server has just sent
>    // the fromServer data.  If more is true, the server expects a
>    // response, which next should return as toServer; otherwise
>    // Next should return toServer == nil.
>    // If Next returns a non-nil os.Error, the SMTP client aborts
>    // the authentication attempt and hangs up connection.
>    Next(fromServer []byte, more bool) (toServer []byte, err os.Error)
> }
>
> The more flag could be done away with by having
>
>    Next(fromServer []byte) (toServer []byte, os.Error)
>    Finish(fromServer []byte) os.Error
>
> but I'm not sure it would be simpler.

Sorry, one more question before I upload another patch. Is more still
necessary? By the time the server says we're done, authentication has
already succeeded or failed, so there's nothing more that the
authentication mechanism can do.

It gives an opportunity to error out if the mechanism is expecting
another challenge, but you said yourself that more seems like a
property of the server response. It seems simpler to just not call
Next again when the server's done sending challenges.

Am I missing something?

- Evan

[rsc, 2010-09-16 17:07:12]

> Sorry, one more question before I upload another patch. Is more still
> necessary? By the time the server says we're done, authentication has
> already succeeded or failed, so there's nothing more that the
> authentication mechanism can do.
>
> It gives an opportunity to error out if the mechanism is expecting
> another challenge, but you said yourself that more seems like a
> property of the server response. It seems simpler to just not call
> Next again when the server's done sending challenges.
>
> Am I missing something?

Yes: the client might be authenticating the server too.
That last response might be important for that process,
and the os.Error lets that process fail.

Russ

[eds, 2010-09-16 17:53:01]

Hello rsc, agl, iant (cc: golang-dev@googlegroups.com),

Please take another look.

[eds, 2010-09-27 15:18:43]

On Thu, Sep 16, 2010 at 12:53 PM,  <chickencha@gmail.com> wrote:
> Hello rsc, agl, iant (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/2052042/
>

Ping

- Evan

[rsc1, 2010-09-27 15:43:23]

Looking pretty good.  A few small things.

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/auth.go
File src/pkg/smtp/auth.go (right):

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/auth.go#newcode14
src/pkg/smtp/auth.go:14: // Start begins and authentication with the named
server.
s/and/an/
s/the named/a/

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/auth.go#newcode20
src/pkg/smtp/auth.go:20: // the authentication attempt and hangs up connection.
s/hangs up/closes the/

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/auth.go#newcode28
src/pkg/smtp/auth.go:28: // the authentication attempt and hangs up connection.
s/hangs up/closes the/

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/auth.go#newcode32
src/pkg/smtp/auth.go:32: // A ServerInfo provides information about a server
that may be useful
// ServerInfo records information about an SMTP server.

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/auth.go#newcode35
src/pkg/smtp/auth.go:35: Name string   // name used in greeting, verified with
TLS
// name used in greeting

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/auth.go#newcode36
src/pkg/smtp/auth.go:36: TLS  bool     // started TLS
using TLS, with valid certificate for Name

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/auth.go#newcode44
src/pkg/smtp/auth.go:44: // NewAuthLogin returns an Auth that implements the
LOGIN authentication
Since there's no AuthLogin type this can just be AuthLogin,
or in the more usual little-endian naming, LoginAuth:
a = smtp.LoginAuth(...)

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/auth.go#newcode46
src/pkg/smtp/auth.go:46: //
http://sepp.oetiker.ch/sasl-2.1.19-ds/draft-murchison-sasl-login-00.txt.
This says that LOGIN is deprecated in favor of PLAIN.
Is this worth having at all?

I'd like it to require TLS and also require a specific server name.
We don't want to encourage people to be sending passwords
over unencrypted, unauthenticated connections.

If LoginAuth stays, it should do as the draft suggests and
refuse to start if there are any other plaintext authentication
mechanisms available (presumably PLAIN).

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/auth.go#newcode69
src/pkg/smtp/auth.go:69: // NewAuthPlain returns an Auth that implements the
PLAIN authentication
PlainAuth ?

TLS comment from LOGIN applies here too.

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/smtp.go
File src/pkg/smtp/smtp.go (right):

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/smtp.go#newcode58
src/pkg/smtp/smtp.go:58: if strings.Index(msg, "ESMTP") >= 0 {
Should also be saving the host name from msg.

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/smtp.go#newcode83
src/pkg/smtp/smtp.go:83: sp := strings.IndexRune(msg, ' ')
Seems wrong.
Here's an example session where this fails:

$ telnet am.lcs.mit.edu 25
Trying 18.26.4.9...
Connected to amsterdam.lcs.mit.edu.
Escape character is '^]'.
220 amsterdam.lcs.mit.edu ESMTP
EHLO rsc
250-amsterdam.lcs.mit.edu
250-STARTTLS
250 PIPELINING

I think you want to save the host name from the initial 220,
not from the EHLO/HELO response.

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/smtp.go#newcode132
src/pkg/smtp/smtp.go:132: c.conn = tls.Client(c.conn, nil)
after this
call c.conn.VerifyHostname with the server name
and check for error

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/smtp.go#newcode153
src/pkg/smtp/smtp.go:153: return err
close the connection?

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/smtp.go#newcode172
src/pkg/smtp/smtp.go:172: // abort the AUTH
+ close the connection, no?

[eds, 2010-09-27 17:24:07]

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/auth.go
File src/pkg/smtp/auth.go (right):

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/auth.go#newcode46
src/pkg/smtp/auth.go:46: //
http://sepp.oetiker.ch/sasl-2.1.19-ds/draft-murchison-sasl-login-00.txt.
On 2010/09/27 15:43:23, rsc1 wrote:
> This says that LOGIN is deprecated in favor of PLAIN.
> Is this worth having at all?

Maybe not. I haven't seen a server that supports LOGIN but not PLAIN. It's
possible, though, and LOGIN is still widely supported by servers.

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/smtp.go
File src/pkg/smtp/smtp.go (right):

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/smtp.go#newcode83
src/pkg/smtp/smtp.go:83: sp := strings.IndexRune(msg, ' ')
On 2010/09/27 15:43:23, rsc1 wrote:
> I think you want to save the host name from the initial 220,
> not from the EHLO/HELO response.

In practice, it seems like servers supply the host name in the initial 220, but
I don't believe the RFC requires it. It's required for EHLO and HELO responses.

We can assume the host name will be there on the initial 220, but it will be
another place where we don't support servers which are technically well-behaved.

[rsc, 2010-09-27 17:31:43]

> In practice, it seems like servers supply the host name in the initial
> 220, but I don't believe the RFC requires it. It's required for EHLO and
> HELO responses.

Okay, but isn't the existing code still wrong for the
transcript I provided?

Russ

[eds, 2010-09-27 17:32:54]

On Mon, Sep 27, 2010 at 12:31 PM, Russ Cox <rsc@golang.org> wrote:
> Okay, but isn't the existing code still wrong for the
> transcript I provided?

Yes, and I'll fix it.

- Evan

[eds, 2010-09-28 03:15:41]

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/smtp.go
File src/pkg/smtp/smtp.go (right):

http://codereview.appspot.com/2052042/diff/44001/src/pkg/smtp/smtp.go#newcode132
src/pkg/smtp/smtp.go:132: c.conn = tls.Client(c.conn, nil)
On 2010/09/27 15:43:23, rsc1 wrote:
> after this
> call c.conn.VerifyHostname with the server name
> and check for error

I'm having some trouble with this. Servers don't seem to send greetings with the
same host names that they have certificates for.

For example, Gmail responds to EHLO:

250 mx.google.com at your service

The certificate is for smtp.gmail.com, so VerifyHostname fails. Similar things
happen with other servers. I guess we have to keep a copy of the name we used to
connect.

I might replace NewClient with SecureDial, which will take an address instead of
a net.Conn, to allow connecting with TLS right away. (That's the main use of
NewClient right now.) Either that, or add an address argument to NewClient
that's only used for TLS verification.

What do you think?

[rsc, 2010-09-28 16:07:26]

>> call c.conn.VerifyHostname with the server name
>> and check for error
>
> I'm having some trouble with this. Servers don't seem to send greetings
> with the same host names that they have certificates for.
>
> For example, Gmail responds to EHLO:
>
> 250 mx.google.com at your service
>
> The certificate is for smtp.gmail.com, so VerifyHostname fails. Similar
> things happen with other servers. I guess we have to keep a copy of the
> name we used to connect.
>
> I might replace NewClient with SecureDial, which will take an address
> instead of a net.Conn, to allow connecting with TLS right away. (That's
> the main use of NewClient right now.) Either that, or add an address
> argument to NewClient that's only used for TLS verification.
>
> What do you think?

Sigh.  Maybe I'm just paranoid but I don't see the point
of TLS unless you verify the guy on the other end.
I don't know why they don't make this easier.

I'd add DialTLS as a convenience wrapper
(the word "Secure" is loaded and vague, so be more precise)
and add the expected host name as an argument to the
Starttls method, not to NewClient.

Russ

[agl, 2010-09-28 16:22:57]

On Tue, Sep 28, 2010 at 12:07 PM, Russ Cox <rsc@golang.org> wrote:
> Sigh.  Maybe I'm just paranoid but I don't see the point
> of TLS unless you verify the guy on the other end.
> I don't know why they don't make this easier.

Sorry, I haven't been paying attention.

SMTP/TLS is, indeed, opportunistic. It prevents eavesdroppers from
reading email, but not active attackers.

(If you think about it, it sort of has to be because the MX lookup
isn't secured (yet))


AGL

[eds, 2010-09-28 17:50:48]

On Tue, Sep 28, 2010 at 11:07 AM, Russ Cox <rsc@golang.org> wrote:
> I'd add DialTLS as a convenience wrapper
> (the word "Secure" is loaded and vague, so be more precise)
> and add the expected host name as an argument to the
> Starttls method, not to NewClient.

Makes sense, but it seems like the Client still has to keep track of
the server name to send through to the Auth mechanism when Start is
called.

- Evan

[rsc, 2010-09-28 18:13:07]

On Tue, Sep 28, 2010 at 13:50, Evan Shaw <chickencha@gmail.com> wrote:
> On Tue, Sep 28, 2010 at 11:07 AM, Russ Cox <rsc@golang.org> wrote:
>> I'd add DialTLS as a convenience wrapper
>> (the word "Secure" is loaded and vague, so be more precise)
>> and add the expected host name as an argument to the
>> Starttls method, not to NewClient.
>
> Makes sense, but it seems like the Client still has to keep track of
> the server name to send through to the Auth mechanism when Start is
> called.

Oh, we want to pass the host for non-TLS connections too.
Sure, add it to NewClient, with a plain Dial wrapper
(not DialTLS) and then drop the comment about the host
name being verified and also the call to VerifyHostname,
as per agl.

Russ

[eds, 2010-09-29 00:17:17]

Hello rsc, agl, iant (cc: golang-dev@googlegroups.com),

Please take another look.

[eds, 2010-10-06 21:43:35]

On Tue, Sep 28, 2010 at 7:17 PM,  <chickencha@gmail.com> wrote:
> Hello rsc, agl, iant (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/2052042/
>

Ping

[rsc1, 2010-10-07 07:45:19]

looks good; just a few tiny things below

http://codereview.appspot.com/2052042/diff/69001/src/pkg/smtp/auth.go
File src/pkg/smtp/auth.go (right):

http://codereview.appspot.com/2052042/diff/69001/src/pkg/smtp/auth.go#newcode45
src/pkg/smtp/auth.go:45: // mechanism as defined in RFC 4616.
Add something about what the arguments are.

// The returned Auth uses the given username and password
// to authenticate on TLS connections to host.

(What is identity???)

http://codereview.appspot.com/2052042/diff/69001/src/pkg/smtp/auth.go#newcode57
src/pkg/smtp/auth.go:57: resp := bytes.Join([][]byte{a.identity, a.username,
a.password}, []byte("\x00"))
The irony here is that using strings would have been fewer mallocs.
If you make the struct fields strings again, then:

resp := []byte(a.identity + "\x00" + a.username + "\x00" + a.password)

http://codereview.appspot.com/2052042/diff/69001/src/pkg/smtp/smtp.go
File src/pkg/smtp/smtp.go (right):

http://codereview.appspot.com/2052042/diff/69001/src/pkg/smtp/smtp.go#newcode51
src/pkg/smtp/smtp.go:51: // server name to be verified if the connection begins
using TLS.
update comment: not verified anymore

[eds, 2010-10-07 17:35:55]

Hello rsc, iant, agl (cc: golang-dev@googlegroups.com),

Please take another look.

[eds, 2010-10-13 23:26:40]

On Thu, Oct 7, 2010 at 12:35 PM,  <chickencha@gmail.com> wrote:
> Hello rsc, iant, agl (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/2052042/
>

Ping

- Evan

[rsc, 2010-10-13 23:56:29]

LGTM

Sorry for the delay.  I was just thinking yesterday
that I wanted to use this package.  :-)

[rsc, 2010-10-14 00:20:35]

please hg sync & hg mail one more time and i will submit.

[eds, 2010-10-14 00:45:45]

Hello rsc, iant, agl (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2010-10-14 02:07:34]

*** Submitted as http://code.google.com/p/go/source/detail?r=f84da45df00e ***

smtp: new package

R=rsc, iant, agl
CC=golang-dev
http://codereview.appspot.com/2052042

Committer: Russ Cox <rsc@golang.org>