Code review - Issue 1684051: code review 1684051: crypto/tls, http: Make HTTPS servers easier.https://codereview.appspot.com/2010-07-02T17:04:20+00:00rietveld
Message from unknown
2010-07-01T21:47:30+00:00agl1urn:md5:11a7c3fd48d6ecce5a5d371851ff1310
Message from unknown
2010-07-01T21:47:45+00:00agl1urn:md5:f8f3770a7ad9494619a444474eeb5140
Message from unknown
2010-07-01T21:56:13+00:00agl1urn:md5:85bfd2fc54cd619ea3313f596a3d7341
Message from agl@golang.org
2010-07-01T21:57:04+00:00agl1urn:md5:336ea8aa4aafcf366dd1373d351ec6eb
Message from adg@golang.org
2010-07-01T22:08:23+00:00adgurn:md5:6270c5a022fbb9eed90af94957b902cf
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.
Message from unknown
2010-07-01T22:11:20+00:00agl1urn:md5:bafb9f3159d451a82c7c1f8e6bb6800f
Message from agl@golang.org
2010-07-01T22:11:42+00:00agl1urn:md5:f1393bc9639eb97f4ba43e4f2de2cf90
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.
Message from unknown
2010-07-01T22:14:15+00:00agl1urn:md5:96a176d1308cdf3b2a709624c3f83a38
Message from rsc@google.com
2010-07-01T23:24:07+00:00rsc1urn:md5:0e5f6c9291d1ae05692ec329ca15e56c
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.
Message from rsc@google.com
2010-07-01T23:24:15+00:00rsc1urn:md5:6e74a838a952148fdd1e65a186499822
Message from r@google.com
2010-07-01T23:25:04+00:00r2urn:md5:895c70703eb8467a112304b9c93eadbf
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
Message from r@google.com
2010-07-01T23:26:08+00:00r2urn:md5:cb57df55102b8e4f0d9866fc93cc6d90
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
Message from rsc@google.com
2010-07-01T23:29:21+00:00rsc1urn:md5:84a4719439a4e10d15dcf6ed54220d31
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?
Message from r@google.com
2010-07-01T23:30:16+00:00r2urn:md5:c4350f1a181522a78f63a8bed2a32154
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
Message from rsc@google.com
2010-07-01T23:38:31+00:00rsc1urn:md5:d14a56473d9b9a05ce23adc812f056b9
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.
Message from agl@golang.org
2010-07-02T15:17:58+00:00agl1urn:md5:87ca9b2bd77488f375f59c1b9a2aa680
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
Message from r@google.com
2010-07-02T15:27:16+00:00r2urn:md5:5cf399c646417656043e782afaf9bc0d
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
Message from unknown
2010-07-02T15:43:27+00:00agl1urn:md5:4a08b380fcf6d88efa0c7661a0fc019b
Message from unknown
2010-07-02T15:44:31+00:00agl1urn:md5:facb54cbdc1966cf99b4a7c9160ee825
Message from agl@golang.org
2010-07-02T15:45:26+00:00agl1urn:md5:5f8d0e6930cc42cdd12ce606a84f5c59
On Fri, Jul 2, 2010 at 11:26 AM, Rob 'Commander' Pike <r@google.com> wrote:
> Yes
Great. In which case, PTAL.
AGL
Message from rsc@google.com
2010-07-02T16:43:23+00:00rsc1urn:md5:32f659238e6d5f4ac12745f7d88777ea
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
Message from agl@golang.org
2010-07-02T17:00:02+00:00agl1urn:md5:8532f54a774f017edf52b277b3995a4e
On Fri, Jul 2, 2010 at 12:43 PM, <rsc@google.com> wrote:
> LGTM
Thanks. Have fixed nits. Submitting.
AGL
Message from unknown
2010-07-02T17:00:11+00:00agl1urn:md5:8dc2cffe500374da7d2a363c0e2f58f6
Message from agl@golang.org
2010-07-02T17:00:17+00:00agl1urn:md5:b61816ef1a874b14a19057854e5c0147
Hello r, adg, rsc (cc: golang-dev@googlegroups.com),
I'd like you to review this change.
Message from agl@golang.org
2010-07-02T17:00:24+00:00agl1urn:md5:7c71c5e772201a8a95cc5590dacf7e92
*** 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
Message from r@golang.org
2010-07-02T17:02:11+00:00rurn:md5:080bfb9ccd08037e70a2088096ee00d6
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?
Message from agl@golang.org
2010-07-02T17:04:20+00:00agl1urn:md5:7bd71798093333c1e3dddf99e7aa8e4a
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