Skip to content

Conversation

@bukka
Copy link
Contributor

@bukka bukka commented Aug 10, 2025

Description

This is a fix for #610 to require login if search associated object class is CKO_PRIVATE_KEY

I was looking into implementing a test but it seems quite tricky to create a key pair where only private key has the mechanisms attribute set but public key doesn't - I'm not sure if that's even possible using pkcs11-tool?

Alternativelly I could just do a grep from the debug log to see if there is that reported error which should be relatively simple.

Or if you are fine to get this merged without test, then it's ready as it is.

Checklist

  • Test suite updated with functionality tests
  • Test suite updated with negative 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

@bukka bukka marked this pull request as draft August 10, 2025 21:05
@bukka
Copy link
Contributor Author

bukka commented Aug 10, 2025

Hmm, it's actually failing the tests as some of them don't have default PIN set and this is being checked. It might need a bit more considering as it could potentially cause issues.

@Jakuje
Copy link
Contributor

Jakuje commented Aug 19, 2025

The test failure is in the regression test, which I added in #549:

https://github.com/latchset/pkcs11-provider/blob/379c2bf19d8953027f5c187dcd230a15324f811f/tests/tbasic#L85C22-L85C77

## Test prompting without PIN in config files

## Make sure we are not prompted for pin to read public RSA key
Enter PIN for PKCS#11 Token (Slot 1832861764 - SoftHSM slot ID 0x6d3f4044):Unexpected pin prompt received!
==============================================================================

Requiring login for the public key is something we do not want as there are dozens of use cases where the user just wants to pull the public key and log in only afterward.

Until last year, the pkcs11-tool was setting the ALLOWED_MECHANISMS only on private keys so you could simulate it by running the older pkcs11-tool or on older distribution: OpenSC/OpenSC@5d15e33

These days, I hope this will be less frequent, but even some tokens might not support storing this attribute on public key object then or we want to support running the pkcs11-provider with older systems.

That said, I would say that the fix for #610 should be to make the error less noisy, rather than trying to log in.

@bukka
Copy link
Contributor Author

bukka commented Sep 13, 2025

I think it might make sense to try to login if it's possible but not try to ask for PIN. I think the whole logic does not make much sense otherwise - I mean what private keys would you expect to get in session without login?

@Jakuje
Copy link
Contributor

Jakuje commented Sep 14, 2025

I think it might make sense to try to login if it's possible but not try to ask for PIN. I think the whole logic does not make much sense otherwise - I mean what private keys would you expect to get in session without login?

Logging in is asking for a pin (unless there is a long-running process that logged in before.

Querying for public keys should never log in (=prompt for pin). If it does either, it depends on the order of operations, which makes the behavior even more confusing. For example if you first login (for some different object) and then query for the public key, you are getting different OpenSSL Key type (RSA x RSA-PSS), which causes super-confusing outcome.

Public keys (and certs) are public information so they should be readable without the need to authenticate (at least in the context of smart cards and tokens).

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.

2 participants