Commit Graph

21 Commits

Author SHA1 Message Date
Aaron Gable 602f3e4708
Fix reference bug in CA.noteSignError (#7534)
In the process of writing
https://github.com/letsencrypt/boulder/pull/7533 I discovered that the
method for detecting pkcs11.Error errors is broken: it attempts to
unwrap the returned error into a pointer-to-a-pointer type, which
doesn't work because only `pkcs11.Error` implements the Error interface,
while `*pkcs11.Error` does not.

Add a test which shows that the current noteSignError implementation is
broken. Then fix noteSignError and the two locations which duplicate
that code by removing the extra layer of indirection. And since the same
code exists in three locations, refactor how the caImpl, ocspImpl, and
crlImpl share metrics so that it only has to exist in one place.

A minimal reproduction case of this type of breakage can be seen here:
https://go.dev/play/p/qCLDQ1SFiWu
2024-06-07 15:34:02 -04:00
Aaron Gable b92581d620
Better compile-time type checking for gRPC server implementations (#7504)
Replaced our embeds of foopb.UnimplementedFooServer with
foopb.UnsafeFooServer. Per the grpc-go docs this reduces the "forwards
compatibility" of our implementations, but that is only a concern for
codebases that are implementing gRPC interfaces maintained by third
parties, and which want to be able to update those third-party
dependencies without updating their own implementations in lockstep.
Because we update our protos and our implementations simultaneously, we
can remove this safety net to replace runtime type checking with
compile-time type checking.

However, that replacement is not enough, because we never pass our
implementation objects to a function which asserts that they match a
specific interface. So this PR also replaces our reflect-based unittests
with idiomatic interface assertions. I do not view this as a perfect
solution, as it relies on people implementing new gRPC servers to add
this line, but it is no worse than the status quo which relied on people
adding the "TestImplementation" test.

Fixes https://github.com/letsencrypt/boulder/issues/7497
2024-05-28 09:26:29 -07:00
Aaron Gable ab6e023b6f
Simplify issuance.NameID and how it is used (#7260)
Rename "IssuerNameID" to just "NameID". Similarly rename the standalone
functions which compute it to better describe their function. Add a
.NameID() directly to issuance.Issuer, so that callers in other packages
don't have to directly access the .Cert member of an Issuer. Finally,
rearrange the code in issuance.go to be sensibly grouped as concerning
NameIDs, Certificates, or Issuers, rather than all mixed up between the
three.

Fixes https://github.com/letsencrypt/boulder/issues/5152
2024-01-17 12:55:56 -08:00
Aaron Gable a9a87cd4a8
Remove fallbacks from IssuerNameID to IssuerID (#7259)
The last rows using the old-style IssuerID were written to the database
in late 2021. Those rows have long since aged out -- we no longer serve
certificates or revocation information for them -- so we can remove the
code which handles those old-style IDs. This allows for some nice
simplifications in the CA's ocspImpl and in the Issuance package, which
will be useful for further reorganization of the CA and issuance
packages.

Fixes https://github.com/letsencrypt/boulder/issues/5152
2024-01-12 14:03:49 -08:00
Aaron Gable 5972d43924
Add error checking and default value for LifespanOCSP (#7222)
We do this in code, rather than with the config validation package,
because our custom config.Duration type confuses the config validator.

Fixes https://github.com/letsencrypt/boulder/issues/7219
2023-12-21 13:07:06 -05:00
Aaron Gable 7f49867ae9
Truncate ocsp thisUpdate to the minute, not the hour (#7191)
Truncating to the hour does not provide any meaningful protection
against signature preimage attacks, and can cause the thisUpdate and
producedAt fields to differ by up to 59 minutes from each other.
Instead, truncate to the minute, to match how x/crypto/ocsp sets the
producedAt field.

Fixes https://github.com/letsencrypt/boulder/issues/7190
2023-12-08 11:48:14 -08:00
Phil Porada 6925fad324
Finish migration from int64 timestamps to timestamppb (#7142)
This is a cleanup PR finishing the migration from int64 timestamps to
protobuf `*timestamppb.Timestamps` by removing all usage of the old
int64 fields. In the previous PR
https://github.com/letsencrypt/boulder/pull/7121 all fields were
switched to read from the protobuf timestamppb fields.

Adds a new case to `core.IsAnyNilOrZero` to check various properties of
a `*timestamppb.Timestamp` reducing the visual complexity for receivers.

Fixes https://github.com/letsencrypt/boulder/issues/7060
2023-11-27 13:37:31 -08:00
Phil Porada 034316ef6a
Rename int64 timestamp related protobuf fields to <fieldname>NS (#7069)
Rename all of int64 timestamp fields to `<fieldname>NS` to indicate they
are Unix nanosecond timestamps.

Part 1 of 4 related to
https://github.com/letsencrypt/boulder/issues/7060
2023-09-15 13:49:07 -04:00
Aaron Gable e55a276efe
CA: Remove deprecated config stanzas (#6595)
These config stanzas have been removed in staging and prod. They used to
configure the separate OCSP and CRL gRPC services provided by the CA
process, but the CA now provides those services on the same port as the
main CA gRPC service.

Fixes #6448
2023-04-07 09:37:34 -07:00
Aaron Gable 2af11e425c
CA: Add configs to disable each gRPC service (#6473)
Add three new boolean config keys to the CA's config json:
- DisableCertService
- DisableOCSPService
- DisableCRLService

Setting any one of these flags to True and restarting the CA causes the
corresponding gRPC service (CertificateAuthority, OCSPGenerator, or
CRLGenerator, respectively) to not be started. The implementation is not
instantiated, the port is not opened, and no requests will be listened
for. All clients will receive failures to connect.

Also, create two temporary "dummy" implementations of the CRL and OCSP
services (and an interface for the fake OCSP service to meet), These are
necessary because the CertificateAuthority gRPC service currently
contains wrapper methods which replicate the methods of the OCSP and CRL
services, and which pass through calls to underlying implementation
objects. These objects must be replaced with fake objects which always
return an error when the service is disabled.

Fixes #6452
2022-11-01 10:00:37 -07:00
Aaron Gable 0340b574d9
Add unparam linter to CI (#6312)
Enable the "unparam" linter, which checks for unused function
parameters, unused function return values, and parameters and
return values that always have the same value every time they
are used.

In addition, fix many instances where the unparam linter complains
about our existing codebase. Remove error return values from a
number of functions that never return an error, remove or use
context and test parameters that were previously unused, and
simplify a number of (mostly test-only) functions that always take the
same value for their parameter. Most notably, remove the ability to
customize the RSA Public Exponent from the ceremony tooling,
since it should always be 65537 anyway.

Fixes #6104
2022-08-23 12:37:24 -07:00
Nina cdf1d321a5
ocsp: log revocation reason code (#6228)
In the CA, rather than logging just the revocation status (0 for Good,
1 for Revoked), begin logging the integer revocation reason (0 for
Unspecified, 1 for KeyCompromise, etc). For OCSP responses with the Good
status, log an underscore ("_") indicating that the revocation reason is
inapplicable.

This brings the OCSP audit logging in line with the new CRL logging,
which logs the reason for each serial included in a CRL.

Fixes #6214
2022-08-01 12:53:17 -07:00
Nina 6986f0d756
ca: avoid ocsp.ResponseStatus (#6213) 2022-07-05 15:43:06 -07:00
Aaron Gable ab79f96d7b
Fixup staticcheck and stylecheck, and violations thereof (#5897)
Add `stylecheck` to our list of lints, since it got separated out from
`staticcheck`. Fix the way we configure both to be clearer and not
rely on regexes.

Additionally fix a number of easy-to-change `staticcheck` and
`stylecheck` violations, allowing us to reduce our number of ignored
checks.

Part of #5681
2022-01-20 16:22:30 -08:00
Aaron Gable 4c2b07004c
Compute OCSP validity interval correctly (#5680)
Like certificates, the validity of an OCSP response is measured
inclusive of its final second. Ensure that we set the `nextUpdate`
timestamp taking this into account.

Fixes #5667
2021-09-30 11:00:24 -07:00
Aaron Gable bab688b98f
Remove sa-wrappers.go (#5663)
Remove the last of the gRPC wrapper files. In order to do so:

- Remove the `core.StorageGetter` interface. Replace it with a new
  interface (whose methods include the `...grpc.CallOption` arg)
  inside the `sa/proto/` package.
- Remove the `core.StorageAdder` interface. There's no real use-case
  for having a write-only interface.
- Remove the `core.StorageAuthority` interface, as it is now redundant
  with the autogenerated `sapb.StorageAuthorityClient` interface.
- Replace the `certificateStorage` interface (which appears in two
  different places) with a single unified interface also in `sa/proto/`.
- Update all test mocks to include the `_ ...grpc.CallOption` arg in
  their method signatures so they match the gRPC client interface.
- Delete many methods from mocks which are no longer necessary (mostly
  because they're mocking old authz1 methods that no longer exist).
- Move the two `test/inmem/` wrappers into their own sub-packages to
  avoid an import cycle.
- Simplify the `satest` package to satisfy one of its TODOs and to
  avoid an import cycle.
- Add many methods to the `test/inmem/sa/` wrapper, to accommodate all
  of the methods which are called in unittests.

Fixes #5600
2021-09-27 13:25:41 -07:00
Aaron Gable 3492a996ab
Update TODOs for Issue #5152 (#5591)
Changing how we're going to finally handle #5152: rather
than changing everything to use IssuerNameIDs, we're going
to change the meaning of IssuerID. This will allow us to avoid
renaming database columns and protobuf message fields.
2021-08-19 14:31:09 -07:00
Aaron Gable 2723a9b0b5
Allow IssuerID or IssuerNameID in OCSP requests (#5515)
Have the OCSP component of the CA keep track of both the (old-style)
IssuerID and the (new-style) IssuerNameID of all issuer certificates
loaded by its config. When receiving a requests to generate an OCSP
response, check the ID contained in that request against both lists,
preferring the IssuerNameID list.

This allows us to have GenerateOCSP clients (the RA and ocsp-updater)
begin specifying IssuerNameIDs in their requests, so that we can get
rid of the old IssuerID entirely.

Part of #5152
2021-07-09 10:20:59 -07:00
Aaron Gable 7bf854fe03
Move OCSP gRPC service to separate file and struct (#5402)
Create a new `ocspImpl` struct which satisfies the interface required
by the `OCSPGenerator` gRPC service. Move the `GenerateOCSP`
method from the `certificateAuthorityImpl` to this new type. To support
existing gRPC clients, keep a reference to the new OCSP service in
the CA impl, and maintain a pass-through `GenerateOCSP` method.
Simplify some of the CA setup code, and make the CA implementation
non-exported because it doesn't need to be.

In order to maintain our existing signature and sign error metrics,
they now need to be initialized outside the CA and OCSP constructors.
This complicates the tests slightly, but seems like a worthwhile
tradeoff.

Fixes #5226
Fixes #5086
2021-04-29 14:20:39 -07:00
Jacob Hoffman-Andrews 9464d025d1
Fix OCSP log queue blocking when maxLogLen = 0. (#5230)
Move responsibility for handling the default path (maxLogLen = 0)
from the ocspLogQueue component to CertificateAuthorityImpl. Now the
ocspLogQueue isn't constructed at all unless we configure it to be used.

Also, remove buffer from OCSP audit log chan. The bug this fixes was
hidden by the large buffer, and on reconsideration the large buffer was
unnecessary.

Verified that with the buffer removed, the integration test fails. Then
adding the fixes to the maxLogLen = 0 case fixes the integration test.

Fixes #5228
2021-01-15 11:32:00 -08:00
Jacob Hoffman-Andrews 8b9145838d
Add logging of OCSP generation events (#5223)
This adds a new component to the CA, ocspLogQueue, which batches up
OCSP generation events for audit logging. It will log accumulated
events when it reaches a certain line length, or when a maximum amount
of times has passed.
2021-01-12 15:31:49 -08:00