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

Add a GetCertificateWithContext function #225

Merged
merged 1 commit into from
May 5, 2023

Conversation

ankon
Copy link
Contributor

@ankon ankon commented Apr 11, 2023

This basically would allow us to pass a context into the "certmagic world", from which it then would get passed through back to our storage implementation.

The reason we want that is so that we can actually set up application performance monitoring by instrumenting the calls.

Note that this does add a new public function, but unless you specifically set your GetCertificate in the TLSConfig to something that uses this function, everything should stay the same.

Our setup looks like this, very simplified:

certmagicConfig.Issuers = []certmagic.Issuer{acmeIssuer}

tlsConfig := certmagicConfig.TLSConfig()
tlsConfig.GetCertificate = func(clientHelloInfo *tls.ClientHelloInfo) (*tls.Certificate, error) {
	txn := monitoring.StartTransaction("TLS/GetCertificate")
	defer txn.End()
	txn.AddAttribute("hostname", clientHelloInfo.ServerName)
	txn.AddAttribute("supportedVersions", supportedVersionsToString(clientHelloInfo.SupportedVersions))

	// ... do some checks here to make sure we want to handle this request

	ctx := newrelic.NewContext(context.Background(), txn)
	cert, err := certmagicConfig.GetCertificateWithContext(ctx, clientHelloInfo)
	if err != nil {
		// ... review the error and possible ignore it/handle it differently
	}
	return cert, err
}

ankon referenced this pull request Apr 11, 2023
Should prevent stampedes to storage. See #185

Also minor fixes elsewhere.
@mholt
Copy link
Member

mholt commented May 5, 2023

Hi, sorry for the lack of reply -- I'll be getting around to this soon as I prepare the Caddy 2.7 beta 1 release :)

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the enhancement!

@mholt mholt merged commit 0dc2834 into caddyserver:master May 5, 2023
3 checks passed
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.

None yet

2 participants