From 971541dc0a867f1a6cc52b927bd0bf64ec44abfc Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Sat, 13 Feb 2016 23:24:19 -0700 Subject: [PATCH] Use http client with timeout of 10s This will prevent indefinitely-hanging requests in case some service or middle box is malfunctioning. Fix vet errors and lint warnings Add vet to CI check Only get issuer certificate if it would be used No need to make a GET request if the OCSP server is not specified in leaf certificate Fix CI tests Make tests verbose --- .travis.yml | 14 +++++++++++--- acme/challenges.go | 1 + acme/crypto.go | 23 +++++++++++------------ acme/dns_challenge.go | 6 +++--- acme/dns_challenge_cloudflare.go | 2 +- acme/dns_challenge_dnsimple.go | 10 +++++----- acme/dns_challenge_route53.go | 11 ++++++----- acme/dns_challenge_route53_test.go | 8 ++++---- acme/dns_challenge_test.go | 4 ++-- acme/http.go | 10 +++++++--- configuration.go | 4 +++- 11 files changed, 54 insertions(+), 39 deletions(-) diff --git a/.travis.yml b/.travis.yml index 6150083b..ba0113bf 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,14 @@ language: go go: - - 1.4.3 - - 1.5.3 - - tip \ No newline at end of file + - 1.4.3 + - 1.5.3 + - tip + +install: + - go get -t ./... + - go get golang.org/x/tools/cmd/vet + +script: + - go vet ./... + - go test -v ./... diff --git a/acme/challenges.go b/acme/challenges.go index 3f679e00..85790050 100644 --- a/acme/challenges.go +++ b/acme/challenges.go @@ -1,5 +1,6 @@ package acme +// Challenge is a string that identifies a particular type and version of ACME challenge. type Challenge string const ( diff --git a/acme/crypto.go b/acme/crypto.go index 347c9bc1..5ce5ea5f 100644 --- a/acme/crypto.go +++ b/acme/crypto.go @@ -56,14 +56,22 @@ func GetOCSPForCert(bundle []byte) ([]byte, *ocsp.Response, error) { return nil, nil, err } - // We only got one certificate, means we have no issuer certificate - get it. + // We expect the certificate slice to be ordered downwards the chain. + // SRV CRT -> CA. We need to pull the leaf and issuer certs out of it, + // which should always be the first two certificates. If there's no + // OCSP server listed in the leaf cert, there's nothing to do. And if + // we have only one certificate so far, we need to get the issuer cert. + issuedCert := certificates[0] + if len(issuedCert.OCSPServer) == 0 { + return nil, nil, errors.New("no OCSP server specified in cert") + } if len(certificates) == 1 { // TODO: build fallback. If this fails, check the remaining array entries. - if len(certificates[0].IssuingCertificateURL) == 0 { + if len(issuedCert.IssuingCertificateURL) == 0 { return nil, nil, errors.New("no issuing certificate URL") } - resp, err := httpGet(certificates[0].IssuingCertificateURL[0]) + resp, err := httpGet(issuedCert.IssuingCertificateURL[0]) if err != nil { return nil, nil, err } @@ -83,17 +91,8 @@ func GetOCSPForCert(bundle []byte) ([]byte, *ocsp.Response, error) { // We want it ordered right SRV CRT -> CA certificates = append(certificates, issuerCert) } - - // We expect the certificate slice to be ordered downwards the chain. - // SRV CRT -> CA. We need to pull the cert and issuer cert out of it, - // which should always be the last two certificates. - issuedCert := certificates[0] issuerCert := certificates[1] - if len(issuedCert.OCSPServer) == 0 { - return nil, nil, errors.New("no OCSP server specified in cert") - } - // Finally kick off the OCSP request. ocspReq, err := ocsp.CreateRequest(issuedCert, issuerCert, nil) if err != nil { diff --git a/acme/dns_challenge.go b/acme/dns_challenge.go index ebbf1d9d..b45a964f 100644 --- a/acme/dns_challenge.go +++ b/acme/dns_challenge.go @@ -15,7 +15,7 @@ import ( type preCheckDNSFunc func(fqdn, value string) (bool, error) -var preCheckDNS preCheckDNSFunc = checkDnsPropagation +var preCheckDNS preCheckDNSFunc = checkDNSPropagation var recursiveNameserver = "google-public-dns-a.google.com" @@ -75,8 +75,8 @@ func (s *dnsChallenge) Solve(chlng challenge, domain string) error { return s.validate(s.jws, domain, chlng.URI, challenge{Resource: "challenge", Type: chlng.Type, Token: chlng.Token, KeyAuthorization: keyAuth}) } -// checkDnsPropagation checks if the expected TXT record has been propagated to all authoritative nameservers. -func checkDnsPropagation(fqdn, value string) (bool, error) { +// checkDNSPropagation checks if the expected TXT record has been propagated to all authoritative nameservers. +func checkDNSPropagation(fqdn, value string) (bool, error) { // Initial attempt to resolve at the recursive NS r, err := dnsQuery(fqdn, dns.TypeTXT, recursiveNameserver, true) if err != nil { diff --git a/acme/dns_challenge_cloudflare.go b/acme/dns_challenge_cloudflare.go index 4781ec5b..45a9e3a6 100644 --- a/acme/dns_challenge_cloudflare.go +++ b/acme/dns_challenge_cloudflare.go @@ -27,7 +27,7 @@ func NewDNSProviderCloudFlare(cloudflareEmail, cloudflareKey string) (*DNSProvid } c := &DNSProviderCloudFlare{ - client: cloudflare.New(&cloudflare.Options{cloudflareEmail, cloudflareKey}), + client: cloudflare.New(&cloudflare.Options{Email: cloudflareEmail, Key: cloudflareKey}), ctx: context.Background(), } diff --git a/acme/dns_challenge_dnsimple.go b/acme/dns_challenge_dnsimple.go index f22590c7..56b96f97 100644 --- a/acme/dns_challenge_dnsimple.go +++ b/acme/dns_challenge_dnsimple.go @@ -16,16 +16,16 @@ type DNSProviderDNSimple struct { // NewDNSProviderDNSimple returns a DNSProviderDNSimple instance with a configured dnsimple client. // Authentication is either done using the passed credentials or - when empty - using the environment // variables DNSIMPLE_EMAIL and DNSIMPLE_API_KEY. -func NewDNSProviderDNSimple(dnsimpleEmail, dnsimpleApiKey string) (*DNSProviderDNSimple, error) { - if dnsimpleEmail == "" || dnsimpleApiKey == "" { - dnsimpleEmail, dnsimpleApiKey = dnsimpleEnvAuth() - if dnsimpleEmail == "" || dnsimpleApiKey == "" { +func NewDNSProviderDNSimple(dnsimpleEmail, dnsimpleAPIKey string) (*DNSProviderDNSimple, error) { + if dnsimpleEmail == "" || dnsimpleAPIKey == "" { + dnsimpleEmail, dnsimpleAPIKey = dnsimpleEnvAuth() + if dnsimpleEmail == "" || dnsimpleAPIKey == "" { return nil, fmt.Errorf("DNSimple credentials missing") } } c := &DNSProviderDNSimple{ - client: dnsimple.NewClient(dnsimpleApiKey, dnsimpleEmail), + client: dnsimple.NewClient(dnsimpleAPIKey, dnsimpleEmail), } return c, nil diff --git a/acme/dns_challenge_route53.go b/acme/dns_challenge_route53.go index c4ce6eb2..68a05b0c 100644 --- a/acme/dns_challenge_route53.go +++ b/acme/dns_challenge_route53.go @@ -32,12 +32,13 @@ func NewDNSProviderRoute53(awsAccessKey, awsSecretKey, awsRegionName string) (*D // - uses AWS_ACCESS_KEY_ID + AWS_SECRET_ACCESS_KEY and optionally AWS_SECURITY_TOKEN, if provided // - uses EC2 instance metadata credentials (http://169.254.169.254/latest/meta-data/…), if available // ...and otherwise returns an error - if auth, err := aws.GetAuth(awsAccessKey, awsSecretKey); err != nil { + auth, err := aws.GetAuth(awsAccessKey, awsSecretKey) + if err != nil { return nil, err - } else { - client := route53.New(auth, region) - return &DNSProviderRoute53{client: client}, nil } + + client := route53.New(auth, region) + return &DNSProviderRoute53{client: client}, nil } // Present creates a TXT record using the specified parameters @@ -60,7 +61,7 @@ func (r *DNSProviderRoute53) changeRecord(action, fqdn, value string, ttl int) e return err } recordSet := newTXTRecordSet(fqdn, value, ttl) - update := route53.Change{action, recordSet} + update := route53.Change{Action: action, Record: recordSet} changes := []route53.Change{update} req := route53.ChangeResourceRecordSetsRequest{Comment: "Created by Lego", Changes: changes} resp, err := r.client.ChangeResourceRecordSets(hostedZoneID, &req) diff --git a/acme/dns_challenge_route53_test.go b/acme/dns_challenge_route53_test.go index e57fdaa7..2c58b263 100644 --- a/acme/dns_challenge_route53_test.go +++ b/acme/dns_challenge_route53_test.go @@ -65,9 +65,9 @@ var GetChangeAnswer = ` ` var serverResponseMap = testutil.ResponseMap{ - "/2013-04-01/hostedzone/": testutil.Response{200, nil, ListHostedZonesAnswer}, - "/2013-04-01/hostedzone/Z2K123214213123/rrset": testutil.Response{200, nil, ChangeResourceRecordSetsAnswer}, - "/2013-04-01/change/asdf": testutil.Response{200, nil, GetChangeAnswer}, + "/2013-04-01/hostedzone/": testutil.Response{Status: 200, Headers: nil, Body: ListHostedZonesAnswer}, + "/2013-04-01/hostedzone/Z2K123214213123/rrset": testutil.Response{Status: 200, Headers: nil, Body: ChangeResourceRecordSetsAnswer}, + "/2013-04-01/change/asdf": testutil.Response{Status: 200, Headers: nil, Body: GetChangeAnswer}, } func init() { @@ -92,7 +92,7 @@ func makeRoute53TestServer() *testutil.HTTPServer { } func makeRoute53Provider(server *testutil.HTTPServer) *DNSProviderRoute53 { - auth := aws.Auth{"abc", "123", ""} + auth := aws.Auth{AccessKey: "abc", SecretKey: "123", Token: ""} client := route53.NewWithClient(auth, aws.Region{Route53Endpoint: server.URL}, testutil.DefaultClient) return &DNSProviderRoute53{client: client} } diff --git a/acme/dns_challenge_test.go b/acme/dns_challenge_test.go index 031fa555..20090edc 100644 --- a/acme/dns_challenge_test.go +++ b/acme/dns_challenge_test.go @@ -141,7 +141,7 @@ func TestCheckAuthoritativeNss(t *testing.T) { for _, tt := range checkAuthoritativeNssTests { ok, _ := checkAuthoritativeNss(tt.fqdn, tt.value, tt.ns) if ok != tt.ok { - t.Errorf("#%s: got %t; want %t", tt.fqdn, tt.ok) + t.Errorf("%s: got %t; want %t", tt.fqdn, tt.ok, tt.ok) } } } @@ -174,7 +174,7 @@ func TestWaitForTimeout(t *testing.T) { t.Fatal("timeout exceeded") case err := <-c: if err == nil { - t.Errorf("expected timeout error; got ", err) + t.Errorf("expected timeout error; got %v", err) } } } diff --git a/acme/http.go b/acme/http.go index 6933899b..410aead6 100644 --- a/acme/http.go +++ b/acme/http.go @@ -8,11 +8,15 @@ import ( "net/http" "runtime" "strings" + "time" ) // UserAgent (if non-empty) will be tacked onto the User-Agent string in requests. var UserAgent string +// defaultClient is an HTTP client with a reasonable timeout value. +var defaultClient = http.Client{Timeout: 10 * time.Second} + const ( // defaultGoUserAgent is the Go HTTP package user agent string. Too // bad it isn't exported. If it changes, we should update it here, too. @@ -32,7 +36,7 @@ func httpHead(url string) (resp *http.Response, err error) { req.Header.Set("User-Agent", userAgent()) - resp, err = http.DefaultClient.Do(req) + resp, err = defaultClient.Do(req) if err != nil { return resp, err } @@ -50,7 +54,7 @@ func httpPost(url string, bodyType string, body io.Reader) (resp *http.Response, req.Header.Set("Content-Type", bodyType) req.Header.Set("User-Agent", userAgent()) - return http.DefaultClient.Do(req) + return defaultClient.Do(req) } // httpGet performs a GET request with a proper User-Agent string. @@ -62,7 +66,7 @@ func httpGet(url string) (resp *http.Response, err error) { } req.Header.Set("User-Agent", userAgent()) - return http.DefaultClient.Do(req) + return defaultClient.Do(req) } // getJSON performs an HTTP GET request and parses the response body diff --git a/configuration.go b/configuration.go index 503510f8..cbb157f3 100644 --- a/configuration.go +++ b/configuration.go @@ -25,6 +25,7 @@ func (c *Configuration) RsaBits() int { return c.context.GlobalInt("rsa-key-size") } +// ExcludedSolvers is a list of solvers that are to be excluded. func (c *Configuration) ExcludedSolvers() (cc []acme.Challenge) { for _, s := range c.context.GlobalStringSlice("exclude") { cc = append(cc, acme.Challenge(s)) @@ -39,6 +40,7 @@ func (c *Configuration) ServerPath() string { return strings.Replace(srvStr, "/", string(os.PathSeparator), -1) } +// CertPath gets the path for certificates. func (c *Configuration) CertPath() string { return path.Join(c.context.GlobalString("path"), "certificates") } @@ -54,7 +56,7 @@ func (c *Configuration) AccountPath(acc string) string { return path.Join(c.AccountsPath(), acc) } -// AccountPath returns the OS dependent path to the keys of a particular account +// AccountKeysPath returns the OS dependent path to the keys of a particular account func (c *Configuration) AccountKeysPath(acc string) string { return path.Join(c.AccountPath(acc), "keys") }