code review 5451087: net/smtp: add CRAM-MD5 authentication

net/smtp: add CRAM-MD5 authentication

[vadik, 2011-12-03 02:57:40]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[eds2, 2011-12-03 16:57:05]

Looks pretty good.

I started writing this when I wrote the package but couldn't find a free mail
server that used this mechanism so couldn't test it. (Also couldn't be troubled
to set up my own.) What server are you testing against?

- Evan

http://codereview.appspot.com/5451087/diff/3/src/pkg/net/smtp/auth.go
File src/pkg/net/smtp/auth.go (right):

http://codereview.appspot.com/5451087/diff/3/src/pkg/net/smtp/auth.go#newcode73
src/pkg/net/smtp/auth.go:73: type cramMd5Auth struct {
Should be named cramMD5Auth

http://codereview.appspot.com/5451087/diff/3/src/pkg/net/smtp/auth.go#newcode88
src/pkg/net/smtp/auth.go:88: if server.Name != a.host {
I think the reason we added this check in plainAuth was because we were
requiring TLS. If every Auth implementation has to have this check, then we
should move it somewhere else to avoid repetition.

[vadik, 2011-12-03 18:26:40]

On 2011/12/03 16:57:05, eds2 wrote:
> Looks pretty good.

Thank you, kind Sir!

> What server are you testing against?

I happen to have my own (running Exim).

=>
http://codereview.appspot.com/5451087/diff/3/src/pkg/net/smtp/auth.go#newcode73
> src/pkg/net/smtp/auth.go:73: type cramMd5Auth struct {
> Should be named cramMD5Auth

Ok.  I'm a little confused by Go capitalisation traditions.
 
>
http://codereview.appspot.com/5451087/diff/3/src/pkg/net/smtp/auth.go#newcode88
> src/pkg/net/smtp/auth.go:88: if server.Name != a.host {
> I think the reason we added this check in plainAuth was because we were
> requiring TLS. If every Auth implementation has to have this check, then we
> should move it somewhere else to avoid repetition.

With PLAIN there's also a danger of sending a plaintext password to random
hosts.  But I think the caller should take care of matching SMTP connection with
the Auth anyway.  That is, Auth may be the place to fail the transaction, but
not for the reason of host mismatch, *especially* if the host string came from
the caller anyway.  But I'm rambling.

I'm removing host from the structure 'cuz it's deemed useless.

Vadik.

[vadik, 2011-12-03 18:31:45]

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

Please take another look.

[eds2, 2011-12-04 06:15:03]

On Sun, Dec 4, 2011 at 7:26 AM,  <unixdj@gmail.com> wrote:
> Ok.  I'm a little confused by Go capitalisation traditions.

I thought we always capitalized abbreviations and acronyms even if the
identifiers weren't exported, but I'm finding a few counterexamples in
the code base. Someone else will chime in. Sorry if I was wrong and
caused extra work.

> With PLAIN there's also a danger of sending a plaintext password to
> random hosts.  But I think the caller should take care of matching SMTP
> connection with the Auth anyway.  That is, Auth may be the place to fail
> the transaction, but not for the reason of host mismatch, *especially*
> if the host string came from the caller anyway.  But I'm rambling.
>
> I'm removing host from the structure 'cuz it's deemed useless.

That sounds fine to me.

I can't submit CLs and I'm also on vacation, so someone else will have
to finish this. I haven't reread the RFC or tried testing the code so
whoever follows up should look more closely at correctness.

- Evan

[bradfitz, 2011-12-05 16:29:43]

On Sat, Dec 3, 2011 at 10:26 AM, <unixdj@gmail.com> wrote:

>
> http://codereview.appspot.com/**5451087/diff/3/src/pkg/net/**
>
smtp/auth.go#newcode73<http://codereview.appspot.com/5451087/diff/3/src/pkg/net/smtp/auth.go#newcode73>
>
>> src/pkg/net/smtp/auth.go:73: type cramMd5Auth struct {
>> Should be named cramMD5Auth
>>
>
> Ok.  I'm a little confused by Go capitalisation traditions.


privateFooBar
PublicFooBar

But each letter of an acronym should have the same case as its first letter:

privateMD5Bar
md5FooBar

[vadik, 2011-12-09 11:34:34]

On 2011/12/05 16:29:43, bradfitz wrote:
> On Sat, Dec 3, 2011 at 10:26 AM, <mailto:unixdj@gmail.com> wrote:
> 
> >
> > http://codereview.appspot.com/**5451087/diff/3/src/pkg/net/**
> >
>
smtp/auth.go#newcode73<http://codereview.appspot.com/5451087/diff/3/src/pkg/net/smtp/auth.go#newcode73>
> >
> >> src/pkg/net/smtp/auth.go:73: type cramMd5Auth struct {
> >> Should be named cramMD5Auth
> >>
> >
> > Ok.  I'm a little confused by Go capitalisation traditions.
> 
> 
> privateFooBar
> PublicFooBar
> 
> But each letter of an acronym should have the same case as its first letter:
> 
> privateMD5Bar
> md5FooBar

[vadik, 2011-12-09 11:36:59]

On 2011/12/05 16:29:43, bradfitz wrote:
> privateFooBar
> PublicFooBar
> 
> But each letter of an acronym should have the same case as its first letter:
> 
> privateMD5Bar
> md5FooBar

CRAM is also an acronym, so this implies CRAMMD5Auth or CRAM_MD5Auth, which
doesn't seem to fit.

[rsc, 2011-12-09 14:33:46]

On Fri, Dec 9, 2011 at 06:36,  <unixdj@gmail.com> wrote:
> CRAM is also an acronym, so this implies CRAMMD5Auth or CRAM_MD5Auth,
> which doesn't seem to fit.

CRAMMD5Auth it is.
not our fault they picked a bad name.

[vadik, 2011-12-13 22:45:50]

Hello golang-dev@googlegroups.com, edsrzf@gmail.com, bradfitz@golang.org,
rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-12-14 21:54:18]

LGTM

[rsc, 2011-12-14 22:17:28]

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

net/smtp: add CRAM-MD5 authentication

R=golang-dev, edsrzf, bradfitz, rsc
CC=golang-dev
http://codereview.appspot.com/5451087

Committer: Russ Cox <rsc@golang.org>