code review 70380043: net/smtp: set ServerName in StartTLS, as now required b...

net/smtp: set ServerName in StartTLS, as now required by crypto/tls

the crypto/tls revision d3d43f270632 (CL 67010043, requiring ServerName or InsecureSkipVerify) breaks net/smtp,
since it seems impossible to do SMTP via TLS anymore. i've tried to fix this by simply using a tls.Config with
ServerName, instead of a nil *tls.Config. without this fix, doing SMTP with TLS results in error "tls: either
ServerName or InsecureSkipVerify must be specified in the tls.Config".

testing: the new method TestTlsClient(...) sets up a skeletal smtp server with tls capability, and test client
injects a "fake" certificate allowing tls to work on localhost; thus, the modification to SendMail(...) enabling
this.

Fixes Issue 7437.

[mra, 2014-03-01 12:13:36]

Hello golang-codereviews@googlegroups.com (cc: bradfitz@golang.org,
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[josharian, 2014-03-01 15:16:37]

Thanks! Want to also add a test?

[mra, 2014-03-01 15:31:53]

josh, i would love to, thanks, and i'll try to make similar to current
net/smtp test(s). i'll get back to you asap.

best,
mike



On Sat, Mar 1, 2014 at 10:16 AM, <josharian@gmail.com> wrote:

> Thanks! Want to also add a test?
>
> https://codereview.appspot.com/70380043/
>

[bradfitz, 2014-03-01 17:36:29]

I've filed a bug for this so we don't forget to fix it before Go 1.3:
https://code.google.com/p/go/issues/detail?id=7437

Please add "Fixes Issue 7437" as the last sentence of your change
description too.



On Sat, Mar 1, 2014 at 7:31 AM, Mike Andrews <mra@xoba.com> wrote:

> josh, i would love to, thanks, and i'll try to make similar to current
> net/smtp test(s). i'll get back to you asap.
>
> best,
> mike
>
>
>
> On Sat, Mar 1, 2014 at 10:16 AM, <josharian@gmail.com> wrote:
>
>> Thanks! Want to also add a test?
>>
>> https://codereview.appspot.com/70380043/
>>
>
>

[mra, 2014-03-01 22:58:02]

Hello golang-codereviews@googlegroups.com, josharian@gmail.com (cc:
bradfitz@golang.org, golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-03-04 01:06:08]

https://codereview.appspot.com/70380043/diff/80001/src/pkg/net/smtp/smtp.go
File src/pkg/net/smtp/smtp.go (right):

https://codereview.appspot.com/70380043/diff/80001/src/pkg/net/smtp/smtp.go#n...
src/pkg/net/smtp/smtp.go:275: func sendMail(addr string, a Auth, from string, to
[]string, msg []byte, testing func(*tls.Config)) error {
testing is a weird name for a test hook, since it's the name of a package. 
normally they're just package-globals like:

var testHookStartTLS func(*tls.Config) // nil, except for tests

but considering you already have an end-to-end test, I'm not sure the value of
this hook anyway.  if your test passes, that seems sufficient.

https://codereview.appspot.com/70380043/diff/80001/src/pkg/net/smtp/smtp_test.go
File src/pkg/net/smtp/smtp_test.go (right):

https://codereview.appspot.com/70380043/diff/80001/src/pkg/net/smtp/smtp_test...
src/pkg/net/smtp/smtp_test.go:555: func TestTlsClient(t *testing.T) {
TestTLSClient per Go naming convention

https://codereview.appspot.com/70380043/diff/80001/src/pkg/net/smtp/smtp_test...
src/pkg/net/smtp/smtp_test.go:579: func makeCrlf(c net.Conn, suffix string)
func(s string, a ...interface{}) {
makeCRLF per Go naming convention (abbreviations don't have mixed case... all
lower or all upper).

but that's not what this function it does. It does not make a CRLF.  It does
some printing to multiple places (why to stdout?)

https://codereview.appspot.com/70380043/diff/80001/src/pkg/net/smtp/smtp_test...
src/pkg/net/smtp/smtp_test.go:582: fmt.Printf("S%s: %s\n", suffix, x)
No stdout printing in tests. Is this just debug?

[mra, 2014-03-04 01:27:04]

Hello golang-codereviews@googlegroups.com, josharian@gmail.com,
bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[mra, 2014-03-04 01:29:51]

thanks for reviewing! i agree with all your comments, and i've done hg
upload and mail just a moment ago with my changes, but i'll get more
specific in answering below, inline:

On Mon, Mar 3, 2014 at 8:06 PM, <bradfitz@golang.org> wrote:

>
> https://codereview.appspot.com/70380043/diff/80001/src/
> pkg/net/smtp/smtp.go
> File src/pkg/net/smtp/smtp.go (right):
>
> https://codereview.appspot.com/70380043/diff/80001/src/
> pkg/net/smtp/smtp.go#newcode275
> src/pkg/net/smtp/smtp.go:275: func sendMail(addr string, a Auth, from
> string, to []string, msg []byte, testing func(*tls.Config)) error {
> testing is a weird name for a test hook, since it's the name of a
> package.  normally they're just package-globals like:
>
> var testHookStartTLS func(*tls.Config) // nil, except for tests
>
> but considering you already have an end-to-end test, I'm not sure the
> value of this hook anyway.  if your test passes, that seems sufficient.
>
> yup, i like your  testHookStartTLS idea alot, it's incorporated now. the
reason we need a "hook" is to install our "fake" server certificate, in
order for it to get accepted by our client.

https://codereview.appspot.com/70380043/diff/80001/src/
> pkg/net/smtp/smtp_test.go
> File src/pkg/net/smtp/smtp_test.go (right):
>
> https://codereview.appspot.com/70380043/diff/80001/src/
> pkg/net/smtp/smtp_test.go#newcode555
> src/pkg/net/smtp/smtp_test.go:555: func TestTlsClient(t *testing.T) {
> TestTLSClient per Go naming convention
>

yup, i made this change.


>
> https://codereview.appspot.com/70380043/diff/80001/src/
> pkg/net/smtp/smtp_test.go#newcode579
> src/pkg/net/smtp/smtp_test.go:579: func makeCrlf(c net.Conn, suffix
> string) func(s string, a ...interface{}) {
> makeCRLF per Go naming convention (abbreviations don't have mixed
> case... all lower or all upper).
>
> but that's not what this function it does. It does not make a CRLF.  It
> does some printing to multiple places (why to stdout?)
>
>
yup, and i've changed that to something alot more meaningful and apropos,
also eliminated stdout debugging


> https://codereview.appspot.com/70380043/diff/80001/src/
> pkg/net/smtp/smtp_test.go#newcode582
> src/pkg/net/smtp/smtp_test.go:582: fmt.Printf("S%s: %s\n", suffix, x)
> No stdout printing in tests. Is this just debug?
>
> yes, it was just debug, all stdout printf's  are now gone.

https://codereview.appspot.com/70380043/
>

thanks!

best regards,
mike

[josharian, 2014-03-04 03:46:34]

Hi Mike,

I haven't finished a full pass over this -- have to stop for the night -- but I
figured faster, partial feedback is preferable.

Want to file a CLA? See the bottom of
http://golang.org/doc/contribute.html#copyright.

Thanks,
Josh

https://codereview.appspot.com/70380043/diff/110001/src/pkg/net/smtp/smtp.go
File src/pkg/net/smtp/smtp.go (right):

https://codereview.appspot.com/70380043/diff/110001/src/pkg/net/smtp/smtp.go#...
src/pkg/net/smtp/smtp.go:267: var testHookStartTLS func(*tls.Config) // nil,
except for tests
It strikes me as simpler for this to be a *x509.CertPool (testRootCAs?), which
init() in the tests could set up, and in SendMail, set config.RootCAs if
non-nil. That would remove a level of indirection.

https://codereview.appspot.com/70380043/diff/110001/src/pkg/net/smtp/smtp_tes...
File src/pkg/net/smtp/smtp_test.go (right):

https://codereview.appspot.com/70380043/diff/110001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:560: errs := make(chan error)
This is more commonly called `errc` (c for channel).

https://codereview.appspot.com/70380043/diff/110001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:594: switch {
You can spare some duplication by switching on line.

switch s.Text() {
case fmt.Sprintf("EHLO %s", TLS_HOST):
	// ...
case "STARTTLS":
	// ...
}

Same thing in serverHandleTLS, below.

https://codereview.appspot.com/70380043/diff/110001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:616: defer c.Close()
It seems more natural for this defer to be closer to the creation of c, in
serverHandle.

https://codereview.appspot.com/70380043/diff/110001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:640: func client() error {
s/client/sendMail/

https://codereview.appspot.com/70380043/diff/110001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:657: TLS_EMAIL = "joe@example.com"
Mixed caps: https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Mixed_Caps

[mra, 2014-03-04 10:08:30]

Hello golang-codereviews@googlegroups.com, josharian@gmail.com,
bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[mra, 2014-03-04 10:08:52]

thanks for reviewing josh, i've made all your suggested changes and
hg-uploaded/mailed just a moment ago. i particularly liked your testRootCAs
idea, but since that required adding crypto/x509 import to net/smtp, it now
causes this error in ./all.bash:

--- FAIL: TestDependencies (0.10 seconds)
deps_test.go:424: linux/amd64/cgo=true unexpected dependency: net/smtp
imports [*crypto/x509*]
 deps_test.go:431: skipping other systems

despite "go test net/smtp" per se passing.

since tls (and thus x509 too) are sorta important for smtp, should we just
allow the new crypto/x509 dependency, or instead go back to my original
method of using a "hook" (just a function), effectively moving the
crypto/x509 dependency to testing files only? i could imagine as we
continue improving security, there might also be additional reasons later
on to import crypto/x509?

best regards,
mike



On Mon, Mar 3, 2014 at 10:46 PM, <josharian@gmail.com> wrote:

> Hi Mike,
>
> I haven't finished a full pass over this -- have to stop for the night
> -- but I figured faster, partial feedback is preferable.
>
> Want to file a CLA? See the bottom of
> http://golang.org/doc/contribute.html#copyright.
>
> Thanks,
> Josh
>
>
>
> https://codereview.appspot.com/70380043/diff/110001/src/
> pkg/net/smtp/smtp.go
> File src/pkg/net/smtp/smtp.go (right):
>
> https://codereview.appspot.com/70380043/diff/110001/src/
> pkg/net/smtp/smtp.go#newcode267
> src/pkg/net/smtp/smtp.go:267: var testHookStartTLS func(*tls.Config) //
> nil, except for tests
> It strikes me as simpler for this to be a *x509.CertPool (testRootCAs?),
> which init() in the tests could set up, and in SendMail, set
> config.RootCAs if non-nil. That would remove a level of indirection.
>
> https://codereview.appspot.com/70380043/diff/110001/src/
> pkg/net/smtp/smtp_test.go
> File src/pkg/net/smtp/smtp_test.go (right):
>
> https://codereview.appspot.com/70380043/diff/110001/src/
> pkg/net/smtp/smtp_test.go#newcode560
> src/pkg/net/smtp/smtp_test.go:560: errs := make(chan error)
> This is more commonly called `errc` (c for channel).
>
> https://codereview.appspot.com/70380043/diff/110001/src/
> pkg/net/smtp/smtp_test.go#newcode594
> src/pkg/net/smtp/smtp_test.go:594: switch {
> You can spare some duplication by switching on line.
>
> switch s.Text() {
> case fmt.Sprintf("EHLO %s", TLS_HOST):
>         // ...
> case "STARTTLS":
>         // ...
> }
>
> Same thing in serverHandleTLS, below.
>
> https://codereview.appspot.com/70380043/diff/110001/src/
> pkg/net/smtp/smtp_test.go#newcode616
> src/pkg/net/smtp/smtp_test.go:616: defer c.Close()
> It seems more natural for this defer to be closer to the creation of c,
> in serverHandle.
>
> https://codereview.appspot.com/70380043/diff/110001/src/
> pkg/net/smtp/smtp_test.go#newcode640
> src/pkg/net/smtp/smtp_test.go:640: func client() error {
> s/client/sendMail/
>
> https://codereview.appspot.com/70380043/diff/110001/src/
> pkg/net/smtp/smtp_test.go#newcode657
> src/pkg/net/smtp/smtp_test.go:657: TLS_EMAIL = "joe@example.com"
> Mixed caps:
> https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Mixed_Caps
>
> https://codereview.appspot.com/70380043/
>

[mra, 2014-03-04 10:43:47]

Hello golang-codereviews@googlegroups.com, josharian@gmail.com,
bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[mra, 2014-03-04 10:50:37]

brad, josh,

i thought about this a little bit more and just uploaded/mailed a slightly
different strategy for installing the test certificate. it basically blends
both of your approaches, but avoids adding the crypto/x509 dependency to
net/smtp. i did this because deps_test.go says that adding dependencies is
a really big "policy" deal, which we can easily avoid for this net/smtp
test.

so basically i now suggest using brad's testHookStartTLS and josh's init()
to instantiate it. one benefit of the "hook" vs explicit x509 approach, is
it's actually more general than setting RootCAs, something we can exploit
in additional future tests when we write them.

best regards,
mike



On Tue, Mar 4, 2014 at 5:43 AM, <mra@xoba.com> wrote:

> Hello golang-codereviews@googlegroups.com, josharian@gmail.com,
> bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),
>
> Please take another look.
>
>
> https://codereview.appspot.com/70380043/
>

[bradfitz, 2014-03-04 16:33:09]

Please make the CL description's first line fit on one line, ~76 chars or less,
and then a blank line, and then more words.

Maybe something like:

net/smtp: set ServerName in StartTLS, as now required by crypto/tls

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
File src/pkg/net/smtp/smtp_test.go (right):

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:556: ln, err := net.Listen("tcp",
fmt.Sprintf(":%d", tlsPort))
This causes a firewall pop-up dialog on OS X and maybe other systems.

Use this:

func newLocalListener(t *testing.T) Listener {
        ln, err := Listen("tcp", "127.0.0.1:0")
        if err != nil {
                ln, err = Listen("tcp6", "[::1]:0")
        }
        if err != nil {
                t.Fatal(err)
        }
        return ln
}

And then:

   ln := newLocalListener(t)

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:582: func (s smtpSender) send(f string, a
...interface{}) {
all this sendf instead of send. The convention that is if it's printf-like, it
ends in f. Then tools like govet check the args.

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:583: fmt.Fprintf(s.w, "%s\r\n", fmt.Sprintf(f,
a...))
you can drop the Sprintf.  just:

fmt.Fprintf(s.w, f+"\r\n", a...)

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:656: tlsPort  = 1587
you can't just pick a port for testing.  something might be using that port on
the machine where the tests run.

you'll need to connect to the port as returned by newLocalListener.

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:657: tlsCert  = `-----BEGIN CERTIFICATE-----
I would use the same key pair as the one at the bottom of
src/pkg/net/http/httptest/server.go and copy its comment, too, so people know
how to recreate this in the future.

[josharian, 2014-03-04 16:34:54]

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
File src/pkg/net/smtp/smtp_test.go (right):

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:556: ln, err := net.Listen("tcp",
fmt.Sprintf(":%d", tlsPort))
I worry that listening on a specific, hard-coded port will cause spurious
failures. Better would be to have the OS select a free port for us and pass that
in to sendMail. See e.g. http://golang.org/pkg/net/http/httptest.

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:558: t.Fatalf("can't listen on port %d: %v",
tlsPort, err)
s/can't/failed to/ here and below

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:575: ln.Close()
defer ln.Close() right after checking listener err != nil

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:605: }
Slightly simpler config setup:

keypair, err := tls.X509KeyPair([]byte(tlsCert), []byte(tlsKey))
if err != nil {
	return err
}
config := &tls.Config{Certificates: []tls.Certificate{keypair}}

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:608: return serverHandleTLS(c)
Should this switch have a default case that returns an error?

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:619: case fmt.Sprintf("EHLO %s", tlsHost):
I'd find this all a bit easier to read if tlsHost and tlsEmail were just inlined
throughout this code, rather than calling fmt.Sprintf. (As a bonus, we wouldn't
be re-evaluating fmt.Sprintf on every pass through.) This also matches the style
in the other tests.

https://codereview.appspot.com/70380043/diff/190001/src/pkg/net/smtp/smtp_tes...
src/pkg/net/smtp/smtp_test.go:640:
testRootCAs.AppendCertsFromPEM([]byte(tlsCert))
May as well move this parsing out of the func itself, and just close over
testRootCAs.

[mra, 2014-03-04 21:31:33]

Hello golang-codereviews@googlegroups.com, josharian@gmail.com,
bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[mra, 2014-03-04 21:32:31]

brad and josh, i think i addressed all your points in my latest hg upload
and mail, thanks for your patience and work in these code review rounds! i
also eliminated the dependency on fmt package and made a couple other
simplifications i hope are ok. best regards, mike.


On Tue, Mar 4, 2014 at 11:34 AM, <josharian@gmail.com> wrote:

>
> https://codereview.appspot.com/70380043/diff/190001/src/
> pkg/net/smtp/smtp_test.go
> File src/pkg/net/smtp/smtp_test.go (right):
>
> https://codereview.appspot.com/70380043/diff/190001/src/
> pkg/net/smtp/smtp_test.go#newcode556
> src/pkg/net/smtp/smtp_test.go:556: ln, err := net.Listen("tcp",
> fmt.Sprintf(":%d", tlsPort))
> I worry that listening on a specific, hard-coded port will cause
> spurious failures. Better would be to have the OS select a free port for
> us and pass that in to sendMail. See e.g.
> http://golang.org/pkg/net/http/httptest.
>
> https://codereview.appspot.com/70380043/diff/190001/src/
> pkg/net/smtp/smtp_test.go#newcode558
> src/pkg/net/smtp/smtp_test.go:558: t.Fatalf("can't listen on port %d:
> %v", tlsPort, err)
> s/can't/failed to/ here and below
>
> https://codereview.appspot.com/70380043/diff/190001/src/
> pkg/net/smtp/smtp_test.go#newcode575
> src/pkg/net/smtp/smtp_test.go:575: ln.Close()
> defer ln.Close() right after checking listener err != nil
>
> https://codereview.appspot.com/70380043/diff/190001/src/
> pkg/net/smtp/smtp_test.go#newcode605
> src/pkg/net/smtp/smtp_test.go:605: }
> Slightly simpler config setup:
>
> keypair, err := tls.X509KeyPair([]byte(tlsCert), []byte(tlsKey))
> if err != nil {
>         return err
> }
> config := &tls.Config{Certificates: []tls.Certificate{keypair}}
>
> https://codereview.appspot.com/70380043/diff/190001/src/
> pkg/net/smtp/smtp_test.go#newcode608
> src/pkg/net/smtp/smtp_test.go:608: return serverHandleTLS(c)
> Should this switch have a default case that returns an error?
>
> https://codereview.appspot.com/70380043/diff/190001/src/
> pkg/net/smtp/smtp_test.go#newcode619
> src/pkg/net/smtp/smtp_test.go:619: case fmt.Sprintf("EHLO %s", tlsHost):
> I'd find this all a bit easier to read if tlsHost and tlsEmail were just
> inlined throughout this code, rather than calling fmt.Sprintf. (As a
> bonus, we wouldn't be re-evaluating fmt.Sprintf on every pass through.)
> This also matches the style in the other tests.
>
> https://codereview.appspot.com/70380043/diff/190001/src/
> pkg/net/smtp/smtp_test.go#newcode640
> src/pkg/net/smtp/smtp_test.go:640:
> testRootCAs.AppendCertsFromPEM([]byte(tlsCert))
> May as well move this parsing out of the func itself, and just close
> over testRootCAs.
>
> https://codereview.appspot.com/70380043/
>

[bradfitz, 2014-03-04 21:36:43]

No need to not depend on fmt in a test.  It's fine either way, but now you
have a func named sendf that isn't printf-like. I'd remove the f now.

LGTM otherwise.



On Tue, Mar 4, 2014 at 1:31 PM, Mike Andrews <mra@xoba.com> wrote:

> brad and josh, i think i addressed all your points in my latest hg upload
> and mail, thanks for your patience and work in these code review rounds! i
> also eliminated the dependency on fmt package and made a couple other
> simplifications i hope are ok. best regards, mike.
>
>
> On Tue, Mar 4, 2014 at 11:34 AM, <josharian@gmail.com> wrote:
>
>>
>> https://codereview.appspot.com/70380043/diff/190001/src/
>> pkg/net/smtp/smtp_test.go
>> File src/pkg/net/smtp/smtp_test.go (right):
>>
>> https://codereview.appspot.com/70380043/diff/190001/src/
>> pkg/net/smtp/smtp_test.go#newcode556
>> src/pkg/net/smtp/smtp_test.go:556: ln, err := net.Listen("tcp",
>> fmt.Sprintf(":%d", tlsPort))
>> I worry that listening on a specific, hard-coded port will cause
>> spurious failures. Better would be to have the OS select a free port for
>> us and pass that in to sendMail. See e.g.
>> http://golang.org/pkg/net/http/httptest.
>>
>> https://codereview.appspot.com/70380043/diff/190001/src/
>> pkg/net/smtp/smtp_test.go#newcode558
>> src/pkg/net/smtp/smtp_test.go:558: t.Fatalf("can't listen on port %d:
>> %v", tlsPort, err)
>> s/can't/failed to/ here and below
>>
>> https://codereview.appspot.com/70380043/diff/190001/src/
>> pkg/net/smtp/smtp_test.go#newcode575
>> src/pkg/net/smtp/smtp_test.go:575: ln.Close()
>> defer ln.Close() right after checking listener err != nil
>>
>> https://codereview.appspot.com/70380043/diff/190001/src/
>> pkg/net/smtp/smtp_test.go#newcode605
>> src/pkg/net/smtp/smtp_test.go:605: }
>> Slightly simpler config setup:
>>
>> keypair, err := tls.X509KeyPair([]byte(tlsCert), []byte(tlsKey))
>> if err != nil {
>>         return err
>> }
>> config := &tls.Config{Certificates: []tls.Certificate{keypair}}
>>
>> https://codereview.appspot.com/70380043/diff/190001/src/
>> pkg/net/smtp/smtp_test.go#newcode608
>> src/pkg/net/smtp/smtp_test.go:608: return serverHandleTLS(c)
>> Should this switch have a default case that returns an error?
>>
>> https://codereview.appspot.com/70380043/diff/190001/src/
>> pkg/net/smtp/smtp_test.go#newcode619
>> src/pkg/net/smtp/smtp_test.go:619: case fmt.Sprintf("EHLO %s", tlsHost):
>> I'd find this all a bit easier to read if tlsHost and tlsEmail were just
>> inlined throughout this code, rather than calling fmt.Sprintf. (As a
>> bonus, we wouldn't be re-evaluating fmt.Sprintf on every pass through.)
>> This also matches the style in the other tests.
>>
>> https://codereview.appspot.com/70380043/diff/190001/src/
>> pkg/net/smtp/smtp_test.go#newcode640
>> src/pkg/net/smtp/smtp_test.go:640:
>> testRootCAs.AppendCertsFromPEM([]byte(tlsCert))
>> May as well move this parsing out of the func itself, and just close
>> over testRootCAs.
>>
>> https://codereview.appspot.com/70380043/
>>
>
>

[mra, 2014-03-04 21:41:36]

Hello golang-codereviews@googlegroups.com, josharian@gmail.com,
bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-03-04 21:43:16]

LGTM



On Tue, Mar 4, 2014 at 1:41 PM, <mra@xoba.com> wrote:

> Hello golang-codereviews@googlegroups.com, josharian@gmail.com,
> bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),
>
> Please take another look.
>
>
> https://codereview.appspot.com/70380043/
>

[bradfitz, 2014-03-04 21:43:32]

*** Submitted as https://code.google.com/p/go/source/detail?r=8f8ae5044f24 ***

net/smtp: set ServerName in StartTLS, as now required by crypto/tls

the crypto/tls revision d3d43f270632 (CL 67010043, requiring ServerName or
InsecureSkipVerify) breaks net/smtp,
since it seems impossible to do SMTP via TLS anymore. i've tried to fix this by
simply using a tls.Config with
ServerName, instead of a nil *tls.Config. without this fix, doing SMTP with TLS
results in error "tls: either
ServerName or InsecureSkipVerify must be specified in the tls.Config".

testing: the new method TestTlsClient(...) sets up a skeletal smtp server with
tls capability, and test client
injects a "fake" certificate allowing tls to work on localhost; thus, the
modification to SendMail(...) enabling
this.

Fixes Issue 7437.

LGTM=bradfitz
R=golang-codereviews, josharian, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/70380043

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[mra, 2014-03-04 21:45:19]

thanks brad, i changed sendf to send. i believe i've already electronically
signed the CLA form via http://golang.org/doc/contribute.html#copyright (
https://developers.google.com/open-source/cla/individual?csw=1), but if
folks can't find me in there i'd be glad to try again. best, mike


On Tue, Mar 4, 2014 at 4:36 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> No need to not depend on fmt in a test.  It's fine either way, but now you
> have a func named sendf that isn't printf-like. I'd remove the f now.
>
> LGTM otherwise.
>
>
>
> On Tue, Mar 4, 2014 at 1:31 PM, Mike Andrews <mra@xoba.com> wrote:
>
>> brad and josh, i think i addressed all your points in my latest hg upload
>> and mail, thanks for your patience and work in these code review rounds! i
>> also eliminated the dependency on fmt package and made a couple other
>> simplifications i hope are ok. best regards, mike.
>>
>>
>> On Tue, Mar 4, 2014 at 11:34 AM, <josharian@gmail.com> wrote:
>>
>>>
>>> https://codereview.appspot.com/70380043/diff/190001/src/
>>> pkg/net/smtp/smtp_test.go
>>> File src/pkg/net/smtp/smtp_test.go (right):
>>>
>>> https://codereview.appspot.com/70380043/diff/190001/src/
>>> pkg/net/smtp/smtp_test.go#newcode556
>>> src/pkg/net/smtp/smtp_test.go:556: ln, err := net.Listen("tcp",
>>> fmt.Sprintf(":%d", tlsPort))
>>> I worry that listening on a specific, hard-coded port will cause
>>> spurious failures. Better would be to have the OS select a free port for
>>> us and pass that in to sendMail. See e.g.
>>> http://golang.org/pkg/net/http/httptest.
>>>
>>> https://codereview.appspot.com/70380043/diff/190001/src/
>>> pkg/net/smtp/smtp_test.go#newcode558
>>> src/pkg/net/smtp/smtp_test.go:558: t.Fatalf("can't listen on port %d:
>>> %v", tlsPort, err)
>>> s/can't/failed to/ here and below
>>>
>>> https://codereview.appspot.com/70380043/diff/190001/src/
>>> pkg/net/smtp/smtp_test.go#newcode575
>>> src/pkg/net/smtp/smtp_test.go:575: ln.Close()
>>> defer ln.Close() right after checking listener err != nil
>>>
>>> https://codereview.appspot.com/70380043/diff/190001/src/
>>> pkg/net/smtp/smtp_test.go#newcode605
>>> src/pkg/net/smtp/smtp_test.go:605: }
>>> Slightly simpler config setup:
>>>
>>> keypair, err := tls.X509KeyPair([]byte(tlsCert), []byte(tlsKey))
>>> if err != nil {
>>>         return err
>>> }
>>> config := &tls.Config{Certificates: []tls.Certificate{keypair}}
>>>
>>> https://codereview.appspot.com/70380043/diff/190001/src/
>>> pkg/net/smtp/smtp_test.go#newcode608
>>> src/pkg/net/smtp/smtp_test.go:608: return serverHandleTLS(c)
>>> Should this switch have a default case that returns an error?
>>>
>>> https://codereview.appspot.com/70380043/diff/190001/src/
>>> pkg/net/smtp/smtp_test.go#newcode619
>>> src/pkg/net/smtp/smtp_test.go:619: case fmt.Sprintf("EHLO %s", tlsHost):
>>> I'd find this all a bit easier to read if tlsHost and tlsEmail were just
>>> inlined throughout this code, rather than calling fmt.Sprintf. (As a
>>> bonus, we wouldn't be re-evaluating fmt.Sprintf on every pass through.)
>>> This also matches the style in the other tests.
>>>
>>> https://codereview.appspot.com/70380043/diff/190001/src/
>>> pkg/net/smtp/smtp_test.go#newcode640
>>> src/pkg/net/smtp/smtp_test.go:640:
>>> testRootCAs.AppendCertsFromPEM([]byte(tlsCert))
>>> May as well move this parsing out of the func itself, and just close
>>> over testRootCAs.
>>>
>>> https://codereview.appspot.com/70380043/
>>>
>>
>>
>