feat: add sbom Makefile target#414
Conversation
Adds sbom target that calls gen-sbom to produce CycloneDX and SPDX output files. Parses version from ChangeLog.md. Sources enumerated from src/*.c. Requires WOLFSSL_DIR pointing to wolfssl tree with the feat/sbom-embedded branch (includes gen-sbom).
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a make sbom target to generate CycloneDX and SPDX SBOM artifacts for wolfHSM using wolfSSL’s scripts/gen-sbom.
Changes:
- Introduces an
sbomMakefile target with checks for version parsing andWOLFSSL_DIR. - Gathers sources from
src/*.cand passes compiler defines + metadata intogen-sbom. - Emits versioned
wolfhsm-<version>.cdx.jsonandwolfhsm-<version>.spdx.jsonoutputs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # ---- SBOM generation ---- | ||
| CC ?= cc | ||
| WOLFSSL_DIR ?= ../../wolfssl | ||
| VERSION := $(shell sed -n 's/^# wolfHSM Release v\([0-9][0-9.]*\).*/\1/p' ChangeLog.md | head -1) |
There was a problem hiding this comment.
Fixed — changed VERSION := to a recursive VERSION = (and SBOM_CDX/SBOM_SPDX likewise) so the sed only runs when the sbom target is invoked. Confirmed make -n clean no longer shells out.
| CC ?= cc | ||
| WOLFSSL_DIR ?= ../../wolfssl | ||
| VERSION := $(shell sed -n 's/^# wolfHSM Release v\([0-9][0-9.]*\).*/\1/p' ChangeLog.md | head -1) | ||
| SRCS := $(wildcard src/*.c) |
There was a problem hiding this comment.
Fixed — SRCS := $(sort $(wildcard src/*.c)) for reproducible ordering. Left the empty-list guard out: src/ always has .c files here and gen-sbom already errors on an empty --srcs.
| --supplier "wolfSSL Inc." \ | ||
| --license-file LICENSING \ | ||
| --options-h "$$_defines" \ | ||
| --srcs $(SRCS) \ |
There was a problem hiding this comment.
Fixed — SRCS := $(sort $(wildcard src/*.c)) for reproducible ordering.
| _py=$$(command -v python3 2>/dev/null || command -v python 2>/dev/null) && \ | ||
| [ -n "$$_py" ] || { echo "ERROR: python3 not found." >&2; exit 1; } && \ | ||
| "$$_py" $(WOLFSSL_DIR)/scripts/gen-sbom \ |
There was a problem hiding this comment.
Fixed — requires python3 explicitly now and errors if it's missing; dropped the python fallback.
| @_defines=$$(mktemp /tmp/wolfhsm-defines.XXXXXX) && \ | ||
| trap 'rm -f "$$_defines"' EXIT && \ |
There was a problem hiding this comment.
Fixed — switched to trap '...' 0, the portable form.
There was a problem hiding this comment.
Running make sbom WOLFSSL_DIR=../wolfssl against the latest wolfSSL master I get the following error:
ERROR: ../wolfssl/scripts/gen-sbom not found.
Use a wolfSSL tree that includes SBOM support.
make: *** [Makefile:73: sbom] Error 1
Did this not merge into wolfSSL master yet? If so it shouldn't merge into wolfHSM.
| wolfHSM uses a custom build system; invoke `gen-sbom` from the wolfssl source | ||
| tree directly: | ||
|
|
||
| ```sh | ||
| python3 $WOLFSSL_DIR/scripts/gen-sbom \ | ||
| --name wolfhsm \ | ||
| --version $(head -1 $WOLFHSM_DIR/ChangeLog.md | grep -oE '[0-9]+\.[0-9]+\.[0-9]+') \ | ||
| --supplier "wolfSSL Inc." \ | ||
| --options-h $WOLFSSL_DIR/include/wolfssl/options.h \ | ||
| --srcs $WOLFHSM_DIR/src/*.c | ||
| ``` | ||
|
|
||
| `WOLFSSL_DIR` must point to a wolfssl source tree containing `scripts/gen-sbom` | ||
| (branch `feat/sbom-embedded`, or `master` once wolfSSL/wolfssl#10343 merges). | ||
| `WOLFHSM_DIR` is the root of the wolfHSM source tree. | ||
|
|
||
| Requires `python3` and `pyspdxtools` (`pip install spdx-tools`). |
There was a problem hiding this comment.
this doc snippet looks outdated based on the changes in this PR, shouldn't we just recommend the end user run the new makefile target?
There was a problem hiding this comment.
Fixed — the section now leads with make sbom WOLFSSL_DIR=../wolfssl and keeps the raw gen-sbom command as a documented fallback, with flags matching the target.
jackctj117
left a comment
jackctj117
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: COMMENT
Findings: 5 total — 5 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review] Default WOLFSSL_DIR is off by one level for the top-level Makefile —
Makefile:62 - [Medium] [review+review-security] gen-sbom --options-h receives only compiler builtin macros, not wolfHSM/wolfSSL config —
Makefile:87-99 - [Low] [review+review-security] VERSION shell-out runs on every make invocation, not just sbom —
Makefile:63 - [Low] [review+review-security] Temp file uses hardcoded /tmp prefix, ignoring $TMPDIR —
Makefile:87 - [Low] [review+review-security] README manual invocation diverges from the Makefile sbom target —
README.md:37-44
Review generated by Skoll
|
|
||
| # ---- SBOM generation ---- | ||
| CC ?= cc | ||
| WOLFSSL_DIR ?= ../../wolfssl |
There was a problem hiding this comment.
🟠 [Medium] Default WOLFSSL_DIR is off by one level for the top-level Makefile · convention
The new default WOLFSSL_DIR ?= ../../wolfssl is copied from the sub-Makefiles in test/, benchmark/, etc., which live one directory below the repo root — for them, ../../wolfssl correctly resolves to a sibling checkout. But this assignment is in the top-level Makefile (repo root), so ../../wolfssl resolves to the grandparent directory, i.e. \<grandparent>/wolfssl, not the documented sibling \<parent>/wolfssl. On a standard sibling checkout, make sbom with no override will fail the [ ! -d "$(WOLFSSL_DIR)" ] guard. The correct default here is ../wolfssl. This won't cause silent misbehavior (the directory-exists guard produces a clear error and the PR docs tell users to pass WOLFSSL_DIR= explicitly), but the default is misleading and inconsistent with the repo's documented sibling layout (see CLAUDE.md 'Dependency layout').
Fix: Change the top-level default to ../wolfssl so a standard sibling checkout works out-of-the-box, or drop the default entirely and rely solely on the guard message since the docs already require an explicit WOLFSSL_DIR=.
There was a problem hiding this comment.
Fixed — the root Makefile's sibling checkout is ../wolfssl, so the default is now ../wolfssl.
| fi | ||
| @echo "wolfHSM version: $(VERSION)" | ||
| @echo "Sources: $(words $(SRCS)) .c files in src/" | ||
| @_defines=$$(mktemp /tmp/wolfhsm-defines.XXXXXX) && \ |
There was a problem hiding this comment.
🟠 [Medium] gen-sbom --options-h receives only compiler builtin macros, not wolfHSM/wolfSSL config · Logic
The new sbom target generates the defines file passed to --options-h by preprocessing an EMPTY translation unit: $(CC) -dM -E -I. -I$(WOLFSSL_DIR) -x c /dev/null. Because /dev/null includes no headers, -dM -E emits only the compiler's predefined macros (e.g. __GNUC__, __STDC__), NOT any WOLFHSM_CFG_* or wolfSSL HAVE_*/NO_* configuration macros. The -I. / -I$(WOLFSSL_DIR) include paths have no effect since nothing is #included. As a result the SBOM's build-configuration record is effectively empty, which undermines the stated CRA-compliance purpose of accurately capturing the build configuration, and diverges from the README, which instead points --options-h at a real options.h. Severity views differ across scans: the review mode rated this Medium (Low confidence, treating capture of compiler builtins as possibly intentional), while the review-security mode rated it Low (High confidence); the stricter Medium is retained.
Fix: Confirm what gen-sbom expects in --options-h. If it should reflect wolfHSM/wolfSSL configured macros, preprocess a stub that includes the relevant config header (e.g. wolfhsm/wh_settings.h) with the appropriate user_settings/config defines, or point --options-h at a concrete options header as the README shows; if only compiler builtins are needed, drop the no-op -I./-I$(WOLFSSL_DIR) flags to avoid implying otherwise.
There was a problem hiding this comment.
Good catch — confirmed. Preprocessing /dev/null with no -include emits only compiler builtins, which gen-sbom's noise filter then drops, so the SBOM's config section is effectively empty (the generated CDX had zero config macros). Deciding the right fix — point --options-h at a real options header, add -include <config header>, or use the --user-settings path — is a small design question I'd rather settle deliberately than paper over. Tracking it as a follow-up; not folding it into this PR unless you want it here.
There was a problem hiding this comment.
Update: implemented in 4cef60f rather than deferred. The recipe now preprocesses wolfhsm/wh_settings.h with the same defines/include path as the test build (-DWOLFHSM_CFG -DWOLFSSL_USER_SETTINGS, WOLFHSM_CFG_DIR on the path, default test/config), so the SBOM records the effective config — 31 WOLFHSM_CFG_* plus the wolfSSL options from user_settings.h, verified in the generated CDX. Integrators point WOLFHSM_CFG_DIR at their port's config dir. Known trade-off, documented inline on the target: wh_settings.h's system includes drag ~330 libc constants into the dump, and they are deliberately not filtered here because a prefix allowlist would drop real unprefixed options (GCM_TABLE_4BIT, FP_MAX_BITS, ...). The durable fix is a provenance-aware noise filter in gen-sbom itself (wolfSSL#10343); the Makefile comment spells out two implementation options and this recipe needs no change once that lands.
| # ---- SBOM generation ---- | ||
| CC ?= cc | ||
| WOLFSSL_DIR ?= ../../wolfssl | ||
| VERSION := $(shell sed -n 's/^# wolfHSM Release v\([0-9][0-9.]*\).*/\1/p' ChangeLog.md | head -1) |
There was a problem hiding this comment.
🔵 [Low] VERSION shell-out runs on every make invocation, not just sbom · Logic
VERSION := $(shell sed -n ... ChangeLog.md | head -1) uses immediate (:=) expansion at the top level of the root Makefile, so the sed | head subshell runs on every invocation (make all, make test, make clean, etc.), even though VERSION is only used by the sbom target. This is harmless but wasteful, and if ChangeLog.md were ever absent the shell would still run (silently producing an empty value) for unrelated targets. Note the SBOM_CDX/SBOM_SPDX names reference VERSION, so a lazy = assignment keeps them working.
Fix: Use a recursive = assignment (evaluated only when referenced) or a target-specific variable so the shell only runs when the sbom target is invoked.
There was a problem hiding this comment.
Fixed — recursive VERSION = now; the sed only runs when the sbom target is invoked.
| fi | ||
| @echo "wolfHSM version: $(VERSION)" | ||
| @echo "Sources: $(words $(SRCS)) .c files in src/" | ||
| @_defines=$$(mktemp /tmp/wolfhsm-defines.XXXXXX) && \ |
There was a problem hiding this comment.
🔵 [Low] Temp file uses hardcoded /tmp prefix, ignoring $TMPDIR · Resource Leak
The defines file is created with mktemp /tmp/wolfhsm-defines.XXXXXX, hardcoding /tmp. mktemp is used safely here (atomic creation with a random XXXXXX suffix and mode 0600, plus a trap ... EXIT cleanup scoped to the single recipe shell), so there is no symlink/predictability/TOCTOU vulnerability. However, hardcoding /tmp ignores $TMPDIR and can fail on environments where /tmp is unavailable or non-writable (some CI sandboxes, restricted containers, macOS).
Fix: Use mktemp without a hardcoded directory (respects $TMPDIR), e.g. mktemp "$${TMPDIR:-/tmp}/wolfhsm-defines.XXXXXX" or plain mktemp, for better portability across macOS/CI sandboxes.
There was a problem hiding this comment.
Fixed — mktemp "$${TMPDIR:-/tmp}/wolfhsm-defines.XXXXXX" so it respects $TMPDIR.
| wolfHSM uses a custom build system; invoke `gen-sbom` from the wolfssl source | ||
| tree directly: | ||
|
|
||
| ```sh |
There was a problem hiding this comment.
🔵 [Low] README manual invocation diverges from the Makefile sbom target · Logic
The README documents a manual gen-sbom invocation that differs from what make sbom actually runs: the README passes --options-h $WOLFSSL_DIR/include/wolfssl/options.h (a checked-in wolfSSL header) whereas the Makefile generates a defines file via $(CC) -dM -E; the README omits --license-file LICENSING, --cdx-out, and --spdx-out that the Makefile passes; and the README derives the version with head -1 ... | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' while the Makefile uses a sed pattern. Two documented-but-divergent invocation paths risk drifting out of sync and produce SBOMs with different content depending on which route a user follows.
Fix: Lead the README section with make sbom WOLFSSL_DIR=... (matching the new target, including --license-file) and keep the raw gen-sbom command as an explicit 'manual invocation' fallback, so both routes produce equivalent SBOMs and the documented flags stay consistent with the Makefile.
There was a problem hiding this comment.
Fixed — README manual invocation now matches the Makefile target (same flags and version derivation), with the raw command kept as a documented fallback.
| # ---- SBOM generation ---- | ||
| CC ?= cc | ||
| WOLFSSL_DIR ?= ../../wolfssl | ||
| VERSION := $(shell sed -n 's/^# wolfHSM Release v\([0-9][0-9.]*\).*/\1/p' ChangeLog.md | head -1) |
There was a problem hiding this comment.
Blocker — Makefile fails to parse on GNU Make 3.81 (macOS default make)
Line 63's VERSION := $(shell sed ... '([0-9][0-9.]*)' ...) trips a known make-3.81 function-scanner bug on the (...) parens. Because it's a file-scope :=, the error aborts every target, not just sbom:
$ make -n clean
Makefile:63: *** unterminated call to function shell': missing )'. Stop.
Repro: make 3.81 fails; gmake 4.4.1 is fine. The base Makefile parses under 3.81, so this is a regression. Fix by dropping the capture-group parens (e.g. grep -oE 'v[0-9.]+' + strip) or computing VERSION inside the sbom recipe (which also resolves Copilot's every-invocation note).
There was a problem hiding this comment.
Fixed in 4c0af52. The actual trigger is the # inside the file-scope $(shell): make <= 4.2 strips everything after it as a comment at read time, which is what leaves the unterminated function call — that's also why it aborted every target. The version extraction no longer uses # or escaped parens (sed -n 's/^. wolfHSM Release v//p' | cut -d' ' -f1), so nothing make-version-sensitive is left at file scope. Verified parse (make -n clean) and a full make sbom end-to-end on GNU Make 4.3; I don't have a 3.81 here, so a macOS confirm would be welcome.
|
@bigbrett Correct — gen-sbom hasn't merged to wolfssl master. It's in wolfssl PR wolfSSL/wolfssl#10343, which is still open, so no wolfssl release or master checkout has scripts/gen-sbom yet. The target can't create that file; what it can do is fail clearly. I updated the error to name the dependency and point you at setting WOLFSSL_DIR to a tree that has the script, and the README says the same. So the wolfHSM side is ready to use the moment #10343 lands; it just no-ops with a clear message until then. I did not vendor a copy of gen-sbom into wolfHSM. If you'd rather hold this PR until #10343 merges, flag it and I'll mark it blocked. |
Summary
Adds a
make sbomtarget that produces CycloneDX and SPDX SBOM files for wolfHSM.ChangeLog.md(e.g.,# wolfHSM Release v1.4.0)src/*.c(sorted)gen-sbomto produce CDX + SPDX outputsUsage
```sh
make sbom WOLFSSL_DIR=/path/to/wolfssl/source
```
Outputs:
wolfhsm-<version>.cdx.json,wolfhsm-<version>.spdx.jsonRequirements
WOLFSSL_DIRmust point to a wolfssl source tree containingscripts/gen-sbom(available on thefeat/sbom-embeddedbranch of wolfssl)python3andpip install spdx-toolson the build hostTest plan
make sbom WOLFSSL_DIR=...