Skip login if token does not require login #258
Merged
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
|
I am not sure this is the right place where to fix it. It may make sense to rather ignore login errors and attempt to continue than not attempting login at all. I will form a better opinion if I can see the log. |
|
Hey, thanks for the feedback. Yeah I wasn't sure if this would be the best place to fix it either. You can find the log below at log level 2: Just a few details about this. I actually worked around the initial if (ctx->expect == 0 || ctx->expect == OSSL_STORE_INFO_PKEY
|| login_behavior == PUBKEY_LOGIN_ALWAYS) {
login = true;
}Although no such workaround exists for the signing operation it seems like. With the patch I submitted this workaround wasn't necessary as it would just skip the login altogether. Let me know your thoughts on what might be the best way to solve this issue. |
|
It appears without my patch, it essentially hits this branch and } else {
ret = CKR_CANCEL;
goto done;
} |
|
So what is happening here is that we are entering token_login() but no pin or callback is available, and this causes the function to exit with CKR_CANCEL ... The problem with returning CKR_OK here is that this makes the provider think the session is a login session when it really isn't. Is it normal for TPM modules to not support login at all ? |
|
I'm not sure if it's "normal" but it's certainly one way of using it, since the pkcs11 really just acts as a bridge for the TPM and whatever's trying to use it through a pkcs11 interface. In the same way you wouldn't necessarily enter a password to use the TPM for Windows 11, some may prefer not having a PIN through the pkcs11 interface either for other operations and have it be transparent. I could definitely try to maybe provide an empty PIN or a callback returning an empty PIN the next chance I get and see if that solves my issue. |
|
I see that a Login function is supported by the tpm module you pointed out, and it will be attempted if a pin or a callback function is provided. Perhaps what we can try is to consider the reqlogin variable provide as input into p11prov_get_session() as an advisory request. And check the slot->token to see if the token actually requires login just before calling slot_login(). This can be done by changing the value of reqlogin before checking it this way: where: |
|
We also need to think what the behavior should be if PUBKEY_LOGIN_ALWAYS is set. Probably should always attempt login in that case so a more complex helper function to determine whether to login may be needed. |
|
Right, it does provide Login since there is an option to set a PIN on the slot when creating it via the module, though it is entirely optional. I quite like that idea. Actually I originally wanted to do it from a more "outside" place than I agree here that |
|
Sorry for dropping the ball here. |
|
All good, thanks for reviewing the changes. |
|
@CharlieYJH would you mind squashing the commits, the second basically undo the first IIRC |
…UBKEY_LOGIN_ALWAYS is not set Signed-off-by: Charlie Yin <charlieyinjunhao@hotmail.com>
|
Thanks for the patch! |
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.
Encountered an issue with tokens that don't require login while using this provider with https://github.com/tpm2-software/tpm2-pkcs11 where sign operations would not complete properly and always produced an error due to login error.
This is a proposal to just skip the token login if the flag CKF_LOGIN_REQUIRED is not present.
I'm not sure if there are any side effects to this but it at least solved my issue.