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

Test kryoptic with NSS DB #487

Merged
merged 2 commits into from
Dec 17, 2024
Merged

Test kryoptic with NSS DB #487

merged 2 commits into from
Dec 17, 2024

Conversation

Jakuje
Copy link
Contributor

@Jakuje Jakuje commented Dec 12, 2024

Description

This is more value for the kryoptic to make sure all the operations on the newly introduced DB work as expected.

The NSSDB backend is now super-slow so I am bumping the timeout to 60s/test. The kryoptic build is also adjusted to include this non-default feature.

This change depends on kryoptic including one last fix to provide CKF_USER_PIN_INITIALIZED flag from NSS DB backend.

Checklist

  • Test suite updated with functionality tests

Reviewer's checklist:

  • Any issues marked for closing are addressed
  • There is a test suite reasonably covering new functionality or modifications
  • This feature/change has adequate documentation added
  • Code conform to coding style that today cannot yet be enforced via the check style test
  • Commits have short titles and sensible commit messages
  • Coverity Scan has run if needed (code PR) and no new defects were found


# this overrides what we define in the generic init
# the NSS DB can not store custom labels
export TOKENLABEL="Kryoptic Soft Token"
Copy link
Member

Choose a reason for hiding this comment

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

You can pass in some of these labels via configuration like you did for the passwordRequired Flags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make sure that we will really work with the NSS DB so using different one than the SQL DB. I am not sure if these particular knobs are used, but I can double-check

Copy link
Member

Choose a reason for hiding this comment

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

Tokenlabel is needed by opensc pkcs11-tool to be able to find the right slot/token for some of the operations.
I also do not think they should be identical to the std kryoptic run, I would see it specific like "Kryoptic NSSDB Token"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention was to keep the most of the options default.

The flags='passwordRequired' is needed for gnutls p11tool to attempt to login when searching for private keys. Without that, it skips the login and fails. It might make sense to describe this in the comment in the script rather than here though.

@simo5
Copy link
Member

simo5 commented Dec 12, 2024

It'd be nice to make the kryoptic.nss text somehow optional and not enabled by default when we run make check, I generally want to test with the KRYOPTIC env var set, but I do not want to always try to run the nssdb tests until their perf improves at least.

The NSSDB backend is now super-slow so I am bumping the timeout to
60s/test. The kryoptic build is also adjusted to include this
non-default feature.

Signed-off-by: Jakub Jelen <[email protected]>
At random times, the uri test was failing with some error like this:

```
openssl storeutl -text "$uri"
Couldn't open file or uri URI7xnNajqYJnkZTktp42UM2yp38/G8hCR7QWsnuREHRXLaVoqVJouteOg==
40A7BAF0F27F0000:error:16000069:STORE routines:ossl_store_get0_loader_int:unregistered scheme:../crypto/store/store_register.c:237:scheme=file
40A7BAF0F27F0000:error:80000002:system library:file_open:No such file or directory:../providers/implementations/storemgmt/file_store.c:267:calling stat(URI7xnNajqYJnkZTktp42UM2yp38/G8hCR7QWsnuREHRXLaVoqVJouteOg==)
```

Signed-off-by: Jakub Jelen <[email protected]>
@Jakuje Jakuje marked this pull request as ready for review December 17, 2024 10:23
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.

LGTM,
still I would have expected some better performance, we'll have to do some profiling running the tests on the same host for proper comparison

@simo5 simo5 added the covscan-ok Coverity scan passed label Dec 17, 2024
@simo5
Copy link
Member

simo5 commented Dec 17, 2024

Covscan not needed.

@simo5 simo5 merged commit 6be8f7a into latchset:main Dec 17, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
covscan-ok Coverity scan passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants