Block when creating the renewal timer, rather than doing it in a
goroutine. This fixes an issue where startRenew and stopRenew are called
very closely together, and due to lock ordering, stopRenew may be called
before startRenew, resulting in the appearance that the renewal timer
has been stopped before it has actually been created.
This is only an issue in tests, as that is the only place stopRenew is
actually used. In particular this issue manifests in TestGetCertiifcate
sub-tests, where a httptest server reuses a port across two of the
sub-tests. In this case, the renewal calls end up creating dirty state
for the subsequent test, which can cause confusing behavior (such as
attempting to register an account twice.)
Another solution to this problem would be introducing a bool, protected
by renewalMu, which indicates if renewal has been halted, and to check
it in startRenew to check if stopRenew has already been called, which
would allow us to continue calling startRenew in a goroutine and relying
on renewalMu locking for ordering. That said I don't see a particularly
strong reason to call startRenew concurrently, so this seems like the
simplest solution for now.
Fixesgolang/go#52494
Change-Id: I95420d3fd877572a0b9e408d2f8cd353f6a4e80e
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/433016
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Don't assume the path passed into the DirCache methods is absolute, and
clean it before further operating on it. Put and Delete are not attacker
controlled, but clean them anyway.
Fixes#53082
Fixes CVE-2022-30636
Change-Id: I755f525a737da60ccba07ebce4d41cc8faebfcca
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/408694
Run-TryBot: Roland Shoemaker <roland@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
In the test server, rather than spawning a goroutine to validate
challenges, block on the validation before responding to the client.
This prevents a test race, where testing.T.Logf is called after the
test is completed.
While this has a slight behavioral difference to some production
ACME server implementations (although is behavior allowed in the spec),
the change has little material impact on what we are testing, since
previously the validation would happen so quickly that it would be
indistinguishable from the new blocking behavior (i.e. we would not be
sending multiple requests during polling previously.)
Fixesgolang/go#52170
Change-Id: I75e3b2da69ddc2302be25a99f1b1151ed0f4af9b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/405548
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
TestRenewFromCache and TestRenewFromCacheAlreadyRenewed had several
races and API misuses:
1. They called t.Fatalf from a goroutine other than the one invoking
the Test function, which is explicitly disallowed (see
https://pkg.go.dev/testing#T).
2. The test did not stop the renewal timers prior to restoring
test-hook functions, and the process of stopping the renewal timers
itself did not wait for in-flight calls to complete. That could
cause data races if one of the renewals failed and triggered a
retry with a short-enough randomized backoff.
(One such race was observed in
https://build.golang.org/log/1a19e22ad826bedeb5a939c6130f368f9979208a.)
3. The testDidRenewLoop hooks accessed the Manager.renewal field
without locking the Mutex guarding that field.
4. TestGetCertificate_failedAttempt set a testDidRemoveState hook, but
didn't wait for the timers referring to that hook to complete
before restoring it, causing races with other timers. I tried
pulling on that thread a bit, but couldn't untangle the numerous
untracked goroutines in the package. Instead, I have made a smaller
and more local change to copy the value of testDidRemoveState into
a local variable in the timer's closure.
Given the number of untracked goroutines in this package, it is likely
that races and/or deadlocks remain. Notably, so far I have been unable
to spot the actual cause of golang/go#51080.
For golang/go#51080
Change-Id: I7797f6ac34ef3c272f16ca805251dac3aa7f0009
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/384594
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
At Let's Encrypt, we've found that most autocert users do not provide an email address, which makes it hard to get in touch when things go wrong with their client. Demonstrating how to provide an email will probably encourage more people to provide one.
Change-Id: I0aa1ecea7cfbe55ae155804194fb57a8e695c658
GitHub-Last-Rev: 902b5fe2682a6be5af91e886272fd680fd67a94e
GitHub-Pull-Request: golang/crypto#186
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/323313
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Trust: Katie Hockman <katie@golang.org>
Let's Encrypt is revoking all certificates verified with TLS-ALPN-01
beofre January 26th due to a compliance issue. Detect them and force a
renewal.
Also, fix the tests which were not testing if expired certificates were
renewed anymore, as the test certificates were always invalid due to not
having SANs.
Change-Id: If9d0632b2edfe0b7fb70f6cfd7e65e46e2d047dc
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/381114
Trust: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Update acme/autocert test to not depend on whether idna.Lookup uses
transitional processing (Go 1.17 and earlier) or nontransitional
processing (Go 1.18 and later).
Change-Id: I29ca0aaca0ac75a932919a4c7cf8e9fca033b497
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/364014
Trust: Damien Neil <dneil@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Previously, the o.AuthzURLs slice was sometimes used from the call to
client.WaitOrder at the bottom of the for loop.
By that point, o may be nil if client.WaitOrder returned an error,
which would cause a nil pointer dereference panic inside the deferred
function call. If client.WaitOrder did not return an error, then the
call to deactivatePendingAuthz would use its AuthzURLs slice instead
of the one from client.AuthorizeOrder.
Fixesgolang/go#35225
Updates golang/go#21081
Change-Id: I7db055ee1149871b6e5d34a8618526899c68f827
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/203919
Reviewed-by: Alex Vaghin <ddos@google.com>
The Manager now uses RFC 8555 implementation of Let's Encrypt by default.
Existing users need not do any manual upgrades. If you vendor
acme/autocert, it is enough to just rebuild your binaries at this CL.
If there's an account key stored in Manager's cache which has been used
with an earlier Let's Encrypt implementation (aka v1 or draft-02),
it will be automatically re-registered with the new endpoint.
One notable change is the CAServer from internal/acmetest was amended
to simulate a CA implementing RFC 8555, replacing draft-02.
Support for both RFC and draft-02 seemed too complicated and not worth
the benefits: the old pre-RFC bits will be removed from both acme and
acme/autocert packages at some point.
Fixesgolang/go#21081
Change-Id: Id530758ac612b1c20f9df51c4d10f770e5f41ecf
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/199520
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
According to the [RFC 5280, section 4.2.1.6](https://tools.ietf.org/html/rfc5280#section-4.2.1.6):
> ...
> When the subjectAltName extension contains a domain name system
> label, the domain name MUST be stored in the dNSName (an IA5String).
> The name MUST be in the "preferred name syntax", as specified by
> Section 3.5 of [RFC1034] and as modified by Section 2.1 of
> [RFC1123]. Note that while uppercase and lowercase letters are
> allowed in domain names, no significance is attached to the case.
> ...
And the [RFC 1034, section 3.5](https://tools.ietf.org/html/rfc1034#section-3.5):
> ...
> Note that while upper and lower case letters are allowed in domain
> names, no significance is attached to the case. That is, two names with
> the same spelling but different case are to be treated as if identical.
> ...
We should return the same TLS certificate for both `example.com` and `EXAMPLE.COM`. But the `autocert.Manager.GetCertificate` treats the two as totally different, it signs and returns two different TLS certificates.
Moreover, now the `autocert.Manager.GetCertificate` and the `autocert.HostWhitelist` can only handle Punycode IDN. If the client sends a Unicode IDN to `autocert.Manager.GetCertificate` (cURL is doing this), the "Invalid character in DNS name" error will be triggered.
This PR corrects these problems by converting the host of the TLS certificate to be obtained to Punycode via `idna.Lookup.ToASCII`.
Change-Id: I993821b3a6ae532a53772e2db00524479ef111af
GitHub-Last-Rev: 6c12694574b6ac71a920da8a8bdb130c0b6e0272
GitHub-Pull-Request: golang/crypto#85
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/171997
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
More informative error message enables HTTPS server configuration mistakes to be corrected quickly, since log files will now include the rejected hostname. If the hostname should be accepted, it can be added to the HostWhitelist Policy.
Fixesgolang/go#28345
Change-Id: I801c82f0d3b19bc34592c9cd0bce77f1b284d19d
GitHub-Last-Rev: 5dfe731d732d693f9bdd79c8a74efd30a2f56d0b
GitHub-Pull-Request: golang/crypto#63
Reviewed-on: https://go-review.googlesource.com/c/144337
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
The timeNow package variable doesn't work well here: since the renewal
functionality spawns goroutines that invoke timeNow, once a test has
caused such goroutines to exist, another test can't fiddle with it
without the race detector triggering.
Instead, have a private member of Manager that the tests can set if they
need.
Change-Id: Iaf1a68d8efb84c9c5e2804aeb9cc6b2d3f3fef43
Reviewed-on: https://go-review.googlesource.com/128655
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Because tls.Config now requires more fields to be set
in order for tls-alpn to work, Manager provides a new
TLSConfig method for easier setup.
This CL also adds a new internal package for end-to-end tests.
The package implements a simple ACME CA server.
Fixesgolang/go#25013Fixesgolang/go#25901
Updates golang/go#17251
Change-Id: I2687ea8d5c445ddafad5ea2cdd36cd4e7d10bc86
Reviewed-on: https://go-review.googlesource.com/125495
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
A var isn't needed and a const is what the upstream (Go 1.10+) version
it's copying is.
Change-Id: I335270be3b3d09ac3c22cf6fb889a74ac39b8f1d
Reviewed-on: https://go-review.googlesource.com/116379
Reviewed-by: Filippo Valsorda <filippo@golang.org>
GetCertificate has all the information it needs to know if a client
supports ECDSA in ClientHelloInfo. Deprecate and ignore ForceRSA, and
just obtain a RSA certificate on the fly when a client that doesn't
support ECDSA connects.
This changes the cache key format to have a "+rsa" suffix for RSA
certificates. The default (ForceRSA = false) cache key is unchanged,
so most DirCache instances will still be valid. Caches created with
ForceRSA set will be silently ignored and certificates reissued.
The cache keys for HTTP tokens and the account key are changed to be
guaranteed not to overlap with valid domain names as well.
Note that ECDSA support detection is more strict in following RFC 5246
than crypto/tls, which ignores signature_algorithms.
Fixesgolang/go#22066
Change-Id: I70227747b563d6849cb693f83a950d57040b3f39
Reviewed-on: https://go-review.googlesource.com/114501
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Fixes a bug introduced in golang.org/cl/100078 where incorrect
ACME client was used, causing nil pointer dereference.
The change also improves related tests,
removing code paths diverging in testing.
Fixesgolang/go#25581
Change-Id: I8c5531fcc5814a5a64f14911c0ad86c476a76d2f
Reviewed-on: https://go-review.googlesource.com/114817
Reviewed-by: Filippo Valsorda <filippo@golang.org>
We now keep track of pending authorization requests during verify() and
defer the asynchronous revocation of the ones that failed.
This should help avoid letsencrypt's "too many currently pending
authorizations" error.
Fixesgolang/go#23426
Change-Id: Ibffb10f59733962d45e43b67fc42a2ec7c5faf51
Reviewed-on: https://go-review.googlesource.com/100078
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Leo Antunes <costela@gmail.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Currently, the renewal flow will check the cache before renewing to make
sure it is actually necessary. This change modifies this flow to update
the local state so the cached cert is actually used by the manager.
Fixesgolang/go#22960
Change-Id: I16668e8098616190938ee52858294b59bc1a5160
Reviewed-on: https://go-review.googlesource.com/89995
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The Manager now loops through known challenge types,
trying to fulfill one at a time until it succeeds or runs out
of supported challenges.
The provisioning of "http-01" challenges can be done
using the new Manager.HTTPHandler method.
It requires listening on unencrypted port 80.
Fixesgolang/go#21890
Change-Id: I55de9501f0069a9f460fedd8b5b0a09b94f9ef05
Reviewed-on: https://go-review.googlesource.com/87201
Run-TryBot: Alex Vaghin <ddos@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
If you don't use a cache you're just begging for Let's Encrypt
to ban you for a week due to duplicate certificate requests.
Don't encourage users to get banned.
Change-Id: Icf999c7f28817990bb45931d30692d4040c0ed71
Reviewed-on: https://go-review.googlesource.com/66550
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alex Vaghin <ddos@google.com>
Roland Shoemaker
Sean Liao
Dmitri Shuralyov
Bryan C. Mills
cui fliter
Russ Cox
Ben Burkert
Jacob Hoffman-Andrews
HowJmay
Filippo Valsorda
Damien Neil
Trevor Dixon
Jason Baker
Alex Vaghin
yuanhh
Aofei Sheng
David Jones
David Ndungu
Adam Langley
Alex Vaghin
Leo Antunes
Brad Morgan
Jake Burkhead
Ingo Gottwald
Kevin Burke
Joe Kyo