code review 1684051: crypto/tls, http: Make HTTPS servers easier.

crypto/tls, http: Make HTTPS servers easier.

[adg, 2010-07-01 22:08:23]

LGTM

A couple of nits below.

I'd like to write more of these examples/ directories throughout the standard
library.

Should the directory be 'example' (singular) ?

http://codereview.appspot.com/1684051/diff/5001/6001
File src/pkg/crypto/tls/examples/generate_cert.go (right):

http://codereview.appspot.com/1684051/diff/5001/6001#newcode1
src/pkg/crypto/tls/examples/generate_cert.go:1: // Generate a self-signed X.509
certificate for a TLS server.
Should mention that cert.pem and key.pem will be [over-]written.

http://codereview.appspot.com/1684051/diff/5001/6004
File src/pkg/http/https/https.go (right):

http://codereview.appspot.com/1684051/diff/5001/6004#newcode16
src/pkg/http/https/https.go:16: conn, err := net.Listen("tcp", addr)
Perhaps the Listen should happen after the config. Currently, if LoadX509KeyPair
fails then conn won't be torn down.

[agl1, 2010-07-01 22:11:42]

Happy to make the directory 'example' if people like that.

http://codereview.appspot.com/1684051/diff/5001/6001
File src/pkg/crypto/tls/examples/generate_cert.go (right):

http://codereview.appspot.com/1684051/diff/5001/6001#newcode1
src/pkg/crypto/tls/examples/generate_cert.go:1: // Generate a self-signed X.509
certificate for a TLS server.
On 2010/07/01 22:08:23, adg wrote:
> Should mention that cert.pem and key.pem will be [over-]written.
> 
> 

Done.

http://codereview.appspot.com/1684051/diff/5001/6004
File src/pkg/http/https/https.go (right):

http://codereview.appspot.com/1684051/diff/5001/6004#newcode16
src/pkg/http/https/https.go:16: conn, err := net.Listen("tcp", addr)
On 2010/07/01 22:08:23, adg wrote:
> Perhaps the Listen should happen after the config. Currently, if
LoadX509KeyPair
> fails then conn won't be torn down.

Done.

[rsc1, 2010-07-01 23:24:07]

I'm inclined to drop the examples directory
and just leave the generate_cert program
in the main tls directory.  This is like http/triv.go
being in http.

The example server should be part of the 
package doc comment in the http/https.

The doc comment for crypto/tls might direct
people to http/https for the HTTPS server.

I'm not sure whether http/https merits its 
own package.  The http client is going to 
need to do TLS too, and I had intended to
just import tls directly from http, so that
wouldn't be a new dependency.

If you wanted to make it http.ListenAndServeTLS
I think that would be fine too.  The example could
be in the doc comment for ListenAndServeTLS.

See what r thinks.

[r2, 2010-07-01 23:25:04]

I think 'examples' is an ok name but singular is fine too.  What's  
important is that these examples are compiled and maybe run as part of  
all.bash and that they are mentioned in the package documentation and  
indexed by godoc.

-rob

[r2, 2010-07-01 23:26:08]

On Jul 1, 2010, at 4:24 PM, rsc@google.com wrote:

> I'm inclined to drop the examples directory
> and just leave the generate_cert program
> in the main tls directory.  This is like http/triv.go
> being in http.
>
> The example server should be part of the
> package doc comment in the http/https.

that works too, but the longer one is nice in isolation.  it could go  
in the top directory but i don't mind the precedent. maybe triv.go  
should go into an example directory too.

but see comments in other mail.

>
> The doc comment for crypto/tls might direct
> people to http/https for the HTTPS server.
>
> I'm not sure whether http/https merits its
> own package.  The http client is going to
> need to do TLS too, and I had intended to
> just import tls directly from http, so that
> wouldn't be a new dependency.
>
> If you wanted to make it http.ListenAndServeTLS
> I think that would be fine too.  The example could
> be in the doc comment for ListenAndServeTLS.
>
> See what r thinks.
>
>
>
> http://codereview.appspot.com/1684051/show

[rsc1, 2010-07-01 23:29:21]

I'd like to discuss the example thing separately.
Can we leave the example in tls for now and
worry about both triv (which no longer compiles)
and this example in a future CL?

[r2, 2010-07-01 23:30:16]

On Jul 1, 2010, at 4:29 PM, Russ Cox wrote:

> I'd like to discuss the example thing separately.
> Can we leave the example in tls for now and
> worry about both triv (which no longer compiles)
> and this example in a future CL?

i can't parse that, but sure, whatever.

-rob

[rsc1, 2010-07-01 23:38:31]

okay, so i think:

move ListenAndServeTLS into package http, dropping http/https
move generate_certs.go up one directory
move server.go into the doc comment for ListenAndServeTLS.

[agl1, 2010-07-02 15:17:58]

On Thu, Jul 1, 2010 at 7:38 PM,  <rsc@google.com> wrote:
> move generate_certs.go up one directory
> move server.go into the doc comment for ListenAndServeTLS.

Will do.

> move ListenAndServeTLS into package http, dropping http/https

Is the linker smart enough not to pull in unneeded code in the case
that the program imports http, but doesn't use HTTPS? The reason that
I made it a different package was to avoid bloat: big, asn1, rsa,
x509, tls is a lot.


Cheers

AGL

[r2, 2010-07-02 15:27:16]

On Jul 2, 2010, at 8:17 AM, Adam Langley wrote:

> On Thu, Jul 1, 2010 at 7:38 PM,  <rsc@google.com> wrote:
>> move generate_certs.go up one directory
>> move server.go into the doc comment for ListenAndServeTLS.
> 
> Will do.
> 
>> move ListenAndServeTLS into package http, dropping http/https
> 
> Is the linker smart enough not to pull in unneeded code in the case
> that the program imports http, but doesn't use HTTPS?

Yes

> The reason that
> I made it a different package was to avoid bloat: big, asn1, rsa,
> x509, tls is a lot.
> 
> 
> Cheers
> 
> AGL

[agl1, 2010-07-02 15:45:26]

On Fri, Jul 2, 2010 at 11:26 AM, Rob 'Commander' Pike <r@google.com> wrote:
> Yes

Great. In which case, PTAL.


AGL

[rsc1, 2010-07-02 16:43:23]

LGTM

thanks for adding this

http://codereview.appspot.com/1684051/diff/23001/24001
File src/pkg/crypto/tls/generate_cert.go (right):

http://codereview.appspot.com/1684051/diff/23001/24001#newcode1
src/pkg/crypto/tls/generate_cert.go:1: // Generate a self-signed X.509
certificate for a TLS server. Outputs to
copyright notice

http://codereview.appspot.com/1684051/diff/23001/24003
File src/pkg/http/server.go (right):

http://codereview.appspot.com/1684051/diff/23001/24003#newcode645
src/pkg/http/server.go:645: // ListenAndServeTLS acts identically to
ListenAndServe, expect that it
s/expect/except/

http://codereview.appspot.com/1684051/diff/23001/24003#newcode653
src/pkg/http/server.go:653: //		"http/https"
d

http://codereview.appspot.com/1684051/diff/23001/24003#newcode665
src/pkg/http/server.go:665: //		err := https.ListenAndServe(":10443",
"cert.pem", "key.pem", nil)
s/https/http/

http://codereview.appspot.com/1684051/diff/23001/24003#newcode667
src/pkg/http/server.go:667: //			log.Crash(err)
log.Exit is probably better; the stack trace is not useful

[agl1, 2010-07-02 17:00:02]

On Fri, Jul 2, 2010 at 12:43 PM,  <rsc@google.com> wrote:
> LGTM

Thanks. Have fixed nits. Submitting.


AGL

[agl1, 2010-07-02 17:00:17]

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

I'd like you to review this change.

[agl1, 2010-07-02 17:00:24]

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

crypto/tls, http: Make HTTPS servers easier.

R=r, adg, rsc
CC=golang-dev
http://codereview.appspot.com/1684051

[r, 2010-07-02 17:02:11]

http://codereview.appspot.com/1684051/diff/23001/24001
File src/pkg/crypto/tls/generate_cert.go (right):

http://codereview.appspot.com/1684051/diff/23001/24001#newcode20
src/pkg/crypto/tls/generate_cert.go:20: }
you might want to use the flags package and say generate_cert -host foo
that gives you nice errors and a representative interface.  avoid grubbing
around in the args vector too

http://codereview.appspot.com/1684051/diff/23001/24001#newcode30
src/pkg/crypto/tls/generate_cert.go:30: log.Stdoutf("Generating RSA key\n")
d

http://codereview.appspot.com/1684051/diff/23001/24001#newcode33
src/pkg/crypto/tls/generate_cert.go:33: log.Crashf("failed to generate private
key: %s\n", err)
do you want Crashf or Exitf?
in any case you don't need the \n
(throughout)

http://codereview.appspot.com/1684051/diff/23001/24001#newcode46
src/pkg/crypto/tls/generate_cert.go:46: NotAfter:  time.SecondsToUTC(now +
86400*365), // valid for 1 year.
how about 24*60*365

http://codereview.appspot.com/1684051/diff/23001/24002
File src/pkg/crypto/tls/tls.go (right):

http://codereview.appspot.com/1684051/diff/23001/24002#newcode82
src/pkg/crypto/tls/tls.go:82: err = os.ErrorString("failed to parse certificate
PEM data")
these errors will tend to appear in web pages and other such places. perhaps
it's worth prefixing all of them with the package name or other indicia

http://codereview.appspot.com/1684051/diff/23001/24003
File src/pkg/http/server.go (right):

http://codereview.appspot.com/1684051/diff/23001/24003#newcode12
src/pkg/http/server.go:12: package http
can we have all.bash build this?

[agl1, 2010-07-02 17:04:20]

On Fri, Jul 2, 2010 at 1:02 PM,  <r@golang.org> wrote:
> http://codereview.appspot.com/1684051/diff/23001/24001
> File src/pkg/crypto/tls/generate_cert.go (right):

Even if I wait a while you still manage to comment two minutes after I submit :)

Will fix.


AGL