Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "tests" when built outside source directory #111

Merged
merged 1 commit into from
Nov 21, 2022
Merged

Conversation

dengert
Copy link
Contributor

@dengert dengert commented Nov 19, 2022

The test now work when built in the source or some other directory and configured like ../pkcs11-provider/configure

Fixes: #109

Define "testblddir" and "TESTBLDDIR" to point to the build/test directory i.e. "@abs_top_builddir@/tests" and correct several places where original code made some assumptions that the source directory was the same as the build directory.

tests/test-wrapper was patched to allow a file path name to include "-" It would fail to run some of the tests because it would not find the command an options.

The export PINFILE="${BASEDIR}/${PINFILE}" work when basename was null as previously in code it was already defined as PINFILE="${PWD}/pinfile.txt"

Changes to be committed:
modified: tests/Makefile.am
modified: tests/openssl.cnf.in
modified: tests/setup-softhsm.sh
modified: tests/setup-softokn.sh
modified: tests/test-wrapper

Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent looks good, but I am not sure about some of the substitution in the testvars files.

Also please can you add your signef-off-by to the commit ?

tests/setup-softokn.sh Outdated Show resolved Hide resolved
@simo5
Copy link
Member

simo5 commented Nov 20, 2022

Can you also make me a favor and rename TESTSDIR to TESTSSRCDIR while there?
This way it will be much clearer which dir that one refers to.

@simo5
Copy link
Member

simo5 commented Nov 20, 2022

Oh and the - in the paths is a great catch, I was very confused about what test-wrapper was trying to run ...

@simo5
Copy link
Member

simo5 commented Nov 21, 2022

LGTM, can you please rebase and squash the commits and add your signed-off-by line?

@dengert
Copy link
Contributor Author

dengert commented Nov 21, 2022

both setup-softhsm.sh and setup-softokn.sh

  • create an openssl.cnf file in their respective tests/tmp.softhsm and tests/tmp.softokn directories
  • create the same pinfile in tests
  • Both happen to set the same pin of 12345678

openssl.cnf has a pkcs11-module-token-pin = file:/a/appl/pkcs11-provider-git/pkcs11-provider/tests/pinfile.txt

If they really need a pinfile, I would suggest each create their own pinfile in their respective tests/tmp.softhsm and tests/tmp.softokn directories to avoid future problems if they set different pins. I could see other tests such as a setup-opensc.sh where a real test card is used, which may have a different pin or prompt for the pin.

Is the pkcs11-module-token-pin actually used? Was it just added during early development? pkcs11 URI allows for passing a pin value.

@simo5
Copy link
Member

simo5 commented Nov 21, 2022

both setup-softhsm.sh and setup-softokn.sh

* create an openssl.cnf file in their respective tests/tmp.softhsm and tests/tmp.softokn directories

* create the same pinfile in tests

* Both happen to set the same pin of 12345678

openssl.cnf has a pkcs11-module-token-pin = file:/a/appl/pkcs11-provider-git/pkcs11-provider/tests/pinfile.txt

If they really need a pinfile, I would suggest each create their own pinfile in their respective tests/tmp.softhsm and tests/tmp.softokn directories to avoid future problems if they set different pins. I could see other tests such as a setup-opensc.sh where a real test card is used, which may have a different pin or prompt for the pin.

We should probably also add tests that exercise the pkcs11 URI pin support as well as the configuration one. Mind creating an issue about this?

Is the pkcs11-module-token-pin actually used? Was it just added during early development? pkcs11 URI allows for passing a pin value.

It is totally used, and a key feature to be able to specify the pinfile in the provider configuration rather than passing a naked pin around with keys as it is easier to leak that in log files.

Besides with things like p11-kit it is possible to obtain privilege separation with some applications via the client/server model (you can see it used in the softhsm tests) and keep the pinfile secret from the application, that is not possible if you pass the pinvalue through via pkcs11 URIs.

The test now work when built in the source or some other directory
and configured like ../pkcs11-provider/configure

Define "testsblddir" and "TESTSBLDDIR" to point to the build/tests directory
i.e. "@abs_top_builddir@/tests" and correct several places where
original code made some assumptions that the source directory was the
same as the build directory.

Renamed the "testsdir" and "TESTSDIR" to "testssrcdir" and "TESTSSRCDIR"
to match the "testsblddir" and "TESTSBLDDIR" convention

`tests/test-wrapper` was patched to allow a file path name to include "-"
It would fail to run some of the tests because it would not find the
command an options.

Remove "export PINFILE=..."

Location of PINFILE is not needed outside the setup routines and even
if needed PINVALUE is exported and could be be used. Changes to be committed:

 Changes to be committed:
	modified:   tests/Makefile.am
	modified:   tests/openssl.cnf.in
	modified:   tests/setup-softhsm.sh
	modified:   tests/setup-softokn.sh
	modified:   tests/softhsm-proxy.sh
	modified:   tests/tbasic
	modified:   tests/teccsha2
	modified:   tests/test-wrapper
	modified:   tests/thkdf
	modified:   tests/toaepsha2
	modified:   tests/trsapss

 Date:     Mon Nov 21 14:52:24 2022 -0600

Signed-off-by: Doug Engert <deengert@gmail.com>
@dengert
Copy link
Contributor Author

dengert commented Nov 21, 2022

OK, rebased and signoff

@simo5 simo5 merged commit 167482c into latchset:main Nov 21, 2022
@simo5
Copy link
Member

simo5 commented Nov 21, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests fail when building outside of source
2 participants