pmproxy: release the X509 reference in the CERT_REQD check

[wtatarski]

SSL_get_peer_certificate() returns a certificate whose reference count has been
incremented, so the caller owns that reference and must release it with X509_free().
On OpenSSL 3.x the name is a compatibility macro for SSL_get1_peer_certificate(),
which has the same owning semantics.

81a9efe ("pmproxy: enforce -Q (CERT_REQD) for REST API connections") called it
inline in the NULL test and discarded the result:

	if (!client->stream.secure ||
	    !client->secure.ssl ||
	    SSL_get_peer_certificate(client->secure.ssl) == NULL) {
	    client->u.http.parser.status_code = HTTP_STATUS_FORBIDDEN;
	}

Because of the short-circuit order the reference is only taken when the connection is
TLS and an SSL object exists, and it is leaked precisely when a certificate is
present — that is, on the path where the request is allowed through. The rejection paths
do not leak. on_headers_complete() runs once per request, so a pmproxy started with
-Q accumulates one X509 per authenticated REST request for the lifetime of the
daemon.

This only affects deployments that have opted into client-certificate enforcement; -Q
is not enabled by the shipped pmproxy.conf. It is a reliability defect rather than a
security issue, since reaching it requires a client certificate that already passed
validation.

The fix captures the reference and frees it, leaving the accept/reject logic unchanged.
SSL_get0_peer_certificate() would avoid taking a reference at all, but it is not
available on the older OpenSSL versions pcp still supports, so X509_free() is the
portable form.

Found while backporting 81a9efe to pcp 6.3.7 and 6.0.1 (OpenSSL 3.2.2). Verified
against current main (55365f2): the block is unchanged there, this commit introduces
the only call to SSL_get_peer_certificate() under src/, and there is no other
X509_free() in the tree — so nothing else was releasing it.

Related Issues

Fixes #2693

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 843e223c-779a-48f9-ad4c-5c96a1683e49

📥 Commits

Reviewing files that changed from the base of the PR and between 55365f2 and 7e3df62.

📒 Files selected for processing (1)

• src/pmproxy/src/http.c

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Improved cleanup during client-certificate verification.
  • Preserved existing security behavior: requests without a valid secure connection or peer certificate continue to be rejected.

Walkthrough

The CERT_REQD check in on_headers_complete now stores the peer certificate in a local variable and releases it with X509_free. Existing forbidden responses remain unchanged.

Changes

Certificate Reference Cleanup

Layer / File(s)	Summary
Release peer certificate references src/pmproxy/src/http.c	The certificate-required check releases a non-NULL peer certificate while preserving rejection for missing TLS state or certificates.

Suggested reviewers: natoscott

Poem

A rabbit checks the TLS gate

A borrowed certificate meets its fate
The NULL path still guards the door
The freed reference leaks no more
Requests hop safely through the night

Merge Risk: ⚪ Minimal · up to 7e3df

This localized change releases the certificate reference taken during CERT_REQD checks, preventing a per-request memory leak for affected TLS clients. No actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the primary change: releasing the X509 reference in the pmproxy CERT_REQD check.
Description check	✅ Passed	The description directly explains the X509 reference leak, its impact, and the implemented fix.
Linked Issues check	✅ Passed	The change addresses issue #2693 by capturing and freeing the certificate reference while preserving the existing short-circuit and acceptance logic.
Out of Scope Changes check	✅ Passed	The changes are limited to the CERT_REQD certificate check and contain no unrelated code changes.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[natoscott]

@wtatarski thanks!