The RA performs an RPC to the SA's `GetValidAuthorizations` function
when attempting to find existing valid authorizations to reuse. Prior to
this commit, ff the RPC fails (e.g. due to a timeout) the calling code
logs the failure as a warning but fails to return the error and cease
processing. This results in a nil panic when we later try to index
`auths`
This commit inserts the missing `return` to ensure we don't process
further, thereby resolving #2274.
Right now, we only get single-threaded performance from our HSM, even though it
has multiple cores. We can use the pkcs11key's NewPool function to create a pool
of PKCS#11 sessions, allowing us to take advantage of the HSM's full
performance.
A number of SA functions used Go's named return value support. This can
make the code harder to follow and the Boulder style has moved to always
favouring normal returns.
This commit replaces all of the named return usage with traditional
returns and resolves#2248.
Along the way I updated a `// TODO` in `AddCertificate()` that was less
accurate than it could be. This resolves#2265.
* Switch back to go 1.5 in Travis.
* Add back GO15VENDOREXPERIMENT.
* Add GO15VENDOREXPERIMENT to Dockerfile
* Revert FAKE_DNS change.
* Revert "Properly close test servers (#2110)"
* Revert "Close VA HTTP test servers (#2111)"
* Change Godep version to 1.5.
* Standardize on issue number
### Connect before sending mail, not at startup
Per #2250 when we connect to the remote SMTP server at start-up time by calling `mailer.Connect()` but do not actually call `mailer.SendMail()` until after we have done some potentially expensive/time-consuming work we are liable to have our connection closed due to timeout.
This PR moves the `Connect()` call in `expiration-mailer` and `notify-mailer` to be closer to where the actual messages are sent via `SendMail()` and resolves#2250
### Handle SMTP 421 errors gracefully
Issue #2249 describes a case where we see this SMTP error code from the remote server when our connection has been idle for too long. This would manifest when connecting to the remote server at startup, running a very long database query, and then sending mail. This commit allows the mailer to treat SMTP 421 errors as an event that should produce a reconnect attempt and resolves#2249.
A unit test is added to the mailer tests to test that reconnection works when the server sends a SMTP 421 error. Prior to b64e51f and support for SMTP 421 reconnection this test failed in a manner matching issue #2249:
```
go test -p 1 -race --test.run TestReconnectSMTP421
github.com/letsencrypt/boulder/mail
Wrote goodbye msg: 421 1.2.3 green.eggs.and.spam Error: timeout exceeded
Cutting off client early
--- FAIL: TestReconnectSMTP421 (0.00s)
mailer_test.go:257: Expected SendMail() to not fail. Got err: 421
1.2.3 green.eggs.and.spam Error: timeout exceeded
FAIL
FAIL github.com/letsencrypt/boulder/mail 0.023s
```
With b64e51f the test passes and the client gracefully reconnects.
The existing reconnect testing logic in the `mail-test-srv` integration tests is changed such that half of the forced disconnects are a normal clean connection close and half are a SMTP 421. This allows the existing integration test for server disconnects to be reused to test the 421 reconnect logic.
This means that we get a more useful log message for slow queries, and don't
need to close the MySQL connection. It also means that the query is actually
killed on the MySQL side, rather than just timing out and returning on the
client side.
We set the max_statement_time to 95% of the `readTimeout`.
Also set the `long_query_time` to 80% of the `readTimeout`, so that queries that are
close to timing out will be logged by MySQL's slow query logging.
In #2178 we moved to explicit `SELECT` statements using a set of `const`
fields for each type to support db migrations and forward compatibility.
This commit removes the temptation to interpolate queries by providing
convenience `SelectFoo` functions for each type allowing the caller to
provide the `WHERE` clause and arguments.
Resolves#2214.
To remove challenges with expired/pending authz's when they are deleted we want to introduce a foreign key relationship to the challenges table's authorizationID field with instruction to cascade on delete (#2155). As pointed out in a comment this is made difficult by the current usage of a separate pendingAuthorizations table for pending authorizations.
To be able to remove the pendingAuthorizations table entirely (#2163) we need to first stop using it. This PR introduces the code changes required to achieve this.
Notes:
The SA's NewPendingAuthorization function was updated to store all new pending auths in the authz table and to ensure the status is StatusPending.
The SA's GetAuthorization, UpdatePendingAuthorization, FinalizeAuthorization, and RevokeAuthorizationsByDomain functions were updated to properly handle the fact that a pending authz could be in either the pendingAuthorizations table, or the authz table, and to do the right thing accordingly.
Several places in the RA unit tests created a pending authorization with a status "Valid", then finalized it later. This broke when NewPendingAuthorization was changed to enforce Pending status before creating the authz row since the FinalizeAuthorization code expected to only finalize Valid rows. To fix this some of the RA unit tests were changed to explicitly set status to Valid before calling FinalizeAuthorization. This matches the true intention of the tests to quickly create a pending & then finalized authorization.
The expired-authz-purger utility was updated to purge from both the pendingAuthorizations and authz table as required.
The return values of RevokeAuthorizationsByDomain have changed slightly. Previously it returned a 2 element array where the first element was the number of pending authorizations revoked and the second element was the number of finalized authorizations revoked. This is changed so that now it is the number of rows from the pendingAuthorizations and authz tables respectively. E.g. the second count for the authz table may now include non-finalized authzs in its count of affected rows. The admin-revoker is the only place that used this SA method and it was updated appropriately to describe the "rows" change.
The "purger" database user needs to have a new GRANT SELECT, DELETE for the authz table in addition to its existing GRANT for the pendingAuthorizations table.
This resolves#2162
Based on experience with the new gRPC staging deployment. gRPC generates `FullMethod` names such as `-ServiceName-MethodName` which can be confusing. For client calls to a service we actually want something formatted like `ServiceName-MethodName` and for server requests we want just `MethodName`.
This PR adds a method to clean up the `FullMethod` names returned by gRPC and formats them the way we expect.
The "20160817143417_AddCertStatusNotAfter.sql" db migration adds a "notAfter" column to the certificateStatus database table. This field duplicates the contents of the certificates table "expires" column. This enables performance improvements (see #1864) for both the ocsp-updater and the expiration-mailer utilities.
Since existing rows will have a NULL value in the new field the notafter-backfill utility exists to perform a one-time update of the existing certificateStatus rows to set their notAfter column based on the data that exists in the certificates table.
This follows on https://github.com/letsencrypt/boulder/pull/2177 and requires that the migration be applied & the feature flag set accordingly before use.
Fixes#2237.
Add feature flagged support for issuing for IDNs, fixes#597.
This patch expects that clients have performed valid IDN2008 encoding on any label that includes unicode characters. Invalid encodings (including non-compatible IDN2003 encoding) will be rejected. No script-mixing or script exclusion checks are performed as we assume that if a name is resolvable that it conforms to the registrar's policies on these matters and if it uses non-standard scripts in sub-domains etc that browsers should be the ones choosing how to display those names.
Required a full update of the golang.org/x/net tree to pull in golang.org/x/net/idna, all test suites pass.
Move features sections to the correct JSON object and only test registration validity if regCheck is true
* Pull other flag up to correct level
* Only check status update when status is non-empty
This commit fixes#2190 by allowing the OS to choose what port the test
Listener's bind to. This allows the tests to be run concurrently
without panicing when they all try to bind the same TCP port.
Previously, if we received a CSR with IPAddress or EmailAddress SANs, we would
ignore those fields, issuing only for the DNSNames in the CSR. However, we would
later check in MatchesCSR that the CSR's IPAddresses and EmailAddresses matches
those in the issued certificate. This check would fail, serving a 500 to the end
user.
Instead, we now reject the CSR earlier in the process, and send a
meaningful error message.
Fixes#2203
This PR adds a migration to create two new fields on the `certificateStatus` table: `notAfter` and `isExpired`. The rationale for these fields is explained in #1864. Usage of these fields is gated behind `features.CertStatusOptimizationsMigrated` per [CONTRIBUTING.md](https://github.com/letsencrypt/boulder/blob/master/CONTRIBUTING.md#gating-migrations). This flag should be set to true **only** when the `20160817143417_CertStatusOptimizations.sql` migration has been applied.
Points of difference from #2132 (the initial preparatory "all-in-one go" PR):
**Note 1**: Updating the `isExpired` field in the OCSP updater can not be done yet, the `notAfter` field needs to be fully populated first - otherwise a separate query or a messy `JOIN` would have to be used to determine if a certStatus `isExpired` by using the `certificates` table's `expires` field.
**Note 2**: Similarly we can't remove the `JOIN` on `certificates` from the `findStaleOCSPResponse` query yet until all DB rows have `notAfter` populated. This will happen in a separate **Part Two** PR.
This PR adds a divergence to the acme-divergence doc for Section 6.6 "Certificate Revocation". Boulder does not currently support authorizing a revocation request using an arbitrary account key that is authorized for the same domains as in the certificate.
Fixes#2160.
When we use Gorp's built-in `Get` method, it generates `SELECT *` queries. If we do a migration without a simultaneous change of the data structure, Gorp will subsequently error out when it sees a column in the output of the `SELECT *` which doesn't have a corresponding field in the struct it is trying to marshal. In order to be forward compatible with schema changes, we need to always use `SELECT a, b, c`, where `a`, `b`, and `c` are columns / fields in the current struct.
Unlike the `$GOTESTFLAGS` var there was no way to pass in a default
value to `test.sh` via `docker-compose -e` to indicate you only want to
run specific unit tests.
This commit puts the default `$TESTPATHS` into `$DEFAULT_TESTPATHS` and
assigns it to `$TESTPATHS` only if there isn't already a `$TESTPATHS`
value provided.
Example usage, running just the SA unit tests, with the "next" config,
using `-race -v`:
```
docker-compose run -e BOULDER_CONFIG_DIR="test/config-next"
-e RUN="unit" -e GOTESTFLAGS="-v -race"
-e TESTPATHS="github.com/letsencrypt/boulder/sa" boulder ./test.sh
```
This PR makes two improvements to how we handle migrations locally:
1) Prior to this PR an optimization was present in `test/create_db.sh` that would `exit 0` if the `boulder_sa_integration` database existed. This early exit meant that after the first invocation of `create_db.sh` no further `goose` migrations would be applied unless the operator dropped their databases or edited the script.
This PR reworks the existing DB optimization so that it only skips the `CREATE DATABASE` statements and allows `goose` to try and apply migrations. This doesn't result in significantly longer start up times because Goose is smart enough to know when no migrations are required and outputs something similar to:
`goose: no migrations to run. current version: 20160602142227`
This should address #2174.
2) This PR also implements a separate `sa/_db-next/` directory for "pending" migrations. This is meant to follow the "test/config" vs "test/config-next" approach to managing changes that are developed but not yet activated in production.
Migrations that are to-be-performed by Ops should be created in the `sa/_db-next` directory first. Once they have been performed by ops in staging/prod and the config flag gate for the migration (see CONTRIBUTING.md) has been set to true, the migration can be moved from `_db-next` to `_db`.
By default all pending migrations from the `-next` directory are applied in the local dev env. If you **do not** wish these migrations to be applied then set the `APPLY_NEXT_MIGRATIONS` env var to false. E.g.:
`docker-compose run -eAPPLY_NEXT_MIGRATIONS=false boulder`
This should address #2195
The LookupIPv6 flag has been enabled in production and isn't required anymore. This PR removes the flag entirely.
The errA and errAAAA error handling in LookupHost is left as-is, meaning that a non-nil errAAAA will not be returned to the caller. This matches the existing behaviour, and the expectations of the TestDNSLookupHost unit tests.
This commit also removes the tests from TestDNSLookupHost that tested the LookupIPv6 == false behaviours since those are no longer implemented.
Resolves#2191
The embedded `personModelv1` in the `personModelv2` struct should not be
a pointer. The instantiation code later on already uses
`personModelv1{}` instead of `&personModelv1{}` and does not need
updating, just the initial struct definition.
Updates #1699.
Adds a new package, `features`, which exposes methods to set and check if various internal features are enabled. The implementation uses global state to store the features so that services embedded in another service do not each require their own features map in order to check if something is enabled.
Requires a `boulder-tools` image update to include `golang.org/x/tools/cmd/stringer`.
* Fixes `mockEmailResolver` to return `sql.ErrNoRows`.
This commit reproduces the error observed in #2183 where a registration
ID is provided that doesn't match a row with a valid contact.
First a bug is fixed in the ID range check done by the
`mockEmailResolver` - it was using a `||` where it should have been
using a `&&` and also had a `> 0` where it needed `>= 0`, oops! slipped
past review!
Second the `mockEmailResolver` is modified to return `sql.ErrNoRows`
when the index is out of bounds for the mock data.
Lastly a ID of `999` is added to the `TestResolveEmails` function to
elicit the "mailer.send returned error: sql: no rows in result set"
error.
* Handles `sql.ErrNoRows` in `emailsForReg`.
This commit fixes#2183 (and the failing unit test introduced in the
prior commit) by handling `sql.ErrNoRows` in `emailsForReg` gracefully.
* Clarfies mockEmailResolver comment
Daniel
Roland Bracewell Shoemaker
Jacob Hoffman-Andrews
Daniel McCarney
Ben Irving
Blake Griffith