Fixes for OpenBSD#585
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRefines OpenSSL/LibreSSL compile-time guards, explicitly sets 64-bit RADIX_BITS for POSIX builds, and updates manpage and check-local Makefile rules (suffix-based man rule, input-stem naming; removed per-target SHELL assignment). ChangesCross-platform OpenSSL, Radix, and Build Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/tpm2/TPMCmd/tpm/cryptolibs/Ossl/BnToOsslMath.csrc/tpm2/TPMCmd/tpm/cryptolibs/Ossl/BnToOsslMath.c:26:10: fatal error: 'BnOssl.h' file not found ... [truncated 748 characters] ... nfer/facebook-clang-plugins/clang/install/lib/clang/18/include" src/tpm12/tpm_crypto.cIn file included from src/tpm12/tpm_crypto.c:54: ... [truncated 1386 characters] ... /infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
man/man3/Makefile.am (1)
65-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the old pattern rule to avoid conflicting implicit conversions.
%.3 : %.podis still present while.pod.3:is newly added. Keeping both conversion rules is redundant and can cause the old rule to be selected, defeating the intended suffix-rule switch for BSD-oriented compatibility.Proposed cleanup
-%.3 : %.pod - `@if` test -n "$$SOURCE_DATE_EPOCH"; then \ - BUILD_DATE=$$(date +%F --utc --date="@$$SOURCE_DATE_EPOCH"); \ - else \ - BUILD_DATE=$$(date +%F); \ - fi; \ - pod2man -r "libtpms" \ - -c "" \ - -n $(basename $@) \ - --date="$$BUILD_DATE" \ - --section=3 $< > $@ - SUFFIXES: .pod .3 .pod.3: `@if` test -n "$$SOURCE_DATE_EPOCH"; then \ BUILD_DATE=$$(date +%F --utc --date="@$$SOURCE_DATE_EPOCH"); \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@man/man3/Makefile.am` around lines 65 - 89, Remove the old pattern rule "%.3 : %.pod" which conflicts with the new suffix-style rule ".pod.3:"; keep only the suffix rule ".pod.3:" (and its BUILD_DATE/pod2man invocation) so implicit conversions use the intended BSD-compatible suffix rule and avoid redundant/ambiguous implicit pattern matches.
🧹 Nitpick comments (1)
src/tpm2/TPMCmd/tpm/cryptolibs/Ossl/BnToOsslMath.c (1)
580-601: 💤 Low valueConsider adding a clarifying comment for LibreSSL routing.
The change correctly routes LibreSSL to the OpenSSL 3.0+ code path because LibreSSL lacks the
EC_POINTs_mul()function. While the code is correct, a brief comment would help future maintainers understand why LibreSSL takes this path despite having a lower version number.📝 Optional comment addition
else { -#if OPENSSL_VERSION_NUMBER >= 0x30000000L || defined(LIBRESSL_VERSION_NUMBER) +#if OPENSSL_VERSION_NUMBER >= 0x30000000L || defined(LIBRESSL_VERSION_NUMBER) + // LibreSSL never implemented EC_POINTs_mul, use alternative path EC_POINT *pR1 = EC_POINT_new(E->G);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tpm2/TPMCmd/tpm/cryptolibs/Ossl/BnToOsslMath.c` around lines 580 - 601, Add a short clarifying comment above the preprocessor branch that explains why LibreSSL is routed to the OpenSSL 3.0+ code path: LibreSSL does not provide EC_POINTs_mul(), so we must use the EC_POINT_mul/EC_POINT_add sequence (functions EC_POINT_mul, EC_POINT_add, EC_POINTs_mul are relevant) even though LIBRESSL_VERSION_NUMBER may be numerically lower; place the comment near the `#if` OPENSSL_VERSION_NUMBER >= 0x30000000L || defined(LIBRESSL_VERSION_NUMBER) line to make intent clear for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@man/man3/Makefile.am`:
- Around line 65-89: Remove the old pattern rule "%.3 : %.pod" which conflicts
with the new suffix-style rule ".pod.3:"; keep only the suffix rule ".pod.3:"
(and its BUILD_DATE/pod2man invocation) so implicit conversions use the intended
BSD-compatible suffix rule and avoid redundant/ambiguous implicit pattern
matches.
---
Nitpick comments:
In `@src/tpm2/TPMCmd/tpm/cryptolibs/Ossl/BnToOsslMath.c`:
- Around line 580-601: Add a short clarifying comment above the preprocessor
branch that explains why LibreSSL is routed to the OpenSSL 3.0+ code path:
LibreSSL does not provide EC_POINTs_mul(), so we must use the
EC_POINT_mul/EC_POINT_add sequence (functions EC_POINT_mul, EC_POINT_add,
EC_POINTs_mul are relevant) even though LIBRESSL_VERSION_NUMBER may be
numerically lower; place the comment near the `#if` OPENSSL_VERSION_NUMBER >=
0x30000000L || defined(LIBRESSL_VERSION_NUMBER) line to make intent clear for
future maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4ac4d90-7de6-474b-b57a-231b31968200
📒 Files selected for processing (5)
man/man3/Makefile.amsrc/Makefile.amsrc/tpm12/tpm_crypto.csrc/tpm2/TPMCmd/tpm/cryptolibs/Ossl/BnToOsslMath.csrc/tpm2/TPMCmd/tpm/include/tpm_public/tpm_radix.h
💤 Files with no reviewable changes (2)
- src/Makefile.am
- src/tpm2/TPMCmd/tpm/include/tpm_public/tpm_radix.h
|
Can you prefix the patch titles with My general concern with LibreSSL support is related to PQC supprt. I have lots of outstanding patches for support of PQC support (ML-DSA, ML-KEM) with OpenSSL libcrypto but I am not sure how or whether LibreSSL implements PQC and if it does it hopefully introduced the same API. Otherwise the LibreSSL support may be short-lived. |
| # endif | ||
| # ifndef RADIX_BITS | ||
| # error Need to determine RADIX_BITS value | ||
| # endif |
There was a problem hiding this comment.
Does this rule otherwise break your build?
There was a problem hiding this comment.
CC tpm2/libtpms_tpm2_la-ACTCommands.lo
In file included from tpm2/ACTCommands.c:62:
In file included from ./tpm2/TPMCmd/tpm/include/private/Tpm.h:8:
In file included from ./tpm2/TPMCmd/tpm/include/tpm_public/tpm_public.h:13:
./tpm2/TPMCmd/tpm/include/tpm_public/tpm_radix.h:16:4: error: Need to determine RADIX_BITS value
16 | # error Need to determine RADIX_BITS value
| ^
1 error generated.
There was a problem hiding this comment.
How and/or where does it determine the value for RADIX_BITS then?
There was a problem hiding this comment.
The lines below the change in tpm_radix.h
| LDFLAGS_ARCH += $(findstring -m32, $(AM_LDFLAGS)) | ||
| LDFLAGS_ARCH += $(findstring -m64, $(AM_LDFLAGS)) | ||
|
|
||
| check-local: SHELL?="/usr/bin/env bash" |
There was a problem hiding this comment.
I think I had added this originally because of the case statement below that may not work with other shells. What is the issue on OpenBSD?
There was a problem hiding this comment.
current$ make check
Making check in include
Making check in libtpms
Making check in man
Making check in man3
Making check in src
make check-local
make: don't know how to make bash" (prerequisite of: check-local)
Stop in src
*** Error 2 in src (Makefile:4537 'check-am')
*** Error 1 in /home/mbuhl/projects/libtpms (Makefile:472 'check-recursive': @fail= failcom='exit 1'; for f in x $MAKEFLAGS; do case $f in...)
To my understanding the current way it adds "/usr/bin/env" and "bash" as prerequisites and then it fails because there is no "bash" target or file in the current directory.
Maybe check-local: SHELL?="$(/usr/bin/env bash)"?
This works for my make check and gmake check but I don't know if is right.
There was a problem hiding this comment.
I also had switched to tcsh locally for a test and did not notice a failure on make check. Adding set; to the target showed all kinds of BASH variables, so it must have switched to bash automatically (after removing SHELL?= as you did), though I am not sure whether it would do this on all systems.
There was a problem hiding this comment.
Does SHELL?='/usr/bin/env bash' with single quotes work for you? CoderabbitAI seems to suggest this. It works for me.
There was a problem hiding this comment.
No, the SHELL?=... syntax is GNU make only.
To my understanding, the case statement is POSIX compatible: https://pubs.opengroup.org/onlinepubs/9799919799/utilities/V3_chap02.html#tag_19_09_04_05
And I can confirm that it works with my ksh (both gmake and make).
The target currently evaluates to:
case openbsd7.9 in \
openbsd*) ADDLIBS="-lc" ;; \
darwin*|freebsd*) LDFLAGS_OS="-shared" ;; \
*) ADDLIBS="" ;; \
esac; \
(cc $LDFLAGS_OS -nostdlib -L./.libs -ltpms $ADDLIBS 2>/dev/null || \
(echo "There are undefined symbols in libtpms ($LDFLAGS_OS )";\
cc $LDFLAGS_OS -nostdlib -L./.libs -ltpms $ADDLIBS 2>&1 | grep libtpms))
case openbsd7.9 in \
openbsd*) ADDLIBS="-lc" ;; \
darwin*|freebsd*) LDFLAGS_OS="-shared" ;; \
*) ADDLIBS="" ;; \
esac; \
cc $LDFLAGS_OS -nostdlib -L./.libs -ltpms $ADDLIBS 2>/dev/null
so cc -nostdlib -L./.libs -ltpms -lc for me. I can leave the ADDLIBS out and get the same result:
current$ cc -nostdlib -L./.libs -ltpms -lc
ld: warning: cannot find entry symbol _start; not setting start address
current$ cc -nostdlib -L./.libs -ltpms
ld: warning: cannot find entry symbol _start; not setting start address
Maybe we can wrap the whole target in bash -c?
check-local:
/usr/bin/env bash -c ' \
...
'
There was a problem hiding this comment.
Then let's remove the SHELL?= for now. And you can also remove the openbsd*) case if you don't need it, of course.
I used the case statement for wildcard matching because on FreeBSD host_os is freebsd15.0 for example, so we cannot easily replace it with an if statement.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Makefile.am (1)
711-711:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify
$()expansion semantics across Make implementations.In GNU Make,
$(/usr/bin/env bash)is variable/function expansion syntax—not shell command substitution. Since no Make variable named/usr/bin/env bashexists, this would expand to an empty string, effectively settingSHELL="". This could cause recipe failures.The intent is to avoid OpenBSD's make parsing the original value as prerequisites. A more portable approach may be to avoid embedded spaces or use different quoting.
#!/bin/bash # Test how GNU Make interprets the $() expansion in this context # Create a minimal test Makefile cat > /tmp/test_makefile <<'EOF' test-target: SHELL?="$(/usr/bin/env bash)" test-target: `@echo` "SHELL is: [$(SHELL)]" EOF echo "=== Testing with GNU make ===" make -f /tmp/test_makefile test-target rm /tmp/test_makefile,
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Makefile.am` at line 711, The Makefile sets SHELL using GNU-make-style expansion in the check-local target (SHELL?="$(/usr/bin/env bash)") which is nonportable and can evaluate to empty; replace that assignment with a portable explicit shell assignment (e.g., change the check-local target to set SHELL ?= /bin/bash) or otherwise avoid embedded spaces by assigning a single-token path to SHELL; update the check-local target where SHELL?="$(/usr/bin/env bash)" appears to use SHELL ?= /bin/bash (or another explicit single-path shell) to ensure consistent behavior across make implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/Makefile.am`:
- Line 711: The Makefile sets SHELL using GNU-make-style expansion in the
check-local target (SHELL?="$(/usr/bin/env bash)") which is nonportable and can
evaluate to empty; replace that assignment with a portable explicit shell
assignment (e.g., change the check-local target to set SHELL ?= /bin/bash) or
otherwise avoid embedded spaces by assigning a single-token path to SHELL;
update the check-local target where SHELL?="$(/usr/bin/env bash)" appears to use
SHELL ?= /bin/bash (or another explicit single-path shell) to ensure consistent
behavior across make implementations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79fa7718-cef4-45e4-bc5e-af59595b6437
📒 Files selected for processing (5)
man/man3/Makefile.amsrc/Makefile.amsrc/tpm12/tpm_crypto.csrc/tpm2/TPMCmd/tpm/cryptolibs/Ossl/BnToOsslMath.csrc/tpm2/TPMCmd/tpm/include/tpm_public/tpm_radix.h
💤 Files with no reviewable changes (1)
- src/tpm2/TPMCmd/tpm/include/tpm_public/tpm_radix.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tpm2/TPMCmd/tpm/cryptolibs/Ossl/BnToOsslMath.c
- man/man3/Makefile.am
- src/tpm12/tpm_crypto.c
|
I am occasionally building in a DragonflyBSD VM. There I recently saw that LibreSSL's openssl command line tool does not support the same options ( |
ML-KEM is available, ML-DSA not yet. I am confident that I can work around any ML-API incompatibilities once they arise. |
Regarding -CA and -CAfile, is it the following three locations?
|
Yes, these links look correct to me. At least some of the errors I saw when running swtpm test cases were related to these missing options... |
|
Please rebase on latest master. I moved swtpm to depend on OpenSSL 3.5 due to PQC and EdDSA dependencies... |
Signed-off-by: Moritz Buhl <mbuhl@openbsd.org>
Signed-off-by: Moritz Buhl <mbuhl@openbsd.org>
Signed-off-by: Moritz Buhl <mbuhl@openbsd.org>
Signed-off-by: Moritz Buhl <mbuhl@openbsd.org>
Signed-off-by: Moritz Buhl <mbuhl@openbsd.org>
|
Looks good now. I will merge it shortly. |
2 participants
To build this library on OpenBSD, I did:
A ports Makefile could look like this:
I'll send it to ports@ once I have a working swtpm.
Currently libtpms is linking against the libcrypto in base, I tried creating a OpenSSL flavor without success.
Summary by CodeRabbit