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.

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

Pluggable certificate storage (following on from #284) #330

Closed
wants to merge 3 commits into from

Conversation

hmarr
Copy link
Contributor

@hmarr hmarr commented Mar 31, 2019

Following on from the work @btwotch did in #284, this builds on their PR to address @oec's comment (I encountered a similar issue).

Recap

To recap, the current implementation will regenerate TLS certificates every request. Generating certificates is computationally expensive, so this leads to performance issues when there are many requests to the same host.

@btwotch made it possible to add a custom certificate store to alleviate this issue. However, multiple concurrent requests to the same host would result in the certificate being generated multiple times in parallel.

Changes in this PR

This PR adapts the certificate store's interface. Rather than having Store() and Load() as separate functions, a single function - Fetch() is provided with the cache key (hostname) and a function that returns the certificate to cache, and it handles the storage and retrieval internally.

While a simple storage implementation may mimic the previous behaviour (leaving the described problem in place), an alternative implementation of the certificate storage might take a lock on the
hostname when generating the certificate, meaning only one certificate will be generated and other clients will wait until it is present in the store.

Benchmark

Using a simple in-memory certificate store that takes a lock for each hostname while generating the certificate, I got the following results from some simple load testing using vegeta.

Without the certificate store

Note the mean latency of 1.3s, p99 latency of 11.8s, and success rate of 11.2%.

Requests      [total, rate]            250, 50.17
Duration      [total, attack, wait]    17.624958347s, 4.983325s, 12.641633347s
Latencies     [mean, 50, 95, 99, max]  1.262955939s, 0s, 11.787397416s, 13.733232781s, 14.133813748s
Bytes In      [total, mean]            315096, 1260.38
Bytes Out     [total, mean]            0, 0.00
Success       [ratio]                  11.20%
Status Codes  [code:count]             0:222  200:28

With the certificate store

Note the mean latency of 0.3s, p99 latency of 0.8ms, and success rate of 99.6. Quite an improvement!

Requests      [total, rate]            250, 50.13
Duration      [total, attack, wait]    5.257647755s, 4.987435s, 270.212755ms
Latencies     [mean, 50, 95, 99, max]  305.004131ms, 193.990013ms, 780.066772ms, 842.741389ms, 857.624176ms
Bytes In      [total, mean]            2796573, 11186.29
Bytes Out     [total, mean]            0, 0.00
Success       [ratio]                  99.60%
Status Codes  [code:count]             0:1  200:249

Credit to @btwotch for the foundation of this PR - their original commit was cherry-picked and used as the starting point.

Steven Falken and others added 3 commits March 30, 2019 16:44
Add the possibility to install a certificate storage to cache
certificates; this can make goproxy faster.
Rather than having Store() and Load() as separate functions, Fetch() is
provided with the key and a function that returns the value to cache,
and it handles the store and load internally.

A simple storage implementation may mimic the previous behaviour, but
the problem with this is many concurrent requests will result in the
certificate being generated many times in parallel.

An alternative implementation of the certificate storage might lock on
the key when generating the certificate, meaning only one certificate
will be generated and other clients will wait until it exists.
@hmarr
Copy link
Contributor Author

hmarr commented Mar 31, 2019

For reference, here's the certificate storage implementation I used. Happy to add this to this PR or another PR if you'd like it to be included in goproxy.

package main

import (
	"crypto/tls"
	"sync"
)

type certStore struct {
	certs map[string]*tls.Certificate
	locks map[string]*sync.Mutex
	sync.Mutex
}

func newCertStore() *certStore {
	return &certStore{
		certs: map[string]*tls.Certificate{},
		locks: map[string]*sync.Mutex{},
	}
}

func (s *certStore) Fetch(host string, genCert func() (*tls.Certificate, error)) (*tls.Certificate, error) {
	hostLock := s.hostLock(host)
	hostLock.Lock()
	defer hostLock.Unlock()

	cert, ok := s.certs[host]
	var err error
	if !ok {
		cert, err = genCert()
		if err != nil {
			return nil, err
		}
		s.certs[host] = cert
	}
	return cert, nil
}

func (s *certStore) hostLock(host string) *sync.Mutex {
	s.Lock()
	defer s.Unlock()

	lock, ok := s.locks[host]
	if !ok {
		lock = &sync.Mutex{}
		s.locks[host] = lock
	}
	return lock
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant