Skip to content

Commit 8cc23dd

Browse files
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 8cc23dd

12 files changed

Lines changed: 3267 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: 302 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,302 @@
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+
# OpenSSL is built from source as a static-only (no-shared) install
56+
# into ~/openssl, then nginx is pointed at it via --with-cc-opt and
57+
# --with-ld-opt only — NOT --with-openssl=<src>.
58+
#
59+
# Why NOT --with-openssl=<src>:
60+
# nginx's Makefile always injects a bare "-lcrypto" flag before the
61+
# explicit static archive paths in the final link command. When
62+
# --with-openssl=<src> is used, the search path does not include the
63+
# internal .openssl/lib directory, so that bare flag fails with
64+
# "cannot find -lcrypto" once libssl-dev is absent.
65+
#
66+
# Why libssl-dev must NOT be installed for these jobs:
67+
# With libssl-dev present, "-lcrypto" resolves to the system's
68+
# libcrypto.so.3 (OpenSSL 3.0.x) regardless of what comes later in
69+
# the link command, producing a binary that runs with the wrong
70+
# version. Without libssl-dev, "-lcrypto" and "-lssl" resolve
71+
# exclusively to the static archives in ~/openssl/lib64 via the
72+
# -L flag in --with-ld-opt.
73+
#
74+
# Only ubuntu-24.04; only the three currently maintained nginx versions.
75+
- { os: ubuntu-24.04, nginx: "1.26.3", openssl: "3.6.1", pcre: pcre2 }
76+
- { os: ubuntu-24.04, nginx: "1.28.3", openssl: "3.6.1", pcre: pcre2 }
77+
- { os: ubuntu-24.04, nginx: "1.29.7", openssl: "3.6.1", pcre: pcre2 }
78+
79+
steps:
80+
# -----------------------------------------------------------------------
81+
- name: Checkout module source
82+
uses: actions/checkout@v4
83+
84+
# -----------------------------------------------------------------------
85+
- name: Install system build dependencies
86+
run: |
87+
sudo apt-get update -qq
88+
sudo apt-get install -y --no-install-recommends \
89+
build-essential \
90+
zlib1g-dev \
91+
curl \
92+
ca-certificates
93+
94+
# -----------------------------------------------------------------------
95+
# libssl-dev provides the system OpenSSL headers and shared libraries.
96+
# It is required for system OpenSSL jobs (headers + libcrypto.so for
97+
# nginx's configure feature tests and final linking).
98+
#
99+
# It must NOT be installed for pinned OpenSSL jobs — see matrix comment.
100+
# -----------------------------------------------------------------------
101+
- name: Install system OpenSSL headers (system OpenSSL jobs only)
102+
if: matrix.openssl == 'system'
103+
run: sudo apt-get install -y --no-install-recommends libssl-dev
104+
105+
# -----------------------------------------------------------------------
106+
# PCRE: NGINX 1.20.x requires PCRE1 (libpcre3-dev).
107+
# NGINX 1.26+ uses PCRE2 (libpcre2-dev) by default.
108+
# -----------------------------------------------------------------------
109+
- name: Install PCRE1 (NGINX 1.20.x legacy)
110+
if: matrix.pcre == 'pcre1'
111+
run: sudo apt-get install -y --no-install-recommends libpcre3-dev
112+
113+
- name: Install PCRE2 (NGINX 1.26+)
114+
if: matrix.pcre == 'pcre2'
115+
run: sudo apt-get install -y --no-install-recommends libpcre2-dev
116+
117+
# -----------------------------------------------------------------------
118+
# Cache the installed static OpenSSL tree (~/openssl).
119+
# Key: version + OS. Bump -vN to bust manually if needed.
120+
#
121+
# Note on "Failed to save" warnings: GitHub Actions has no write lock on
122+
# cache keys. When several jobs share the same key and finish concurrently
123+
# for the first time, the first writer wins and the rest log a warning.
124+
# This is harmless — all jobs read the cache successfully on subsequent
125+
# runs. It is a first-run-only occurrence.
126+
# -----------------------------------------------------------------------
127+
- name: Cache OpenSSL ${{ matrix.openssl }} build
128+
if: matrix.openssl != 'system'
129+
id: cache-openssl
130+
uses: actions/cache@v4
131+
with:
132+
path: ~/openssl
133+
key: openssl-${{ matrix.openssl }}-${{ matrix.os }}-v1
134+
135+
# -----------------------------------------------------------------------
136+
# Build OpenSSL from source only on cache miss.
137+
#
138+
# "make build_sw" compiles libraries + CLI only — it skips the full test
139+
# suite (200+ binaries) that "make" would build, saving several minutes.
140+
# "make install_sw" installs into ~/openssl without docs or man pages.
141+
# -----------------------------------------------------------------------
142+
- name: Build OpenSSL ${{ matrix.openssl }} from source
143+
if: matrix.openssl != 'system' && steps.cache-openssl.outputs.cache-hit != 'true'
144+
env:
145+
OPENSSL_VERSION: ${{ matrix.openssl }}
146+
run: |
147+
curl -fsSL \
148+
"https://www.openssl.org/source/openssl-${OPENSSL_VERSION}.tar.gz" \
149+
-o openssl.tar.gz
150+
tar xf openssl.tar.gz
151+
cd "openssl-${OPENSSL_VERSION}"
152+
./Configure --prefix="${HOME}/openssl" \
153+
--openssldir="${HOME}/openssl" \
154+
no-shared linux-x86_64
155+
make -j"$(nproc)" build_sw
156+
make install_sw
157+
158+
# -----------------------------------------------------------------------
159+
- name: Download and extract NGINX ${{ matrix.nginx }}
160+
env:
161+
NGINX_VERSION: ${{ matrix.nginx }}
162+
run: |
163+
curl -fsSL \
164+
"https://nginx.org/download/nginx-${NGINX_VERSION}.tar.gz" \
165+
-o nginx.tar.gz
166+
tar xf nginx.tar.gz
167+
echo "NGINX_SRC=${PWD}/nginx-${NGINX_VERSION}" >> "${GITHUB_ENV}"
168+
169+
# -----------------------------------------------------------------------
170+
- name: Configure NGINX (system OpenSSL)
171+
if: matrix.openssl == 'system'
172+
run: |
173+
cd "${NGINX_SRC}"
174+
./configure \
175+
--with-http_ssl_module \
176+
--with-http_v2_module \
177+
--add-module="${GITHUB_WORKSPACE}" \
178+
--with-cc-opt="-Wall -Wextra -Wno-unused-parameter" \
179+
2>&1 | tee configure.log
180+
181+
# -----------------------------------------------------------------------
182+
# Pinned OpenSSL: configure nginx using --with-cc-opt / --with-ld-opt
183+
# pointing at ~/openssl. --with-openssl=<src> is intentionally omitted.
184+
#
185+
# --with-cc-opt: supplies the 3.6.1 headers for compilation.
186+
# --with-ld-opt: adds ~/openssl/lib64 to the linker search path so that
187+
# the "-lssl" and "-lcrypto" flags nginx injects resolve to the static
188+
# archives there. -ldl and -pthread satisfy OpenSSL's own link deps
189+
# when statically linked.
190+
# -----------------------------------------------------------------------
191+
- name: Configure NGINX (pinned OpenSSL ${{ matrix.openssl }})
192+
if: matrix.openssl != 'system'
193+
run: |
194+
cd "${NGINX_SRC}"
195+
./configure \
196+
--with-http_ssl_module \
197+
--with-http_v2_module \
198+
--with-cc-opt="-Wall -Wextra -Wno-unused-parameter -I${HOME}/openssl/include" \
199+
--with-ld-opt="-L${HOME}/openssl/lib64 -ldl -pthread" \
200+
--add-module="${GITHUB_WORKSPACE}" \
201+
2>&1 | tee configure.log
202+
203+
# -----------------------------------------------------------------------
204+
- name: Build NGINX
205+
run: |
206+
cd "${NGINX_SRC}"
207+
make -j"$(nproc)" 2>&1 | tee build.log
208+
ls -lh objs/nginx
209+
objs/nginx -V 2>&1
210+
211+
# -----------------------------------------------------------------------
212+
- name: Upload configure / build logs on failure
213+
if: failure()
214+
uses: actions/upload-artifact@v4
215+
with:
216+
name: build-logs-nginx${{ matrix.nginx }}-ossl${{ matrix.openssl }}-${{ matrix.os }}
217+
path: |
218+
${{ env.NGINX_SRC }}/configure.log
219+
${{ env.NGINX_SRC }}/build.log
220+
221+
# -----------------------------------------------------------------------
222+
# Cache cpanm-installed Perl modules (Test::Nginx + ~17 deps).
223+
# apt packages (cpanminus, libdigest-*) are fast and not cached.
224+
#
225+
# Both paths are required:
226+
# /usr/local/share/perl — pure-Perl modules
227+
# /usr/local/lib/perl5 — XS modules (e.g. List::MoreUtils::XS)
228+
#
229+
# "Failed to save" on first run: see OpenSSL cache note above — same
230+
# mechanism. Harmless; all parallel jobs on subsequent runs get hits.
231+
# -----------------------------------------------------------------------
232+
- name: Cache Perl dependencies
233+
id: cache-perl
234+
uses: actions/cache@v4
235+
with:
236+
path: |
237+
/usr/local/share/perl
238+
/usr/local/lib/perl5
239+
key: perl-test-nginx-0.32-${{ matrix.os }}-v1
240+
241+
- name: Install Perl test dependencies
242+
run: |
243+
sudo apt-get install -y --no-install-recommends \
244+
cpanminus \
245+
libdigest-sha-perl \
246+
libdigest-hmac-perl \
247+
liburi-perl
248+
if [ "${{ steps.cache-perl.outputs.cache-hit }}" != "true" ]; then
249+
sudo cpanm --notest Test::Nginx
250+
fi
251+
252+
# -----------------------------------------------------------------------
253+
- name: Verify NGINX binary
254+
run: |
255+
"${NGINX_SRC}/objs/nginx" -V 2>&1
256+
257+
# -----------------------------------------------------------------------
258+
- name: Syntax-check test files
259+
run: |
260+
for f in t/01_basic.t t/02_timestamps.t t/03_algorithms.t \
261+
t/04_variables.t t/05_integration.t; do
262+
perl -I t/lib -c "$f"
263+
done
264+
265+
# -----------------------------------------------------------------------
266+
- name: Run test suite
267+
env:
268+
TEST_NGINX_BINARY: "${{ env.NGINX_SRC }}/objs/nginx"
269+
TEST_NGINX_SERVROOT: "${{ runner.temp }}/nginx-test"
270+
run: prove -I t/lib -v --timer t/
271+
272+
# -----------------------------------------------------------------------
273+
- name: Upload nginx error log on test failure
274+
if: failure()
275+
uses: actions/upload-artifact@v4
276+
with:
277+
name: nginx-error-log-nginx${{ matrix.nginx }}-ossl${{ matrix.openssl }}-${{ matrix.os }}
278+
path: "${{ runner.temp }}/nginx-test/logs/error.log"
279+
280+
# ---------------------------------------------------------------------------
281+
# Lint: cppcheck static analysis — fast, no NGINX source needed.
282+
# ---------------------------------------------------------------------------
283+
lint:
284+
name: "cppcheck static analysis"
285+
runs-on: ubuntu-24.04
286+
287+
steps:
288+
- uses: actions/checkout@v4
289+
290+
- name: Install cppcheck
291+
run: sudo apt-get install -y cppcheck
292+
293+
- name: Run cppcheck
294+
run: |
295+
cppcheck \
296+
--enable=all \
297+
--inconclusive \
298+
--std=c99 \
299+
--suppress=missingIncludeSystem \
300+
--suppress=variableScope \
301+
--error-exitcode=1 \
302+
ngx_http_hmac_secure_link_module.c 2>&1

0 commit comments

Comments
 (0)