Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add clock mock plumbing throughout the project #408

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add clock mock plumbing throughout the project #408

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e25f9b5
Add clock mock plumbing throughout the project
pzduniak May 16, 2023
5191bfa
Merge remote-tracking branch 'main/main' into pzduniak/mock-clock
pzduniak Apr 19, 2024
25322b2
Patch a new use of time.Now
pzduniak Apr 19, 2024
55d0abf
Merge remote-tracking branch 'main/main' into pzduniak/mock-clock
pzduniak May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions ca/ca.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import (
"strings"
"time"

"github.com/jmhodges/clock"

"github.com/letsencrypt/pebble/v2/acme"
"github.com/letsencrypt/pebble/v2/core"
"github.com/letsencrypt/pebble/v2/db"
Expand All @@ -34,6 +36,7 @@ type CAImpl struct {
log *log.Logger
db *db.MemoryStore
ocspResponderURL string
clockSource clock.Clock
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and throughout, let's rename clockSource to clk, to match Boulder.


chains []*chain

Expand Down Expand Up @@ -118,8 +121,8 @@ func (ca *CAImpl) makeRootCert(
template := &x509.Certificate{
Subject: subject,
SerialNumber: serial,
NotBefore: time.Now(),
NotAfter: time.Now().AddDate(30, 0, 0),
NotBefore: ca.clockSource.Now(),
NotAfter: ca.clockSource.Now().AddDate(30, 0, 0),

KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth},
Expand Down Expand Up @@ -269,7 +272,7 @@ func (ca *CAImpl) newCertificate(domains []string, ips []net.IP, key crypto.Publ
return nil, fmt.Errorf("cannot create subject key ID: %s", err.Error())
}

certNotBefore := time.Now()
certNotBefore := ca.clockSource.Now()
if notBefore != "" {
certNotBefore, err = time.Parse(time.RFC3339, notBefore)
if err != nil {
Expand Down Expand Up @@ -342,17 +345,21 @@ func (ca *CAImpl) newCertificate(domains []string, ips []net.IP, key crypto.Publ
return newCert, nil
}

func New(log *log.Logger, db *db.MemoryStore, ocspResponderURL string, alternateRoots int, chainLength int, certificateValidityPeriod uint64) *CAImpl {
func New(log *log.Logger, clockSource clock.Clock, db *db.MemoryStore, ocspResponderURL string, alternateRoots int, chainLength int, certificateValidityPeriod uint64) *CAImpl {
ca := &CAImpl{
log: log,
db: db,
clockSource: clockSource,
certValidityPeriod: defaultValidityPeriod,
}

if ocspResponderURL != "" {
ca.ocspResponderURL = ocspResponderURL
ca.log.Printf("Setting OCSP responder URL for issued certificates to %q", ca.ocspResponderURL)
}
if clockSource == nil {
ca.clockSource = clock.New()
}
Comment on lines +360 to +362
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments:

  1. Here and throughout, honestly I think it's fine to leave this check/default off. The clk can just be a required argument and pebble can just panic if it isn't provided. Just like it would if the db argument was nil.
  2. But if you really do want to keep it optional, do this check/default before the creation of the CAImpl on line 349.


intermediateSubject := pkix.Name{
CommonName: intermediateCAPrefix + hex.EncodeToString(makeSerial().Bytes()[:3]),
Expand Down
10 changes: 6 additions & 4 deletions cmd/pebble/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"os"
"strconv"

"github.com/jmhodges/clock"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"github.com/jmhodges/clock"
"github.com/jmhodges/clock"

"github.com/letsencrypt/pebble/v2/ca"
"github.com/letsencrypt/pebble/v2/cmd"
"github.com/letsencrypt/pebble/v2/db"
Expand Down Expand Up @@ -93,9 +94,10 @@ func main() {
chainLength = int(val)
}

db := db.NewMemoryStore()
ca := ca.New(logger, db, c.Pebble.OCSPResponderURL, alternateRoots, chainLength, c.Pebble.CertificateValidityPeriod)
va := va.New(logger, c.Pebble.HTTPPort, c.Pebble.TLSPort, *strictMode, *resolverAddress)
clockSource := clock.New()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on your PR description (you mention "breaking the external API") it sounds like you're using Pebble as a library, rather than as a standalone binary? In that case, your changes here make sense.

But if we're going to go to the trouble of giving Pebble full fake clock support, we should give it to everyone, including people who use Pebble as a binary (which I think is most users of Pebble).

To that end, let's replace this line with something a bit more like this, so that the fake clock can be initialized from an environment variable if someone wants to, or defaults to the system clock otherwise.

db := db.NewMemoryStore(clockSource)
ca := ca.New(logger, clockSource, db, c.Pebble.OCSPResponderURL, alternateRoots, chainLength, c.Pebble.CertificateValidityPeriod)
va := va.New(logger, clockSource, c.Pebble.HTTPPort, c.Pebble.TLSPort, *strictMode, *resolverAddress)

for keyID, key := range c.Pebble.ExternalAccountMACKeys {
err := db.AddExternalAccountKeyByID(keyID, key)
Expand All @@ -107,7 +109,7 @@ func main() {
cmd.FailOnError(err, "Failed to add domain to block list")
}

wfeImpl := wfe.New(logger, db, va, ca, *strictMode, c.Pebble.ExternalAccountBindingRequired, c.Pebble.RetryAfter.Authz, c.Pebble.RetryAfter.Order)
wfeImpl := wfe.New(logger, clockSource, db, va, ca, *strictMode, c.Pebble.ExternalAccountBindingRequired, c.Pebble.RetryAfter.Authz, c.Pebble.RetryAfter.Order)
muxHandler := wfeImpl.Handler()

if c.Pebble.ManagementListenAddress != "" {
Expand Down
4 changes: 2 additions & 2 deletions core/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ type Order struct {
CertificateObject *Certificate
}

func (o *Order) GetStatus() (string, error) {
func (o *Order) GetStatus(currentTime time.Time) (string, error) {
// Lock the order for reading
o.RLock()
defer o.RUnlock()
Expand All @@ -50,7 +50,7 @@ func (o *Order) GetStatus() (string, error) {

authzStatuses[authzStatus]++

if authzExpires.Before(time.Now()) {
if authzExpires.Before(currentTime) {
authzStatuses[acme.StatusExpired]++
}
}
Expand Down
18 changes: 12 additions & 6 deletions db/memorystore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ import (
"strconv"
"strings"
"sync"
"time"

"github.com/go-jose/go-jose/v4"
"github.com/jmhodges/clock"

"github.com/letsencrypt/pebble/v2/acme"
"github.com/letsencrypt/pebble/v2/core"
Expand All @@ -38,6 +38,8 @@ func (e ExistingAccountError) Error() string {
type MemoryStore struct {
sync.RWMutex

clockSource clock.Clock

accountRand *rand.Rand

accountsByID map[string]*core.Account
Expand All @@ -61,9 +63,13 @@ type MemoryStore struct {
blockListByDomain [][]string
}

func NewMemoryStore() *MemoryStore {
func NewMemoryStore(clockSource clock.Clock) *MemoryStore {
if clockSource == nil {
clockSource = clock.New()
}
return &MemoryStore{
accountRand: rand.New(rand.NewSource(time.Now().UnixNano())),
clockSource: clockSource,
accountRand: rand.New(rand.NewSource(clockSource.Now().UnixNano())),
accountsByID: make(map[string]*core.Account),
accountsByKeyID: make(map[string]*core.Account),
ordersByID: make(map[string]*core.Order),
Expand Down Expand Up @@ -200,7 +206,7 @@ func (m *MemoryStore) GetOrderByID(id string) *core.Order {
defer m.RUnlock()

if order, ok := m.ordersByID[id]; ok {
orderStatus, err := order.GetStatus()
orderStatus, err := order.GetStatus(m.clockSource.Now())
if err != nil {
panic(err)
}
Expand All @@ -218,7 +224,7 @@ func (m *MemoryStore) GetOrdersByAccountID(accountID string) []*core.Order {

if orders, ok := m.ordersByAccountID[accountID]; ok {
for _, order := range orders {
orderStatus, err := order.GetStatus()
orderStatus, err := order.GetStatus(m.clockSource.Now())
if err != nil {
panic(err)
}
Expand Down Expand Up @@ -268,7 +274,7 @@ func (m *MemoryStore) FindValidAuthorization(accountID string, identifier acme.I
authz.RLock()
if authz.Status == acme.StatusValid && identifier.Equals(authz.Identifier) &&
authz.Order != nil && authz.Order.AccountID == accountID &&
authz.ExpiresDate.After(time.Now()) {
authz.ExpiresDate.After(m.clockSource.Now()) {
authz.RUnlock()
return authz
}
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ go 1.21

require (
github.com/go-jose/go-jose/v4 v4.0.1
github.com/jmhodges/clock v1.2.0
github.com/letsencrypt/challtestsrv v1.3.2
github.com/miekg/dns v1.1.58
)
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ github.com/go-jose/go-jose/v4 v4.0.1 h1:QVEPDE3OluqXBQZDcnNvQrInro2h0e4eqNbnZSWq
github.com/go-jose/go-jose/v4 v4.0.1/go.mod h1:WVf9LFMHh/QVrmqrOfqun0C45tMe3RoiKJMPvgWwLfY=
github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
github.com/jmhodges/clock v1.2.0 h1:eq4kys+NI0PLngzaHEe7AmPT90XMGIEySD1JfV1PDIs=
github.com/jmhodges/clock v1.2.0/go.mod h1:qKjhA7x7u/lQpPB1XAqX1b1lCI/w3/fNuYpI/ZjLynI=
github.com/letsencrypt/challtestsrv v1.3.2 h1:pIDLBCLXR3B1DLmOmkkqg29qVa7DDozBnsOpL9PxmAY=
github.com/letsencrypt/challtestsrv v1.3.2/go.mod h1:Ur4e4FvELUXLGhkMztHOsPIsvGxD/kzSJninOrkM+zc=
github.com/miekg/dns v1.1.43/go.mod h1:+evo5L0630/F6ca/Z9+GAqzhjGyn8/c+TBaOyfEl0V4=
Expand Down
22 changes: 15 additions & 7 deletions va/va.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"strings"
"time"

"github.com/jmhodges/clock"
"github.com/miekg/dns"

"github.com/letsencrypt/challtestsrv"
Expand Down Expand Up @@ -98,6 +99,7 @@ type vaTask struct {
}

type VAImpl struct {
clockSource clock.Clock
log *log.Logger
httpPort int
tlsPort int
Expand All @@ -112,11 +114,13 @@ type VAImpl struct {

func New(
log *log.Logger,
clockSource clock.Clock,
httpPort, tlsPort int,
strict bool, customResolverAddr string,
) *VAImpl {
va := &VAImpl{
log: log,
clockSource: clockSource,
httpPort: httpPort,
tlsPort: tlsPort,
tasks: make(chan *vaTask, taskQueueSize),
Expand All @@ -126,6 +130,10 @@ func New(
customResolverAddr: customResolverAddr,
}

if clockSource == nil {
va.clockSource = clock.New()
}

if customResolverAddr != "" {
va.log.Printf("Using custom DNS resolver for ACME challenges: %s", customResolverAddr)
va.dnsClient = new(dns.Client)
Expand Down Expand Up @@ -195,7 +203,7 @@ func (va VAImpl) setAuthzValid(authz *core.Authorization, chal *core.Challenge)
authz.Lock()
defer authz.Unlock()
// Update the authz expiry for the new validity period
now := time.Now().UTC()
now := va.clockSource.Now().UTC()
authz.ExpiresDate = now.Add(validAuthzExpire)
authz.Expires = authz.ExpiresDate.Format(time.RFC3339)
// Update the authz status
Expand Down Expand Up @@ -244,7 +252,7 @@ func (va VAImpl) process(task *vaTask) {
chal := task.Challenge
chal.Lock()
// Update the validated date for the challenge
now := time.Now().UTC()
now := va.clockSource.Now().UTC()
chal.ValidatedDate = now
chal.Validated = chal.ValidatedDate.Format(time.RFC3339)
authz := chal.Authz
Expand Down Expand Up @@ -292,7 +300,7 @@ func (va VAImpl) performValidation(task *vaTask, results chan<- *core.Validation
// the URL to the `_acme-challenge` subdomain.
results <- &core.ValidationRecord{
URL: task.Identifier.Value,
ValidatedAt: time.Now(),
ValidatedAt: va.clockSource.Now(),
}
return
}
Expand All @@ -317,7 +325,7 @@ func (va VAImpl) validateDNS01(task *vaTask) *core.ValidationRecord {

result := &core.ValidationRecord{
URL: challengeSubdomain,
ValidatedAt: time.Now(),
ValidatedAt: va.clockSource.Now(),
}

txts, err := va.getTXTEntry(challengeSubdomain)
Expand Down Expand Up @@ -360,7 +368,7 @@ func (va VAImpl) validateDNSAccount01(task *vaTask) *core.ValidationRecord {

result := &core.ValidationRecord{
URL: challengeSubdomain,
ValidatedAt: time.Now(),
ValidatedAt: va.clockSource.Now(),
}

txts, err := va.getTXTEntry(challengeSubdomain)
Expand Down Expand Up @@ -404,7 +412,7 @@ func (va VAImpl) validateTLSALPN01(task *vaTask) *core.ValidationRecord {
}
result := &core.ValidationRecord{
URL: net.JoinHostPort(task.Identifier.Value, portString),
ValidatedAt: time.Now(),
ValidatedAt: va.clockSource.Now(),
}

addrs, err := va.resolveIP(task.Identifier.Value)
Expand Down Expand Up @@ -522,7 +530,7 @@ func (va VAImpl) validateHTTP01(task *vaTask) *core.ValidationRecord {

result := &core.ValidationRecord{
URL: url,
ValidatedAt: time.Now(),
ValidatedAt: va.clockSource.Now(),
Error: err,
}
if result.Error != nil {
Expand Down
6 changes: 4 additions & 2 deletions va/va_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"sync"
"testing"

"github.com/jmhodges/clock"

"github.com/letsencrypt/pebble/v2/acme"
"github.com/letsencrypt/pebble/v2/core"
"github.com/letsencrypt/pebble/v2/db"
Expand All @@ -30,8 +32,8 @@ func TestAuthzRace(_ *testing.T) {
// MemoryStore.FindValidAuthorization searches and tests authz.Status

// This whole test can be removed if/when the MemoryStore becomes 100% by value
ms := db.NewMemoryStore()
va := New(log.New(os.Stdout, "Pebble/TestRace", log.LstdFlags), 14000, 15000, false, "")
ms := db.NewMemoryStore(clock.New())
va := New(log.New(os.Stdout, "Pebble/TestRace", log.LstdFlags), clock.New(), 14000, 15000, false, "")

authz := &core.Authorization{
ID: "auth-id",
Expand Down
8 changes: 8 additions & 0 deletions vendor/github.com/jmhodges/clock/.travis.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 20 additions & 0 deletions vendor/github.com/jmhodges/clock/LICENSE
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions vendor/github.com/jmhodges/clock/README.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading