Skip to content

Commit bfc76c4

Browse files
denjidenji
committed
feat: v2.0.0 — RFC 7231 timestamps, OpenSSL 3.x, 14 bug fixes, full test suite
Add RFC 7231/IMF-fixdate and ISO 8601 'Z' suffix timestamp parsing, port HMAC computation to the OpenSSL 3.0 EVP_MAC API, fix 14 correctness and safety bugs found during code review, and add a full Perl test suite with CI. --- Timestamp parsing --- * Add ISO 8601 UTC 'Z' suffix support ("YYYY-MM-DDThh:mm:ssZ"). Previously fell through to the Unix branch and was silently misread. * Add RFC 7231 / IMF-fixdate support ("Day, DD Mon YYYY hh:mm:ss GMT"), including case-insensitive month-name lookup (RFC 7231 §7.1.1.1). * Fix RFC 7231 internal-comma bug: the embedded comma in "Sun, 06 Nov …" was previously mistaken for the field separator when an expiry field was also present, leaving only a 3-byte substring for the timestamp parser. Now detected and skipped before searching for the next field boundary. * Extract ngx_http_secure_link_gauss() with calendar-field range checks (year ≥ 1970, month 1–12, mday 1–31, hour 0–23, min 0–59, sec 0–60). * Extract ngx_http_secure_link_parse_ts() to centralise all four format branches; returns (time_t)-1 on any parse or range failure. --- Security / correctness --- * Fix Y2038 overflow: "365 * year" was int×int; large dates overflowed before the (time_t) cast. Fixed: (time_t)year * 365 promotes all Gauss arithmetic to the wider type. * Fix strict Unix timestamp validation: replaced sscanf "%llu" fallback (which accepted any digit-leading string) with a digit-only byte scan followed by ngx_atotm(). Prevents ISO 8601 year-only values (e.g. "2025") from being silently accepted. * Fix size_t/unsigned-int mismatch in token variable handler: passing (u_int*)&hmac.len to HMAC() wrote only 4 of 8 bytes on LP64; on big-endian the written bytes were in the wrong half. Use a local unsigned int hmac_len instead. * Fix $secure_link_hmac_expires returning token bytes: ctx->expires was set to value.data/len after value had been trimmed to the token. Now points to the actual expiry-period substring. * Fix EVP_MD_size() not checked: OpenSSL 3.0 returns -1 on error; the unchecked cast to u_int produced a huge length passed to CRYPTO_memcmp. * Fix GMT-offset sign: two independent `if` statements replaced with if/else if/else so an unexpected sign character cannot silently apply no offset. --- OpenSSL compatibility --- * Wrap HMAC computation in ngx_http_secure_link_hmac_compute(): uses EVP_MAC on OpenSSL >= 3.0 (no deprecation warning), HMAC() on 1.x. * NGX_HMAC_MD_SIZE macro wraps EVP_MD_size (1.x) / EVP_MD_get_size (3.x). * Add key.len overflow guard before narrowing size_t to int for HMAC(). --- Code quality --- * Remove incorrect (ngx_tm_*_t*) casts from sscanf calls; plain int* is correct for %d and portable across all platforms. * Change %02d to %2d in all sscanf format strings (the '0' flag is a printf-only concept; silently ignored by scanf). * Fix debug log timestamp width: was hardcoded to 25 bytes; now uses the actual substring length (int)(ts_end - ts_start). * Move gmtoff, var, i to inner scopes (cppcheck --enable=all, zero warnings). * Declare parse_ts ts_last parameter const (cppcheck constParameterPointer). --- Documentation --- * Replace verbose BUGS FIXED header block with a concise changelog; rationale lives in CHANGELOG.md and the commit history. * Document that the field separator in secure_link_hmac is always a comma (hardcoded) and the separator in secure_link_hmac_message is freely chosen by the operator (pipe, colon, slash, none, …). * Document all three embedded variables ($secure_link_hmac, $secure_link_hmac_expires, $secure_link_hmac_token) with their exact possible values and evaluation ordering requirements. * Add RFC 7231 client examples in PHP (gmdate), Node.js (toUTCString), and Python (isoformat + Z suffix). * Expand Security Notes with separator guidance and URL-encoding rules. --- Testing and CI --- * Add t/hmac_secure_link.t: 66 Test::Nginx::Socket tests across 10 categories (basic HMAC, Unix/ISO 8601/RFC 7231 timestamps, algorithms, variable values, separators, config edge cases, access-control patterns). * Add t/lib/HmacSecureLink.pm: shared token generators, timestamp formatters, TS_FIXED constant (eliminates timing-race in token tests). * Add .github/workflows/ci.yml. * Add Makefile with build, test, test-syntax, lint, cpan-deps targets. * Add CHANGELOG.md.
1 parent 60893da commit bfc76c4

12 files changed

Lines changed: 3196 additions & 225 deletions

‎.gitattributes‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
*.t linguist-language=Perl
2+
*.t linguist-vendored

‎.github/workflows/ci.yml‎

Lines changed: 231 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,231 @@
1+
name: CI
2+
3+
on:
4+
push:
5+
branches: ["master", "main", "develop"]
6+
pull_request:
7+
branches: ["master", "main", "develop"]
8+
9+
# ---------------------------------------------------------------------------
10+
# Concurrency: cancel in-progress runs on the same ref so that only the
11+
# latest commit on a branch is tested when pushes arrive quickly.
12+
# ---------------------------------------------------------------------------
13+
concurrency:
14+
group: ${{ github.workflow }}-${{ github.ref }}
15+
cancel-in-progress: true
16+
17+
jobs:
18+
test:
19+
name: "nginx-${{ matrix.nginx }} / openssl-${{ matrix.openssl }} / ${{ matrix.os }}"
20+
runs-on: ${{ matrix.os }}
21+
22+
strategy:
23+
fail-fast: false
24+
matrix:
25+
# -----------------------------------------------------------------------
26+
# Explicit include-only matrix — no cross-product, full control.
27+
#
28+
# NGINX version notes:
29+
# 1.20.2 legacy (Aug 2021) — PCRE1 only; ubuntu-22.04 exclusively
30+
# (libpcre3-dev in main, gcc 11 compatible with that vintage).
31+
# 1.26.3 legacy stable branch.
32+
# 1.28.3 current stable (even minor = stable).
33+
# 1.29.7 current mainline (odd minor = mainline).
34+
#
35+
# The "pcre" field is consumed by the install steps below; it does not
36+
# affect the job name shown in the GitHub UI.
37+
# -----------------------------------------------------------------------
38+
include:
39+
# -- Legacy: NGINX 1.20.2, PCRE1, Ubuntu 22.04 only ------------------
40+
- { os: ubuntu-22.04, nginx: "1.20.2", openssl: system, pcre: pcre1 }
41+
42+
# -- Legacy stable: NGINX 1.26.3 -------------------------------------
43+
- { os: ubuntu-22.04, nginx: "1.26.3", openssl: system, pcre: pcre2 }
44+
- { os: ubuntu-24.04, nginx: "1.26.3", openssl: system, pcre: pcre2 }
45+
46+
# -- Current stable: NGINX 1.28.3 ------------------------------------
47+
- { os: ubuntu-22.04, nginx: "1.28.3", openssl: system, pcre: pcre2 }
48+
- { os: ubuntu-24.04, nginx: "1.28.3", openssl: system, pcre: pcre2 }
49+
50+
# -- Mainline: NGINX 1.29.7 -------------------------------------------
51+
- { os: ubuntu-22.04, nginx: "1.29.7", openssl: system, pcre: pcre2 }
52+
- { os: ubuntu-24.04, nginx: "1.29.7", openssl: system, pcre: pcre2 }
53+
54+
# -- Pinned OpenSSL 3.6.1 — exercises the EVP_MAC code path -----------
55+
# Built from source; linked statically via --with-openssl= and
56+
# --with-openssl-opt=no-shared (prevents the dynamic linker from
57+
# falling back to the system OpenSSL at runtime).
58+
# Only ubuntu-24.04; only the three currently maintained versions.
59+
- { os: ubuntu-24.04, nginx: "1.26.3", openssl: "3.6.1", pcre: pcre2 }
60+
- { os: ubuntu-24.04, nginx: "1.28.3", openssl: "3.6.1", pcre: pcre2 }
61+
- { os: ubuntu-24.04, nginx: "1.29.7", openssl: "3.6.1", pcre: pcre2 }
62+
63+
steps:
64+
# -----------------------------------------------------------------------
65+
- name: Checkout module source
66+
uses: actions/checkout@v4
67+
68+
# -----------------------------------------------------------------------
69+
- name: Install system build dependencies
70+
run: |
71+
sudo apt-get update -qq
72+
sudo apt-get install -y --no-install-recommends \
73+
build-essential \
74+
zlib1g-dev \
75+
libssl-dev \
76+
curl \
77+
ca-certificates
78+
79+
# -----------------------------------------------------------------------
80+
# PCRE: NGINX 1.20.x requires PCRE1 (libpcre3-dev).
81+
# NGINX 1.26+ uses PCRE2 (libpcre2-dev) by default.
82+
# -----------------------------------------------------------------------
83+
- name: Install PCRE1 (NGINX 1.20.x legacy)
84+
if: matrix.pcre == 'pcre1'
85+
run: sudo apt-get install -y --no-install-recommends libpcre3-dev
86+
87+
- name: Install PCRE2 (NGINX 1.26+)
88+
if: matrix.pcre == 'pcre2'
89+
run: sudo apt-get install -y --no-install-recommends libpcre2-dev
90+
91+
# -----------------------------------------------------------------------
92+
# Build a pinned OpenSSL from source when matrix.openssl is not "system".
93+
# Installed into ${HOME}/openssl as a static build; the NGINX configure
94+
# step links against it via --with-openssl= and --with-openssl-opt=no-shared.
95+
# -----------------------------------------------------------------------
96+
- name: Build OpenSSL ${{ matrix.openssl }} from source
97+
if: matrix.openssl != 'system'
98+
env:
99+
OPENSSL_VERSION: ${{ matrix.openssl }}
100+
run: |
101+
curl -fsSL \
102+
"https://www.openssl.org/source/openssl-${OPENSSL_VERSION}.tar.gz" \
103+
-o openssl.tar.gz
104+
tar xf openssl.tar.gz
105+
cd "openssl-${OPENSSL_VERSION}"
106+
./Configure --prefix="${HOME}/openssl" \
107+
--openssldir="${HOME}/openssl" \
108+
no-shared linux-x86_64
109+
make -j"$(nproc)"
110+
make install_sw
111+
echo "OPENSSL_SRC=${PWD}" >> "${GITHUB_ENV}"
112+
113+
# -----------------------------------------------------------------------
114+
- name: Download and extract NGINX ${{ matrix.nginx }}
115+
env:
116+
NGINX_VERSION: ${{ matrix.nginx }}
117+
run: |
118+
curl -fsSL \
119+
"https://nginx.org/download/nginx-${NGINX_VERSION}.tar.gz" \
120+
-o nginx.tar.gz
121+
tar xf nginx.tar.gz
122+
echo "NGINX_SRC=${PWD}/nginx-${NGINX_VERSION}" >> "${GITHUB_ENV}"
123+
124+
# -----------------------------------------------------------------------
125+
- name: Configure NGINX (system OpenSSL)
126+
if: matrix.openssl == 'system'
127+
run: |
128+
cd "${NGINX_SRC}"
129+
./configure \
130+
--with-http_ssl_module \
131+
--with-http_v2_module \
132+
--add-module="${GITHUB_WORKSPACE}" \
133+
--with-cc-opt="-Wall -Wextra -Wno-unused-parameter" \
134+
2>&1 | tee configure.log
135+
136+
- name: Configure NGINX (pinned OpenSSL ${{ matrix.openssl }})
137+
if: matrix.openssl != 'system'
138+
run: |
139+
cd "${NGINX_SRC}"
140+
./configure \
141+
--with-http_ssl_module \
142+
--with-http_v2_module \
143+
--with-openssl="${OPENSSL_SRC}" \
144+
--with-openssl-opt=no-shared \
145+
--add-module="${GITHUB_WORKSPACE}" \
146+
--with-cc-opt="-Wall -Wextra -Wno-unused-parameter" \
147+
2>&1 | tee configure.log
148+
149+
# -----------------------------------------------------------------------
150+
- name: Build NGINX
151+
run: |
152+
cd "${NGINX_SRC}"
153+
make -j"$(nproc)" 2>&1 | tee build.log
154+
ls -lh objs/nginx
155+
objs/nginx -V 2>&1
156+
157+
# -----------------------------------------------------------------------
158+
- name: Upload configure / build logs on failure
159+
if: failure()
160+
uses: actions/upload-artifact@v4
161+
with:
162+
name: build-logs-nginx${{ matrix.nginx }}-ossl${{ matrix.openssl }}-${{ matrix.os }}
163+
path: |
164+
${{ env.NGINX_SRC }}/configure.log
165+
${{ env.NGINX_SRC }}/build.log
166+
167+
# -----------------------------------------------------------------------
168+
- name: Install Perl test dependencies
169+
run: |
170+
sudo apt-get install -y --no-install-recommends \
171+
cpanminus \
172+
libdigest-sha-perl \
173+
libdigest-hmac-perl \
174+
liburi-perl
175+
# Test::Nginx is not packaged in Ubuntu apt repos.
176+
sudo cpanm --notest Test::Nginx
177+
178+
# -----------------------------------------------------------------------
179+
- name: Verify NGINX binary
180+
run: |
181+
"${NGINX_SRC}/objs/nginx" -V 2>&1
182+
183+
# -----------------------------------------------------------------------
184+
- name: Syntax-check test files
185+
run: |
186+
for f in t/01_basic.t t/02_timestamps.t t/03_algorithms.t \
187+
t/04_variables.t t/05_integration.t; do
188+
perl -I t/lib -c "$f"
189+
done
190+
191+
# -----------------------------------------------------------------------
192+
- name: Run test suite
193+
env:
194+
TEST_NGINX_BINARY: "${{ env.NGINX_SRC }}/objs/nginx"
195+
TEST_NGINX_SERVROOT: "${{ runner.temp }}/nginx-test"
196+
run: |
197+
# Runs all test files: 01_basic.t 02_timestamps.t 03_algorithms.t
198+
# 04_variables.t 05_integration.t
199+
prove -I t/lib -v --timer t/
200+
201+
# -----------------------------------------------------------------------
202+
- name: Upload nginx error log on test failure
203+
if: failure()
204+
uses: actions/upload-artifact@v4
205+
with:
206+
name: nginx-error-log-nginx${{ matrix.nginx }}-ossl${{ matrix.openssl }}-${{ matrix.os }}
207+
path: "${{ runner.temp }}/nginx-test/logs/error.log"
208+
209+
# ---------------------------------------------------------------------------
210+
# Lint: cppcheck static analysis — fast, no NGINX source needed.
211+
# ---------------------------------------------------------------------------
212+
lint:
213+
name: "cppcheck static analysis"
214+
runs-on: ubuntu-24.04
215+
216+
steps:
217+
- uses: actions/checkout@v4
218+
219+
- name: Install cppcheck
220+
run: sudo apt-get install -y cppcheck
221+
222+
- name: Run cppcheck
223+
run: |
224+
cppcheck \
225+
--enable=all \
226+
--inconclusive \
227+
--std=c99 \
228+
--suppress=missingIncludeSystem \
229+
--suppress=variableScope \
230+
--error-exitcode=1 \
231+
ngx_http_hmac_secure_link_module.c 2>&1

‎CHANGELOG.md‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
# Changelog
2+
3+
All notable changes to **ngx_http_hmac_secure_link_module** are documented
4+
here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
5+
and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
6+
7+
---
8+
9+
## [2.0.0] — 2026-03-28
10+
11+
### Added
12+
13+
- **RFC 7231 / IMF-fixdate timestamp support.** HTTP dates in the format
14+
`"Day, DD Mon YYYY hh:mm:ss GMT"` (RFC 7231 §7.1.1.1) are now accepted as
15+
the timestamp field of `secure_link_hmac`. Month-name lookup is
16+
case-insensitive. All RFC 7231 dates are implicitly UTC.
17+
18+
- **ISO 8601 `Z` suffix support.** Timestamps of the form
19+
`"YYYY-MM-DDThh:mm:ssZ"` are now recognised as an alias for `+00:00` UTC
20+
offset. Previously they fell through to the Unix-timestamp branch and were
21+
silently misinterpreted.
22+
23+
- **OpenSSL 3.0+ `EVP_MAC` API.** HMAC computation now uses `EVP_MAC` on
24+
OpenSSL ≥ 3.0 (avoiding the `HMAC()` deprecation warning) and retains the
25+
`HMAC()` one-shot function on OpenSSL 1.0/1.1. Controlled by a
26+
`OPENSSL_VERSION_NUMBER` compile-time guard.
27+
28+
- **`NGX_HMAC_MD_SIZE` macro.** Wraps `EVP_MD_size()` (OpenSSL 1.x) and
29+
`EVP_MD_get_size()` (OpenSSL 3.x) behind a single call site.
30+
31+
- **Helper functions** extracted from the main variable handler:
32+
- `ngx_http_secure_link_gauss()` — Gauss proleptic Gregorian calendar
33+
formula with calendar-field range validation.
34+
- `ngx_http_secure_link_parse_ts()` — tries each timestamp format in
35+
order and returns `(time_t)-1` on failure.
36+
- `ngx_http_secure_link_hmac_compute()` — OpenSSL-version-aware HMAC
37+
computation with a key-length overflow guard.
38+
39+
- **Perl test suite** (`t/01_basic.t` … `t/05_integration.t`, 68 tests across 10
40+
categories split into five focused files) covering valid/expired/invalid tokens, all timestamp formats
41+
and their edge cases, algorithm variants, variable values, separator
42+
choices, and real-world access-control patterns.
43+
44+
- **Shared test helper module** (`t/lib/HmacSecureLink.pm`) providing
45+
token generators, timestamp formatters, and the `TS_FIXED` constant that
46+
eliminates timing-race failures in token-comparison tests.
47+
48+
- **CI matrix — NGINX versions updated:**
49+
- Added NGINX 1.28.3 (current stable) and 1.29.7 (current mainline) to
50+
the test matrix on ubuntu-22.04 and ubuntu-24.04.
51+
- Retained NGINX 1.26.3 (legacy stable) on ubuntu-22.04 and ubuntu-24.04.
52+
- Retained NGINX 1.20.2 (legacy, Aug 2021) on ubuntu-22.04 only; this
53+
version requires PCRE1 (`libpcre3-dev`) and is not compatible with PCRE2.
54+
55+
- **`Makefile`** for local build, test, lint, and dependency-installation
56+
targets.
57+
58+
### Fixed
59+
60+
- **Incorrect type casts in `sscanf`.** `sscanf "%d"` requires `int *`.
61+
The original code cast local `int` variables to `ngx_tm_year_t *`,
62+
`ngx_tm_mon_t *`, etc., which differ in size from `int` on some platforms,
63+
causing undefined behaviour and potential stack corruption.
64+
65+
- **Y2038 overflow in the Gauss formula.** The expression `365 * year` was
66+
computed as `int × int`. For years ≥ 2038 on 32-bit `int` platforms the
67+
accumulated day count overflowed before the `(time_t)` cast was applied.
68+
Fixed by casting `year` to `time_t` before the first multiplication.
69+
70+
- **`size_t` / `unsigned int` mismatch in token variable handler.** The
71+
original code passed `(u_int *) &hmac.len` to `HMAC()`. `hmac.len` is
72+
`size_t` (8 bytes on LP64); only the low 4 bytes were written correctly on
73+
little-endian targets. On big-endian LP64 the result was entirely wrong.
74+
Fixed by using a local `unsigned int hmac_len` variable.
75+
76+
- **`$secure_link_hmac_expires` returned token bytes instead of the expiry
77+
period.** `ctx->expires` was set to `value.data/len` at a point where
78+
`value` had already been trimmed to the token substring. Fixed by pointing
79+
`ctx->expires` at the actual expiry-period substring.
80+
81+
- **Loose Unix timestamp validation.** `sscanf(p, "%llu", …)` greedily
82+
accepted any string starting with digits — including ISO 8601 dates such as
83+
`"2025-01-01T…"` (parsed as `2025`). Fixed by requiring every byte in the
84+
timestamp substring to be a decimal digit.
85+
86+
- **`EVP_MD_size()` return value not checked.** OpenSSL 3.0 returns `-1` on
87+
error. The unchecked cast to `u_int` produced a huge buffer size and
88+
passed a garbage length to `CRYPTO_memcmp`. Fixed with an explicit
89+
`md_size <= 0` guard.
90+
91+
- **GMT-offset sign applied twice.** Two independent `if` statements allowed
92+
both branches to execute when the sign character was anything other than
93+
`'+'` or `'-'` (which `sscanf` already ensures cannot happen, but the
94+
logic was fragile). Fixed with `if / else if / else`.
95+
96+
- **RFC 7231 internal comma broke the expiry-field separator search.** The
97+
embedded comma in `"Sun, 06 Nov …"` was found before the real field
98+
separator when an expiry field was present, leaving `ts_end` pointing
99+
at a 3-byte substring that matched no parser. Fixed by detecting the
100+
RFC 7231 weekday pattern and skipping its internal comma before scanning
101+
for the next field boundary.
102+
103+
- **Debug log timestamp width hardcoded to 25 bytes.** The original code
104+
always logged 25 characters regardless of the actual timestamp format.
105+
Fixed to log `(int)(ts_end - ts_start)` bytes.
106+
107+
- **`%02d` in `sscanf` format strings.** The `0` flag is a printf-only
108+
concept; it is silently ignored by `scanf`. Changed to `%2d` throughout
109+
to eliminate misleading code.
110+
111+
- **`key.len` narrowed to `int` without a guard.** `HMAC()`'s `key_len`
112+
parameter is `int`, but `ngx_str_t.len` is `size_t`. Added an explicit
113+
overflow guard in the OpenSSL 1.x compatibility path.
114+
115+
### Changed
116+
117+
- File-level comment block replaced with a concise changelog (this file).
118+
Inline `/* FIX: … */` comments removed from function bodies; rationale
119+
lives here and in the commit history.
120+
121+
- `ngx_http_secure_link_add_variables`: loop variable `var` moved to inner
122+
scope to satisfy `cppcheck --enable=all`.
123+
124+
- `ngx_http_secure_link_parse_ts`: parameters `ts_last` declared `const`.
125+
126+
- **README — algorithm list updated for OpenSSL 3.x provider model:**
127+
- Algorithms are now grouped by provider: *default* (available without
128+
configuration — `sha256`, `sha3-*`, `blake2b512`, `sm3`, etc.) and
129+
*legacy* (requires the OpenSSL legacy provider to be loaded in
130+
`openssl.cnf` — `md4`, `mdc2`, `rmd160`, `gost`).
131+
- Added a note that `gost` and `mdc2` are not available on a default
132+
OpenSSL 3.x install.
133+
- Recommended algorithm note added: prefer `sha256` or stronger.
134+
135+
---
136+
137+
## [1.x] — prior to 2026
138+
139+
Original implementation by the nginx-modules project. Supported ISO 8601
140+
timestamps with numeric UTC offset and Unix timestamps. See repository
141+
history for individual changes.

0 commit comments

Comments
 (0)