Add OCSP stapling unit tests #259
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add a few tests to exercise OCSP stapling, using an httptest.Server acting as the OCSP responder. These tests are very simplistic: - a "good" OCSP response should be stapled - a "revoked" OCSP response should not be stapled - the DisableStapling option should be honored - OCSP stapling requires an issuing certificate No attempt is made to provide sensible timestamps, either in the test certificates or in the OCSP responses. This brings unit test coverage for ocsp.go from 21% to 70%.
|
Hi @mholt, please let me know if you have any feedback about this idea. But feel free to close if you think it's not worth bothering with something like this. |
|
Hey @kenjenkins ! So sorry. I am actually very interested in this. We just had a baby a couple months early and she's about to come home finally, so I've been very busy accumulating a backlog of issues and PRs to review. I've actually been wanting to review this first, since it looks solid and I love tests. I'll do that in a moment. :) |
mholt
approved these changes
Nov 29, 2023
|
Thanks @mholt, and no need to apologize. Congratulations on your baby girl! |
renovate bot
added a commit
to woodpecker-ci/woodpecker
that referenced
this pull request
Dec 17, 2023
[](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/caddyserver/certmagic](https://togithub.com/caddyserver/certmagic) | require | minor | `v0.19.2` -> `v0.20.0` | | [github.com/expr-lang/expr](https://togithub.com/expr-lang/expr) | require | patch | `v1.15.6` -> `v1.15.7` | | [github.com/google/uuid](https://togithub.com/google/uuid) | require | minor | `v1.4.0` -> `v1.5.0` | | [github.com/jellydator/ttlcache/v3](https://togithub.com/jellydator/ttlcache) | require | patch | `v3.1.0` -> `v3.1.1` | | [github.com/mattn/go-sqlite3](https://togithub.com/mattn/go-sqlite3) | require | patch | `v1.14.18` -> `v1.14.19` | | [github.com/xanzy/go-gitlab](https://togithub.com/xanzy/go-gitlab) | require | minor | `v0.94.0` -> `v0.95.2` | | [google.golang.org/grpc](https://togithub.com/grpc/grpc-go) | require | minor | `v1.59.0` -> `v1.60.0` | | [k8s.io/api](https://togithub.com/kubernetes/api) | require | minor | `v0.28.4` -> `v0.29.0` | | [k8s.io/apimachinery](https://togithub.com/kubernetes/apimachinery) | require | minor | `v0.28.4` -> `v0.29.0` | | [k8s.io/client-go](https://togithub.com/kubernetes/client-go) | require | minor | `v0.28.4` -> `v0.29.0` | --- ### Release Notes <details> <summary>caddyserver/certmagic (github.com/caddyserver/certmagic)</summary> ### [`v0.20.0`](https://togithub.com/caddyserver/certmagic/releases/tag/v0.20.0) [Compare Source](https://togithub.com/caddyserver/certmagic/compare/v0.19.2...v0.20.0) This release vastly improves storage cleaning as well improving a few smaller things. There is a minor breaking change as we get ever closer to v1.0. -⚠️ The `DecisionFunc` for On-Demand TLS now takes a `context.Context` value as its first argument. The context carries the `ClientHelloInfo` value (keyed by `ClientHelloInfoCtxKey`) for logging purposes. - Storage cleaning is now synchronized across the cluster, including process restarts. The state of cleaning expired certificates and OCSP staples is written to storage, and distributed locking is used to ensure that only 1 instance does it at a time. This greatly reduces costs for expensive storage backends! Cleaning is also done less often when the process is frequently restarted because the state is written to storage, so it is not forgotten after shutting down. - `.home.arpa` is now considered an internal suffix. - Backoff timings have been tuned based on real-world experience. #### What's Changed - README: Add hint about NextProtos for certmagic.TLS by [@​oliverpool](https://togithub.com/oliverpool) in [caddyserver/certmagic#251 - Bump golang.org/x/net from 0.11.0 to 0.17.0 by [@​dependabot](https://togithub.com/dependabot) in [caddyserver/certmagic#253 - Optionally pass the context argument down to the OnDemand decision func by [@​ankon](https://togithub.com/ankon) in [caddyserver/certmagic#255 - Retain the error stack if `checkIfCertShouldBeObtained` returns an error by [@​ankon](https://togithub.com/ankon) in [caddyserver/certmagic#256 - Add OCSP stapling unit tests by [@​kenjenkins](https://togithub.com/kenjenkins) in [caddyserver/certmagic#259 #### New Contributors - [@​oliverpool](https://togithub.com/oliverpool) made their first contribution in [caddyserver/certmagic#251 **Full Changelog**: caddyserver/certmagic@v0.19.2...v0.20.0 </details> <details> <summary>expr-lang/expr (github.com/expr-lang/expr)</summary> ### [`v1.15.7`](https://togithub.com/expr-lang/expr/releases/tag/v1.15.7) [Compare Source](https://togithub.com/expr-lang/expr/compare/v1.15.6...v1.15.7) **Expr** is a Go-centric expression language designed to deliver dynamic configurations with unparalleled accuracy, safety, and speed. ##### In this release: - Fixed commutative property for comparison between a value and a pointer. ([#​490](https://togithub.com/expr-lang/expr/issues/490)) - Checker: forbid accessing built-ins and custom functions from `$env`. ([#​495](https://togithub.com/expr-lang/expr/issues/495)) - Enhanced the number parser to include support for parsing hexadecimal, binary, and octal literals. ([#​483](https://togithub.com/expr-lang/expr/issues/483)) - Added `GetSource()` method to `vm.Program`. ([#​491](https://togithub.com/expr-lang/expr/issues/491)) </details> <details> <summary>google/uuid (github.com/google/uuid)</summary> ### [`v1.5.0`](https://togithub.com/google/uuid/releases/tag/v1.5.0) [Compare Source](https://togithub.com/google/uuid/compare/v1.4.0...v1.5.0) ##### Features - Validate UUID without creating new UUID ([#​141](https://togithub.com/google/uuid/issues/141)) ([9ee7366](https://togithub.com/google/uuid/commit/9ee7366e66c9ad96bab89139418a713dc584ae29)) </details> <details> <summary>jellydator/ttlcache (github.com/jellydator/ttlcache/v3)</summary> ### [`v3.1.1`](https://togithub.com/jellydator/ttlcache/releases/tag/v3.1.1) [Compare Source](https://togithub.com/jellydator/ttlcache/compare/v3.1.0...v3.1.1) Fix a bug in the `Range` method that causes a panic when the cache is empty </details> <details> <summary>mattn/go-sqlite3 (github.com/mattn/go-sqlite3)</summary> ### [`v1.14.19`](https://togithub.com/mattn/go-sqlite3/compare/v1.14.18...v1.14.19) [Compare Source](https://togithub.com/mattn/go-sqlite3/compare/v1.14.18...v1.14.19) </details> <details> <summary>xanzy/go-gitlab (github.com/xanzy/go-gitlab)</summary> ### [`v0.95.2`](https://togithub.com/xanzy/go-gitlab/compare/v0.95.1...v0.95.2) [Compare Source](https://togithub.com/xanzy/go-gitlab/compare/v0.95.1...v0.95.2) ### [`v0.95.1`](https://togithub.com/xanzy/go-gitlab/compare/v0.95.0...v0.95.1) [Compare Source](https://togithub.com/xanzy/go-gitlab/compare/v0.95.0...v0.95.1) ### [`v0.95.0`](https://togithub.com/xanzy/go-gitlab/compare/v0.94.0...v0.95.0) [Compare Source](https://togithub.com/xanzy/go-gitlab/compare/v0.94.0...v0.95.0) </details> <details> <summary>grpc/grpc-go (google.golang.org/grpc)</summary> ### [`v1.60.0`](https://togithub.com/grpc/grpc-go/releases/tag/v1.60.0): Release 1.60.0 [Compare Source](https://togithub.com/grpc/grpc-go/compare/v1.59.0...v1.60.0) ### Security - credentials/tls: if not set, set TLS MinVersion to 1.2 and CipherSuites according to supported suites not forbidden by RFC7540. - This is a behavior change to bring us into better alignment with RFC 7540. ### API Changes - resolver: remove deprecated and experimental `ClientConn.NewServiceConfig` ([#​6784](https://togithub.com/grpc/grpc-go/issues/6784)) - client: remove deprecated `grpc.WithServiceConfig` `DialOption` ([#​6800](https://togithub.com/grpc/grpc-go/issues/6800)) ### Bug Fixes - client: fix race that could cause a deadlock while entering idle mode and receiving a name resolver update ([#​6804](https://togithub.com/grpc/grpc-go/issues/6804)) - client: always enable TCP keepalives with OS defaults ([#​6834](https://togithub.com/grpc/grpc-go/issues/6834)) - credentials/alts: fix a bug preventing ALTS from connecting to the metadata server if the default scheme is overridden ([#​6686](https://togithub.com/grpc/grpc-go/issues/6686)) - Special Thanks: [@​mjamaloney](https://togithub.com/mjamaloney) ### Behavior Changes - server: Do not return from Stop() or GracefulStop() until all resources are released ([#​6489](https://togithub.com/grpc/grpc-go/issues/6489)) - Special Thanks: [@​fho](https://togithub.com/fho) ### Documentation - codes: clarify that only codes defined by this package are valid and that users should not cast other values to `codes.Code` ([#​6701](https://togithub.com/grpc/grpc-go/issues/6701)) </details> <details> <summary>kubernetes/api (k8s.io/api)</summary> ### [`v0.29.0`](https://togithub.com/kubernetes/api/compare/v0.28.4...v0.29.0) [Compare Source](https://togithub.com/kubernetes/api/compare/v0.28.4...v0.29.0) </details> <details> <summary>kubernetes/apimachinery (k8s.io/apimachinery)</summary> ### [`v0.29.0`](https://togithub.com/kubernetes/apimachinery/compare/v0.28.4...v0.29.0) [Compare Source](https://togithub.com/kubernetes/apimachinery/compare/v0.28.4...v0.29.0) </details> <details> <summary>kubernetes/client-go (k8s.io/client-go)</summary> ### [`v0.29.0`](https://togithub.com/kubernetes/client-go/compare/v0.28.4...v0.29.0) [Compare Source](https://togithub.com/kubernetes/client-go/compare/v0.28.4...v0.29.0) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 4am" (UTC), Automerge - "before 4am" (UTC). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/woodpecker-ci/woodpecker). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy45My4xIiwidXBkYXRlZEluVmVyIjoiMzcuOTMuMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Robert Kaussow <mail@thegeeklab.de>
](https://renovatebot.com){kind=link}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I thought it might be nice to add a few more unit tests for ocsp.go. (This has been at the back of my mind since #245, and I was curious to learn a little bit more about how OCSP works.) Please let me know if you have any concerns, either on the high-level approach or on specific details.
These test cases exercise some more of the logic starting from the
stapleOCSP()method, using a few more test certificates and anhttptest.Serveracting as the OCSP responder.These tests are very simplistic:
DisableStaplingoption should be honoredI haven't made any attempt to provide sensible timestamps, either in the test certificates or in the OCSP responses, as it's always a little tricky writing unit tests where
time.Now()is involved.I believe this should bring unit test coverage for ocsp.go from 21% to 70%.