-
Notifications
You must be signed in to change notification settings - Fork 61
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
Added support for C_SetPIN
#49
Added support for C_SetPIN
#49
Conversation
Signed-off-by: Michael J Boquard <[email protected]>
/// Changes the PIN of either the currently logged in user or of the `CKU_USER` if no user is | ||
/// logged in. | ||
pub fn set_pin(&self, old_pin: &str, new_pin: &str) -> Result<()> { | ||
let old_pin = Secret::new(CString::new(old_pin)?.into_bytes()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with the Secret
API but what's the point in wrapping the PIN and then a couple of lines below expose
ing it?
If Secret
is protecting the bytes in memory maybe this function should take a Secret
instead of string slice (thus the user can work with Secrets directly)... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I modeled this after the init_pin
method in Session
and the init_token
method in Pkcs11
. In both of those, the provided PINs are converted into Secret
s and then accessed.
You raise a fair point, though. Maybe @hug-dev can weigh in on why there was a decision to use Secret
in the way it was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point Mike. In this case I think the reasonable approach would be to create an issue about this problem and ignore it temporarily for this PR.
FTR tomorrow is a team telco with Parsec people and we could bring this matter up there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm of the opinion that Secret
isn't necessary for two reasons:
- The calling application still has the
pin
held within it's memory and there's no guarantee that they protected it in any way - Though this is entirely dependent upon the design of the underlying PKCS#11 library, that library may also be holding onto the
pin
in memory, also unprotected. I think it depends on how the library and token implement the session/process coherency requirements of the PKCS#11 spec.
Furthermore, I don't think we need the set_pin
function in Pkcs11
for the same reasons mentioned above.
However, if there are underlying requirements for this functionality, that's understandable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this makes the library simpler to use and doesn't lower the security I'm all for that 👍
Although let's wait for @hug-dev as he probably has more context on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the main reason why we do that here is to zeroize the CString
(allocated in this function) on Drop
, Secret
implementing that by default. Otherwise we would have to do it manually. Maybe Zeroizing
in enough there?
I agree with your point 1 and 2 above, the use of Secret
here is to try to not leak secrets in RAM but the others could still do it :/
Furthermore, I don't think we need the set_pin function in Pkcs11 for the same reasons mentioned above.
Happy to discuss this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calling application still has the pin held within it's memory and there's no guarantee that they protected it in any way
True, if the client application doesn't care then it doesn't matter whether we use Secret
or not, but if they do care and we don't do it, they can't do anything about it (apart from raising a PR/issue and waiting for a new release).
Maybe Zeroizing in enough there?
We could, but it's a pain because you have to make sure, before every ?
, to clear out the memory, so 5 places in total including the final return from the function. And if we change something in the future we might forget to update that too. We could implement our own wrapper around CString
or whatever other type we need and just derive Zeroizing
on that, then we won't need to use expose_secret
everywhere, but the result is probably kinda similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but if they do care and we don't do it, they can't do anything about it (apart from raising a PR/issue and waiting for a new release).
That still leaves the final layer, that being the actual library, and if it's for an actual HSM, that's closed source and you just give up your secrets to the ether.
Maybe Zeroizing in enough there?
I'd say we shouldn't store the pin at all (that is, get rid of the pins
HashMap
in Pkcs11
) and just call as_ptr()
against the pin
str
slice in the concerned functions (C_InitToken
, C_SetPIN
, C_InitPIN
, and C_Login
), that is if that's possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we shouldn't store the pin at all (that is, get rid of the pins HashMap in Pkcs11) and just call as_ptr() against the pin str slice in the concerned functions (C_InitToken, C_SetPIN, C_InitPIN, and C_Login), that is if that's possible.
I am happy to discuss this!! I can't remember now exactly why we choose to do that, but maybe we could merge this PR as it is now (if you are ok with the rest) and open a new issue to expand on those things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! I do think this, and my other PR, #42 will cause a merge conflict, so whichever loses the race I'll fix the conflict and commit it.
I'll open an issue to discuss the PIN handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
Signed-off-by: Michael J Boquard [email protected]