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

ssl: reuse *tls.Config if connection settings are identical #1033

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kevinburke1
Copy link
Contributor

Previously, we would reload and re-parse a certificate from disk every
single time we initialized a connection and the sslrootcert setting
was enabled. This results in a lot of allocations that can be avoided.

Instead, save the *tls.Config for a given configuration hash, and
reuse it when we see it again.

Fixes #1032.

Previously, we would reload and re-parse a certificate from disk every
single time we initialized a connection and the sslrootcert setting
was enabled. This results in a lot of allocations that can be avoided.

Instead, save the *tls.Config for a given configuration hash, and
reuse it when we see it again.

Fixes lib#1032.
Copy link

@Lekensteyn Lekensteyn left a comment

Choose a reason for hiding this comment

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

Looks reasonable, however there is a potential issue with certain parameter values that would result in two instances being stored (one with the original value, one with the deleted values). See also #1031

hash := string(o.Hash())
// This pseudo-parameter is not recognized by the PostgreSQL server, so let's delete it after use.
configMapMu.Lock()
conf, ok := configMap[hash]
Copy link

@Lekensteyn Lekensteyn Apr 21, 2021

Choose a reason for hiding this comment

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

The hash can change due to delete(o, "sslrootcert") and delete(o, "sslinline") below.

Edit: Actually, delete(o, "sslinline") got removed in #1035.

@jmunson
Copy link

jmunson commented Sep 17, 2021

Unless I'm missing something, this looks like it would result in a stale certificate staying in memory until your program is restarted. I don't know if it would be appropriate to register a SIGHUP handler in a library to drop the cache, but that would be preferable to needing to restart the service.

Or maybe we could still stat() the file on connect time and re-parse if mtime is newer than what we have cached?

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.

Frequently allocating in x509.parseCertificate (to repeatedly parse the same certificate)
3 participants