tpm2-pkcs11: 1.9.0 -> 1.9.1 + adopt #378737
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Asked why fuzztests are failing in tpm2-software/tpm2-pkcs11#884 |
github-actions
bot
added
10.rebuild-darwin: 0
|
Integration tests require both Java and JUnit... got part of the way through trying them and stopped short 🙃 |
illdefined
suggested changes
Feb 15, 2025
|
Thanks, good feedback! |
numinit
force-pushed
the
tpm2-pkcs11/1.9.1
branch
from
45ffd9a to
9267af4
Compare
numinit
force-pushed
the
tpm2-pkcs11/1.9.1
branch
3 times, most recently
from
da7cb9a to
a811f44
Compare
illdefined
reviewed
Feb 16, 2025
Comment on lines
+142
to
+216
| tests.tpm2-pkcs11-abrmd = tpm2-pkcs11.override { | ||
| abrmdSupport = true; | ||
| }; |
There was a problem hiding this comment.
If the fuzzing tests are stable enough, perhaps we could enable them as passthrough tests. It is however just an idea and I defer to your judgment.
Suggested change
| tests.tpm2-pkcs11-abrmd = tpm2-pkcs11.override { | |
| abrmdSupport = true; | |
| }; | |
| tests = { | |
| tpm2-pkcs11-abrmd = tpm2-pkcs11.override { | |
| abrmdSupport = true; | |
| }; | |
| tpm2-pkcs11-fuzzing = tpm2-pkcs11.override { | |
| enableFuzzing = true; | |
| }; | |
| }; |
There was a problem hiding this comment.
Sadly they are not stable yet :(
| # The preConfigure phase doesn't seem to be working here | ||
| # ./bootstrap MUST be executed as the first step, before all | ||
| # of the autoreconfHook stuff | ||
| postPatch = '' | ||
| echo ${version} > VERSION | ||
| echo "$version" > VERSION |
There was a problem hiding this comment.
I wonder if this would work with __structuredAttrs enabled. If you happen to know that it does not, I’d suggest ${finalAttrs.version} instead.
There was a problem hiding this comment.
It works if I set __structuredAttrs = true; in the derivation.
numinit
force-pushed
the
tpm2-pkcs11/1.9.1
branch
from
a811f44 to
d3ff28f
Compare
|
The integration tests are a real mess. I managed to get half of them to work. I’ll provide a diff later. |
|
I suggest to leave the Java integration tests for now: diff --git a/Makefile-integration.am b/Makefile-integration.am
index e2255de..3cea1d8 100644
--- a/Makefile-integration.am
+++ b/Makefile-integration.am
@@ -7,7 +7,6 @@ integration_scripts = \
test/integration/pkcs11-dbup.sh.nosetup \
test/integration/tls-tests.sh \
test/integration/openssl.sh \
- test/integration/pkcs11-javarunner.sh.java \
test/integration/nss-tests.sh \
test/integration/ptool-link.sh.nosetup \
test/integration/python-pkcs11.sh
@@ -110,13 +109,5 @@ test_integration_pkcs_lockout_int_CFLAGS = $(AM_CFLAGS) $(TESTS_CFLAGS)
test_integration_pkcs_lockout_int_LDADD = $(TESTS_LDADD) $(SQLITE3_LIBS)
test_integration_pkcs_lockout_int_SOURCES = test/integration/pkcs-lockout.int.c test/integration/test.c
-#
-# Java Tests
-#
-AM_JAVA_LOG_FLAGS = --tabrmd-tcti=$(TABRMD_TCTI) --tsetup-script=$(top_srcdir)/test/integration/scripts/create_pkcs_store.sh
-JAVA_LOG_COMPILER=$(LOG_COMPILER)
-dist_noinst_JAVA = test/integration/PKCS11JavaTests.java
-CLEANFILES += test/integration/PKCS11JavaTests.class
-
endif
# END INTEGRATION
diff --git a/configure.ac b/configure.ac
index 1ec6eb4..7a0a8ee 100644
--- a/configure.ac
+++ b/configure.ac
@@ -258,13 +258,6 @@ AC_ARG_ENABLE(
[build and execute integration tests])],,
[enable_integration=no])
-# Test for Java compiler and interpreter without throwing fatal errors (since
-# these macros are defined using AC_DEFUN they cannot be called conditionally)
-m4_pushdef([AC_MSG_ERROR], [have_javac=no])
-AX_PROG_JAVAC()
-AX_PROG_JAVA()
-m4_popdef([AC_MSG_ERROR])
-
AC_DEFUN([integration_test_checks], [
AC_CHECK_PROG([tpm2_createprimary], [tpm2_createprimary], [yes], [no])
@@ -382,13 +375,6 @@ AC_DEFUN([integration_test_checks], [
[AC_MSG_ERROR([Integration tests enabled but tss2_provision executable not found.])])
])
- AS_IF([test "x$have_javac" = "xno"],
- [AC_MSG_ERROR([Integration tests enabled but no Java compiler was found])])
- AX_CHECK_CLASS([org.junit.Assert], ,
- [AC_MSG_ERROR([Integration tests enabled but JUnit not found, try setting CLASSPATH])])
- AX_CHECK_CLASS([org.hamcrest.SelfDescribing], ,
- [AC_MSG_ERROR([Integration tests enabled but Hamcrest not found, try setting CLASSPATH])])
-
AC_SUBST([ENABLE_INTEGRATION], [$enable_integration])
]) # end function integration_test_checks
And this works for me so far: diff --git a/pkgs/by-name/tp/tpm2-pkcs11/package.nix b/pkgs/by-name/tp/tpm2-pkcs11/package.nix
index d4803bf7f967..a67de7952bb7 100644
--- a/pkgs/by-name/tp/tpm2-pkcs11/package.nix
+++ b/pkgs/by-name/tp/tpm2-pkcs11/package.nix
@@ -1,24 +1,35 @@
{
autoconf-archive,
autoreconfHook,
+ buildEnv,
clangStdenv,
cmocka,
+ dbus,
+ expect,
fetchFromGitHub,
- glibc,
+ gnutls,
+ iproute2,
lib,
libyaml,
makeWrapper,
opensc,
+ openssh,
openssl,
+ nss,
+ p11-kit,
patchelf,
pkg-config,
python3,
stdenv,
sqlite,
+ swtpm,
tpm2-abrmd,
+ tpm2-openssl,
tpm2-pkcs11, # for passthru abrmd tests
tpm2-tools,
tpm2-tss,
+ which,
+ xxd,
abrmdSupport ? false,
fapiSupport ? true,
enableFuzzing ? false,
@@ -38,25 +49,37 @@ chosenStdenv.mkDerivation (finalAttrs: {
hash = "sha256-W74ckrpK7ypny1L3Gn7nNbOVh8zbHavIk/TX3b8XbI8=";
};
- # The preConfigure phase doesn't seem to be working here
- # ./bootstrap MUST be executed as the first step, before all
- # of the autoreconfHook stuff
+ # Disable Java‐based tests because of missing dependencies
+ patches = [ ./disable-java-integration.patch ];
+
postPatch = ''
- echo "$version" > VERSION
+ echo ${lib.escapeShellArg finalAttrs.version} >VERSION
# Don't run git in the bootstrap
substituteInPlace bootstrap --replace-warn "git" "# git"
- # Don't run tests with dbus
- substituteInPlace Makefile.am --replace-fail "dbus-run-session" "env"
+ # Provide configuration file for D-Bus
+ substituteInPlace Makefile.am --replace-fail \
+ "dbus-run-session" \
+ "dbus-run-session --config-file=${dbus}/share/dbus-1/session.conf"
+
+ # Disable failing tests
+ sed -E -i '/\<test\/integration\/(pkcs-crypt\.int|pkcs11-tool\.sh)\>/d' \
+ Makefile-integration.am
- patchShebangs test
+ patchShebangs test tools
+ # The preConfigure phase doesn't seem to be working here
+ # ./bootstrap MUST be executed as the first step, before all
+ # of the autoreconfHook stuff
./bootstrap
'';
configureFlags =
- lib.singleton (lib.enableFeature finalAttrs.doCheck "unit")
+ [
+ (lib.enableFeature finalAttrs.doCheck "unit")
+ (lib.enableFeature finalAttrs.doCheck "integration")
+ ]
++ lib.optionals enableFuzzing [
"--enable-fuzzing"
"--disable-hardening"
@@ -72,15 +95,20 @@ chosenStdenv.mkDerivation (finalAttrs: {
patchelf
pkg-config
(python3.withPackages (
- ps: with ps; [
+ ps:
+ with ps;
+ [
packaging
pyyaml
+ python-pkcs11
cryptography
pyasn1-modules
tpm2-pytss
]
+ ++ cryptography.optional-dependencies.ssh
))
];
+
buildInputs = [
libyaml
opensc
@@ -89,8 +117,28 @@ chosenStdenv.mkDerivation (finalAttrs: {
tpm2-tools
tpm2-tss
];
+
+ nativeCheckInputs = [
+ dbus
+ expect
+ gnutls
+ iproute2
+ nss.tools
+ opensc
+ openssh
+ openssl
+ p11-kit
+ sqlite
+ swtpm
+ tpm2-abrmd
+ tpm2-tools
+ which
+ xxd
+ ];
+
checkInputs = [
cmocka
+ tpm2-abrmd
];
enableParallelBuilding = true;
@@ -106,27 +154,40 @@ chosenStdenv.mkDerivation (finalAttrs: {
dontStrip = true;
dontPatchELF = true;
- # To be able to use the userspace resource manager, the RUNPATH must
- # explicitly include the tpm2-abrmd shared libraries.
- preFixup =
+ preConfigure = let
+ ldflags = [
+ "-Wl,--push-state,--no-as-needed"
+ "-ltss2-tcti-device"
+ "-ltss2-tcti-tabrmd"
+ "-Wl,--pop-state"
+ ];
+ in lib.optionalString abrmdSupport ''
+ configureFlagsArray+=(EXTRA_LDFLAGS=${lib.escapeShellArg ldflags})
+ '';
+
+ preCheck =
let
- rpath = lib.makeLibraryPath (
- (lib.optional abrmdSupport tpm2-abrmd)
- ++ [
- glibc
- libyaml
+ openssl-modules = buildEnv {
+ name = "openssl-modules";
+ pathsToLink = [ "/lib/ossl-modules" ];
+ paths = map lib.getLib [
openssl
- sqlite
- tpm2-tss
- ]
- );
+ tpm2-openssl
+ ];
+ };
in
''
- patchelf \
- --set-rpath ${rpath} \
- ${lib.optionalString abrmdSupport "--add-needed ${lib.makeLibraryPath [ tpm2-abrmd ]}/libtss2-tcti-tabrmd.so"} \
- --add-needed ${lib.makeLibraryPath [ tpm2-tss ]}/libtss2-tcti-device.so \
- $out/lib/libtpm2_pkcs11.so.0.0.0
+ # Enable tests to load TCTI modules
+ export LD_LIBRARY_PATH+=":${
+ lib.makeLibraryPath [
+ swtpm
+ tpm2-abrmd
+ tpm2-tools
+ ]
+ }"
+
+ # Enable tests to load TPM2 OpenSSL module
+ export OPENSSL_MODULES="${openssl-modules}/lib/ossl-modules"
'';
postInstall = '' |
|
Wow, well done. I'll integrate these. |
|
Apart from the Java‐based tests, I tried both swtpm and ibm-sw-tpm2 as emulators, but the tests fail with both of them. pkcs-crypt: pkcs11-tool: |
|
Some of the OpenSSL tests are skipped because they rely on OpenSSL 1, which I suspect (or hope) is soon going to be removed from nixpkgs. I tried to make them work, but I don’t believe that it is worth the effort. |
|
I'm not familiar with pkcs-crypt but p11tool is quite old compared to other alternatives. |
numinit
added a commit
to numinit/nixpkgs
that referenced
this pull request
Feb 17, 2025
h/t @illdefined for the work on these; see: NixOS#378737 (comment) Only difference is a nixfmt pass.
numinit
force-pushed
the
tpm2-pkcs11/1.9.1
branch
from
9846124 to
a3a0bac
Compare
numinit
added a commit
to numinit/nixpkgs
that referenced
this pull request
Feb 17, 2025
h/t @illdefined for the work on these; see: NixOS#378737 (comment) Only difference is a nixfmt pass.
|
Ran into that weird GH actions bug where nothing runs and I had to repush. |
illdefined
reviewed
Feb 17, 2025
Comment on lines
184
to
176
| lib.makeLibraryPath [ | ||
| swtpm | ||
| tpm2-abrmd | ||
| tpm2-tools | ||
| ] |
There was a problem hiding this comment.
Suggested change
| lib.makeLibraryPath [ | |
| swtpm | |
| tpm2-abrmd | |
| tpm2-tools | |
| ] | |
| lib.makeLibraryPath [ | |
| swtpm | |
| tpm2-tools | |
| ] ++ lib.optionals (!abrmdSupport) [ | |
| # Only add this if userspace‐resource manager support has been disabled to check | |
| # during testing whether it has been properly linked at compile time during. | |
| tpm2-abrmd | |
| ] |
There was a problem hiding this comment.
The passthru tests actually fail if I do that. I guess it's not getting linked properly?
There was a problem hiding this comment.
Maybe my approach to inject the linker flags was incorrect. If in doubt, just revert to the previous variant using patchelf.
|
I suggested a small change to check during testing that the tabrmd TCTI module has been properly linked, by omitting it from Otherwise this looks good to me. |
- Switch to clang since fuzzing uses clang's libfuzzed - Enable tests - Support optional fuzzing (currently fails)
numinit
force-pushed
the
tpm2-pkcs11/1.9.1
branch
from
a3a0bac to
845dbc1
Compare
numinit
added a commit
to numinit/nixpkgs
that referenced
this pull request
Feb 20, 2025
h/t @illdefined for the work on these; see: NixOS#378737 (comment) Only difference is a nixfmt pass.
|
With or without the patchelf and preConfigure, this is the first failure... Then here's one of the next: |
h/t @illdefined for the work on these; see: NixOS#378737 (comment) Only difference is a nixfmt pass.
numinit
force-pushed
the
tpm2-pkcs11/1.9.1
branch
from
845dbc1 to
1488556
Compare
|
Fixed it, reverted to adding abrmd in the LD_LIBRARY_PATH and the patchelf and it's working again. |
|
illdefined
approved these changes
Feb 23, 2025
wegank
added
the
12.approvals: 1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enabled the unit tests, but not the integration or fuzz tests.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.