AuthPin forces conversions that can leak secret

[simo5]

I have not experimented much with AuthPin but it is defined as SecretString, instead of just SecretBox, and this force callers that have a Vec into leaking conversion to a String.

Cryptoki never cares for using the pin as a string, all it does is to extract the String pointer as a *mut u8 to pass it to the pkcs11 token.

A better way to deal with AuthPin would be to make a trait and a default implementation.

The default implementation can probably use &str or String, but the trait would allow developers that already have a way to safely manage secret data to just return a slice of [u8] when needed.

[simo5]

I am aware that there is a login_with_raw that takes a "RawAuthPin", but there is no init_token_raw and then again having to use a different api is kind of really annoying, when all of thie can be easily hidden by using a a trait and have the object provide what is needed, regardless of internal representation

[wiktor-k]

Thanks for reporting. I wonder if it would be simpler to just use SecretBox and u8 slices instead of strings and make the caller deal with strings if they need to?

[simo5]

That may be an option, yes, given that SecretString is a SecretBox, so it should be basically drop in compatible

[hug-dev]

I think a reason why we chose SecretString is because the standard defines the pins as UTF8 character strings so that was mapping well with Rust strings which are as well UTF8. The examples in the standard also define pins as string.

force callers that have a Vec into leaking conversion to a String

Is there really no way, maybe unsafe, to convert a Vec to a String without allocating new memory? Maybe with from_raw_parts?

If we decide that using a String is the standard we want to continue on (or Box) then agree that we should only provide one set of functions using the same type but with examples showing the conversions.

[wiktor-k]

Is there really no way, maybe unsafe, to convert a Vec to a String without allocating new memory?

That's an excellent question. The docs of String::from_utf8 state clearly:

This method will take care to not copy the vector, for efficiency’s sake.

I wonder if I'm misunderstanding the use case...

[simo5]

If I already have the PIN in my Vector that means copying the PIN (unless it is ok to give it away), now you have the PIN twice in memory.

If SecretBox is used, you can still use SecretString to import stuff were you want, but you can also use &[u8] which will prevent copies in memory.

This is not a huge deal, but it'd be nice in general for cryptoki to avoid allocations whenever they are not necessary (same goes for a lot of buffers that are now forcibly allocated instead of accepting pre-allocated slices of the right size form the application at least as an Option()).

[hug-dev]

copying the PIN

When? If you use String::from_utf8 then you would not reallocate the data. Note that into_boxed_str may re-allocate data (I think if the capacity is different from the length) but I would say it's the same as if you would use into_boxed_slice from a Vec<u8>?

use cryptoki::types::AuthPin;

fn main() {
    let pin: Vec<u8> = vec![45, 54, 54, 24, 23];
    let pin_str = String::from_utf8(pin).unwrap();
    let pin_box = pin_str.into_boxed_str();
    let pin = AuthPin::new(pin_box);
    println!("{:?}", pin);
}

[simo5]

copying the PIN

When?

If I have a the pin in a Vector (say in a config structure, then the only way to use from_utf8 is to String::from_utf8(myconfig.pin.clone()) or String::from_utf8(myconfig.pin.to_vec()) depending on how pin is stored in the config. In both cases we are duplicating memory.

I may be able to just "give" the vector to from_utf8() in some situation, but that is not always the case, it depends by many other factors in non-trivial applications. Code examples are not always useful to explore how interfaces are used in real life because they make simplistic assumptions.

[wiktor-k]

Okay, then it seems taking ownership is the issue and since all these login functions immediately use their argument I wonder if we could use a ref here, e.g. Option<impl AsRef<[u8]>> for pin which should automatically work for strings (that implement this). This would make the caller responsible for securely managing their secrets and pass us raw bytes only when stictly necessary.

Sadly I can't check this right now but I'm glad we're thinking about this. 👌

Edit: alternative, simpler solution, is just using Option<&str>

[hug-dev]

Okay, then it seems taking ownership is the issue and since all these login functions immediately use their argument I wonder if we could use a ref here, e.g. Option<impl AsRef<[u8]>> for pin which should automatically work for strings (that implement this).

Agree!! Because even when using SecretBox ownership would need to be passed from the config (in your above example) to the constructor of the SecretBox, unless I am missing something. So a clone would be needed somewhere unless the pin is made to be moved from the start (maybe if it's a SecretBox from the start, as in also in the config?).