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

Do not require a R/W session for --login, ... #2579

Merged
merged 5 commits into from
Aug 9, 2022

Conversation

ulrichb
Copy link
Contributor

@ulrichb ulrichb commented Jul 18, 2022

This allows to perform commands like --login --pin XXX --list-objects or --sign ... on a read-only token (which otherwise could return an CKR_TOKEN_WRITE_PROTECTED error when calling C_OpenSession()).

Fixes #2182.

Open questions

Checklist

  • [n/a] Documentation is added or updated
  • [n/a] New files have a LGPL 2.1 license statement
  • PKCS#11 module is tested
    • With our SignPath PKCS#11 Cryptoprovider
  • [n/a] Windows minidriver is tested
  • [n/a] macOS tokend is tested

@Jakuje
Copy link
Member

Jakuje commented Aug 1, 2022

I would be for creating a new switch --session-rw and trying to use RO session for at least the three operations you mentioned to 1) have a way back if the pkcs11 module does not work without the RW 2) try to use least possible permissions by default.

This allows to perform commands like `--login --pin XXX --list-objects` or `--sign ...` on a read-only token (which otherwise could return an  `CKR_TOKEN_WRITE_PROTECTED` error when calling `C_OpenSession()`).

Fixes OpenSC#2182.
@ulrichb
Copy link
Contributor Author

ulrichb commented Aug 2, 2022

@Jakuje Both done.

@ulrichb
Copy link
Contributor Author

ulrichb commented Aug 2, 2022

@Jakuje Now ... also change to NEED_SESSION_RO for:

  • --wrap
  • --unwrap
  • --test
    ?

@popovec
Copy link
Member

popovec commented Aug 2, 2022

The unwrap operation is used to store wrapped key to card. For this operation RW session is necessary.

@ulrichb
Copy link
Contributor Author

ulrichb commented Aug 2, 2022

@popovec Okay, then consequently also --test because it also does a "unwrap" operation. What about --wrap?

@popovec
Copy link
Member

popovec commented Aug 2, 2022

WRAP: Wrapped key is returned to user. No writing to card occurs during the "wrap" operation.

--test - The wrap and unwrap test is incorrect (#1796). I assume that wrap and unwrap will be blocked in the next opensc release as well. (https://github.com/OpenSC/OpenSC/blob/master/src/tools/pkcs11-tool.c#L6657)

@ulrichb
Copy link
Contributor Author

ulrichb commented Aug 2, 2022

@popovec Okay, also changed --wrap to RO, and left --test with RW as it will need unwrap after #1796.

@Jakuje
Copy link
Member

Jakuje commented Aug 3, 2022

This looks like it breaks the tests in https://github.com/OpenSC/OpenSC/runs/7646167936?check_suite_focus=true

If I read the output right, its the FAIL: test-pkcs11-tool-sign-verify.sh and FAIL test-pkcs11-tool-unwrap-wrap-test.sh, but I did not check details. The test runs some pkcs11-tool operations against softhsm which might (wrongly?) require the RW session for some of the operations (or it already fails during setup?). Can you check locally?

@popovec
Copy link
Member

popovec commented Aug 3, 2022

This looks like it breaks the tests in https://github.com/OpenSC/OpenSC/runs/7646167936?check_suite_focus=true

If I read the output right, its the FAIL: test-pkcs11-tool-sign-verify.sh and FAIL test-pkcs11-tool-unwrap-wrap-test.sh, but I did not check details. The test runs some pkcs11-tool operations against softhsm which might (wrongly?) require the RW session for some of the operations (or it already fails during setup?). Can you check locally?

Interesting, only build-openssl-3 is falling, same tests in build-libressl and build-clang-tidy went without errors.

@ulrichb
Copy link
Contributor Author

ulrichb commented Aug 3, 2022

Hmm, ... cannot reproduce the failing test, ... ran make check on Ubuntu 22.04 with OpenSSL 3.0.2 with the following result.

PASS: test-pkcs11-tool-sign-verify.sh
XFAIL: test-pkcs11-tool-test.sh
XFAIL: test-pkcs11-tool-test-threads.sh
PASS: test-pkcs11-tool-allowed-mechanisms.sh
PASS: test-pkcs11-tool-sym-crypt-test.sh
PASS: test-pkcs11-tool-unwrap-wrap-test.sh

@ulrichb
Copy link
Contributor Author

ulrichb commented Aug 3, 2022

... can we somehow get the test-pkcs11-tool-sign-verify.sh.log from the CI run?

@ulrichb
Copy link
Contributor Author

ulrichb commented Aug 3, 2022

... one suspicion I have, is that we use openssl rsautl and openssl dgst for verification within test-pkcs11-tool-sign-verify.sh. Maybe this broke the verification part (with OpenSSL 3), not the actual pkcs11-tool part.

Maybe related: I get in my local run "The command rsautl was deprecated in version 3.0. Use 'pkeyutl' instead" warnings (although the test passes).

@ulrichb
Copy link
Contributor Author

ulrichb commented Aug 4, 2022

@Jakuje Could you maybe trigger a build on master to get a "baseline"?

@Jakuje
Copy link
Member

Jakuje commented Aug 4, 2022

@Jakuje Could you maybe trigger a build on master to get a "baseline"?

We have build from yesterday on master which worked just ok, which makes me thinking that it has to be something else:

https://github.com/OpenSC/OpenSC/runs/7649380818?check_suite_focus=true

Actually, there are logs uploaded to github as artifacts in https://github.com/OpenSC/OpenSC/actions/runs/2784469746 but the naming is not great (and the upload is skipped because of the build failed, which makes it useless). I just pushed a change to your branch which shold upload the logs.

@Jakuje
Copy link
Member

Jakuje commented Aug 4, 2022

Hmm. Now it works so it was probably just some intermittent issue, maybe bad openssl build ...

@ulrichb
Copy link
Contributor Author

ulrichb commented Aug 4, 2022

Oh. Many thanks @Jakuje !

@ulrichb
Copy link
Contributor Author

ulrichb commented Aug 8, 2022

@popovec Ready to merge?

@Jakuje Jakuje merged commit e84cf49 into OpenSC:master Aug 9, 2022
@Jakuje
Copy link
Member

Jakuje commented Aug 9, 2022

Thank you for your contribution!

@ulrichb
Copy link
Contributor Author

ulrichb commented Aug 9, 2022

Thanks for the review and for merging!

@ulrichb ulrichb deleted the login-no-rw-session branch August 9, 2022 11:34
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.

pkcs11-tool '--login' option always tries to create a read/write session
3 participants