refactor of the url_sig plugin to more modern c++ by traeak · Pull Request #13131 · apache/trafficserver

traeak · 2026-04-30T16:16:20Z

This is an internal refactor of the url_sig plugin to use more modern c++ (like std::string_view) and separation of the signing/verifictaion logic. No changes to any functionality or file format. Most TSError calls have been converted to debug.

Copilot

Pull request overview

This PR refactors the experimental url_sig remap plugin by splitting ATS-specific glue from cache-agnostic core logic (config parsing + signature verification), modernizing the implementation (e.g., std::string_view), and adding unit tests and updated tooling/docs.

Changes:

Extract core logic into url_sig_config.cc (istream-based config parsing) and url_sig_verify.cc (validation/HMAC/signature building).
Simplify url_sig.cc into an ATS adapter that loads config/regex and applies allow/deny behavior.
Add Catch2 unit tests plus Go helper tools and refreshed documentation.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
plugins/experimental/url_sig/url_sig_verify.cc	New core signature validation and path-params parsing logic.
plugins/experimental/url_sig/url_sig_config.cc	New core config parser (istream-based).
plugins/experimental/url_sig/url_sig.h	New shared header for core types/constants and public APIs.
plugins/experimental/url_sig/url_sig.cc	Refactored ATS remap adapter to delegate to core logic.
plugins/experimental/url_sig/unit_tests/test_url_sig.cc	New Catch2 unit tests for core parsing/verification.
plugins/experimental/url_sig/unit_tests/CMakeLists.txt	Build definition for the new unit test binary.
plugins/experimental/url_sig/CMakeLists.txt	Build plugin from multiple translation units and add unit tests when enabled.
plugins/experimental/url_sig/sign.pl	Deleted legacy Perl signing helper.
plugins/experimental/url_sig/genkeys.pl	Deleted legacy Perl key-generation helper.
plugins/experimental/url_sig/sign.go	New Go signing helper (standalone `go run`).
plugins/experimental/url_sig/genkeys.go	New Go key-generation helper (standalone `go run`).
plugins/experimental/url_sig/README.md	New/updated plugin documentation including architecture, tooling, and usage.
doc/admin-guide/plugins/url_sig.en.rst	Updated admin guide with architecture/build/test/tooling details.

Copilot · 2026-04-30T16:31:49Z

+The original Perl script ``sign.pl`` is still available with equivalent
+functionality. It requires ``Digest::SHA``, ``Digest::HMAC_MD5``, and
+``MIME::Base64::URLSafe``.



The documentation says sign.pl is still available with equivalent functionality, but this PR deletes sign.pl. Please reconcile the docs with the repository contents.

Suggested change

The original Perl script ``sign.pl`` is still available with equivalent

functionality. It requires ``Digest::SHA``, ``Digest::HMAC_MD5``, and

``MIME::Base64::URLSafe``.

Copilot · 2026-04-30T16:31:50Z

+      if (keynum >= static_cast<int>(cfg->keys.size())) {
+        cfg->keys.resize(keynum + 1);
+      }
+      cfg->keys[keynum] = std::string(value_part);


This resizes cfg->keys to keynum + 1 with no upper bound, so a typo like key999999 = ... can cause large memory growth. The legacy plugin limited keys to key0..key15 and the docs still describe that; consider enforcing a hard maximum to preserve behavior and avoid misconfiguration issues.

Copilot · 2026-04-30T16:31:50Z

+  // Validate config.
+  switch (cfg->err_status) {
+  case UrlSigErrStatus::MOVED_TEMPORARILY:
+    if (cfg->err_url.empty()) {
+      error = "Invalid config, err_status == 302, but err_url is empty";


Because err_status defaults to FORBIDDEN and this validation only checks consistency, load_config() will succeed even if the config file omits the required error_url line (and even if no keys are provided). The legacy plugin required error_url to be set during parsing. Consider tracking whether error_url (and at least one key) was seen and failing if not.

Copilot · 2026-04-30T16:31:50Z

+        TSUrlHttpQuerySet(rri->requestBufp, rri->requestUrl, result.app_query.c_str(), result.app_query.size());
+      } else {
+        TSUrlHttpQuerySet(rri->requestBufp, rri->requestUrl, nullptr, 0);


The return value from TSUrlHttpQuerySet() is ignored. The prior implementation checked for TS_SUCCESS and logged failures; keeping that check helps avoid silently failing to strip/set the query string (which can affect cache behavior).

Suggested change

TSUrlHttpQuerySet(rri->requestBufp, rri->requestUrl, result.app_query.c_str(), result.app_query.size());

} else {

TSUrlHttpQuerySet(rri->requestBufp, rri->requestUrl, nullptr, 0);

TSReturnCode rc =

TSUrlHttpQuerySet(rri->requestBufp, rri->requestUrl, result.app_query.c_str(), result.app_query.size());

if (rc != TS_SUCCESS) {

Dbg(dbg_ctl, "[url_sig] Failed call to TSUrlHttpQuerySet() while setting query string.");

}

} else {

TSReturnCode rc = TSUrlHttpQuerySet(rri->requestBufp, rri->requestUrl, nullptr, 0);

if (rc != TS_SUCCESS) {

Dbg(dbg_ctl, "[url_sig] Failed call to TSUrlHttpQuerySet() while clearing query string.");

}

Copilot · 2026-04-30T16:31:51Z

+// Helper to generate HMAC-SHA1 hex signature for test URLs.
+static std::string
+hmac_sha1_hex(std::string const &key, std::string const &data)
+{
+  unsigned char sig[20];


This test helper uses snprintf() but doesn't include <cstdio>, which can break builds depending on transitive includes. Add the missing include.

Copilot · 2026-04-30T16:31:52Z

+    return {};
+  }
+
+  if (static_cast<int>(query.size()) < MAX_QUERY_LEN) {


This length check uses < MAX_QUERY_LEN, which rejects query strings of exactly MAX_QUERY_LEN. The legacy code rejected only > MAX_QUERY_LEN, so this is a boundary behavior change.

Suggested change

if (static_cast<int>(query.size()) < MAX_QUERY_LEN) {

if (static_cast<int>(query.size()) <= MAX_QUERY_LEN) {

Copilot · 2026-04-30T16:31:52Z

+      signed_part.append(url_parts[i]);
+      signed_part.push_back('/');
+    }
+    if (j + 1 < par_val.size()) {


The parts-mask index (j) is advanced unconditionally, which changes legacy behavior for malformed P= values (the old code only advanced when the next character was 0 or 1). To avoid behavior changes, either validate P contains only 0/1 and fail fast, or preserve the legacy advancement rule.

Suggested change

if (j + 1 < par_val.size()) {

if (j + 1 < par_val.size() && (par_val[j + 1] == '0' || par_val[j + 1] == '1')) {

Copilot · 2026-04-30T16:31:52Z

+      } else {
+        cfg->err_status = UrlSigErrStatus::FORBIDDEN;
+        cfg->err_url.clear();


For error_url, any status other than 302 is currently treated as 403 (FORBIDDEN). The legacy implementation rejected unsupported status codes at instance creation, so configs like error_url = 404 would now silently change meaning. Consider explicitly accepting only 302/403 (or erroring on anything else) to keep compatibility.

Suggested change

} else {

cfg->err_status = UrlSigErrStatus::FORBIDDEN;

cfg->err_url.clear();

} else if (status_code == 403) {

cfg->err_status = UrlSigErrStatus::FORBIDDEN;

cfg->err_url.clear();

} else {

error = "Line " + std::to_string(line_no) + ": unsupported error_url status code " + std::to_string(status_code);

return nullptr;

Copilot · 2026-04-30T16:31:53Z

+The original Perl script ``genkeys.pl`` is still available for backward
+compatibility but requires the ``Digest::SHA`` and ``MIME::Base64::URLSafe``
+modules.



The documentation says genkeys.pl is still available for backward compatibility, but this PR deletes genkeys.pl. Please update the docs (or keep the script) so the shipped artifacts match the guidance.

Suggested change

The original Perl script ``genkeys.pl`` is still available for backward

compatibility but requires the ``Digest::SHA`` and ``MIME::Base64::URLSafe``

modules.

Copilot · 2026-04-30T16:31:53Z

+    int         pristine_len = 0;
+    char *const pristine_raw = TSUrlStringGet(mbuf, ul, &pristine_len);
+    url_to_check             = std::string(pristine_raw, pristine_len);
+    TSfree(pristine_raw);


TSHttpTxnPristineUrlGet() returns a TSMLoc (ul) that should be released with TSHandleMLocRelease(mbuf, TS_NULL_MLOC, ul) once you're done with it (see e.g. plugins/cachekey/cachekey.cc). This code frees the string but never releases ul, which can leak per-transaction resources in pristine URL mode.

Suggested change

TSfree(pristine_raw);

TSfree(pristine_raw);

TSHandleMLocRelease(mbuf, TS_NULL_MLOC, ul);

refactor of the url_sig plugin to more modern c++

96ae7b8

traeak marked this pull request as draft April 30, 2026 16:16

traeak added url_sig Plugins labels Apr 30, 2026

traeak requested a review from Copilot April 30, 2026 16:24

Copilot started reviewing on behalf of traeak April 30, 2026 16:25 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

allow tsutil/Regex to be used in the logic files

4b28852

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor of the url_sig plugin to more modern c++#13131

refactor of the url_sig plugin to more modern c++#13131
traeak wants to merge 2 commits intoapache:masterfrom
traeak:url_sig_refactor

traeak commented Apr 30, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	The original Perl script ``sign.pl`` is still available with equivalent
	functionality. It requires ``Digest::SHA``, ``Digest::HMAC_MD5``, and
	``MIME::Base64::URLSafe``.

-        TSUrlHttpQuerySet(rri->requestBufp, rri->requestUrl, result.app_query.c_str(), result.app_query.size());
-      } else {
-        TSUrlHttpQuerySet(rri->requestBufp, rri->requestUrl, nullptr, 0);
+        TSReturnCode rc =
+          TSUrlHttpQuerySet(rri->requestBufp, rri->requestUrl, result.app_query.c_str(), result.app_query.size());
+        if (rc != TS_SUCCESS) {
+          Dbg(dbg_ctl, "[url_sig] Failed call to TSUrlHttpQuerySet() while setting query string.");
+        }
+      } else {
+        TSReturnCode rc = TSUrlHttpQuerySet(rri->requestBufp, rri->requestUrl, nullptr, 0);
+        if (rc != TS_SUCCESS) {
+          Dbg(dbg_ctl, "[url_sig] Failed call to TSUrlHttpQuerySet() while clearing query string.");
+        }

	if (static_cast<int>(query.size()) < MAX_QUERY_LEN) {
	if (static_cast<int>(query.size()) <= MAX_QUERY_LEN) {

	if (j + 1 < par_val.size()) {
	if (j + 1 < par_val.size() && (par_val[j + 1] == '0' \|\| par_val[j + 1] == '1')) {

	The original Perl script ``genkeys.pl`` is still available for backward
	compatibility but requires the ``Digest::SHA`` and ``MIME::Base64::URLSafe``
	modules.

	TSfree(pristine_raw);
	TSfree(pristine_raw);
	TSHandleMLocRelease(mbuf, TS_NULL_MLOC, ul);

Conversation

traeak commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

traeak commented Apr 30, 2026 •

edited

Loading