Add PSK support #205
Add PSK support #205
Conversation
dongcarl
force-pushed
the
2022-07-add-psk
branch
from
29a5eda to
f95e61d
Compare
|
I don't think that the |
|
Ping @raoulstrackx @Pagten |
| let cb = &mut *(closure as *mut F); | ||
| let ctx = UnsafeFrom::from(ctx).unwrap(); | ||
|
|
||
| let psk_identity = std::str::from_utf8_unchecked(from_raw_parts(psk_identity, identity_len)); |
There was a problem hiding this comment.
There doesn't seem to be a guarantee that psk_identity is always a valid utf8 string.
There was a problem hiding this comment.
What's the best thing to do here? unwrap a checked from_utf8 so the error is more apparent?
There was a problem hiding this comment.
No you could change the type of PskCallback to
callback!(PskCallback: Fn(&mut HandshakeContext, &[u8]) -> Result<()>);
so there isn't a need to convert it.
mbedtls/tests/ssl_conf_psk_cb.rs
Outdated
| ctx.establish(conn, None).map(|_| ()) | ||
| } | ||
|
|
||
| #[cfg(unix)] |
There was a problem hiding this comment.
Why is this restricted to unix-like systems? This should also run correctly on for example x86_64-fortanix-unknown-sgx
mbedtls/tests/ssl_conf_psk_cb.rs
Outdated
| @@ -0,0 +1,61 @@ | |||
| #![allow(dead_code)] | |||
|
I also fixed CI in #206, so could you rebase on top of that one? |
dongcarl
force-pushed
the
2022-07-add-psk
branch
from
f95e61d to
8162ee4
Compare
|
@raoulstrackx let me know about #205 (comment) |
dongcarl
force-pushed
the
2022-07-add-psk
branch
2 times, most recently
from
6a926c9 to
09650b3
Compare
Co-authored-by: Natnatee Dokmai <[email protected]>
dongcarl
force-pushed
the
2022-07-add-psk
branch
from
09650b3 to
b71ec09
Compare
After running the CI, we see that we actually do need some kind of attribute guard for this test because There are more than one way to do the attribute guard that can be seen in existing tests:
I ended up using a module level Ping @raoulstrackx |
| let cb = &mut *(closure as *mut F); | ||
| let ctx = UnsafeFrom::from(ctx).unwrap(); | ||
|
|
||
| let psk_identity = std::str::from_utf8_unchecked(from_raw_parts(psk_identity, identity_len)); |
There was a problem hiding this comment.
No you could change the type of PskCallback to
callback!(PskCallback: Fn(&mut HandshakeContext, &[u8]) -> Result<()>);
so there isn't a need to convert it.
|
|
||
| pub const PRESHARED_KEY: &'static [u8] = &[ | ||
| 234, 206, 151, 23, 219, 21, 71, 144, | ||
| 107, 42, 23, 67, 249, 173, 182, 224 ]; |
There was a problem hiding this comment.
Could you pick a preshared key that is not valid utf8 and also contains a zero byte? It may trigger some weird code paths.
There was a problem hiding this comment.
Also, could we add a case with a leading zero byte? I'm certain it is correctly handled, but I have seen way too many leading zero bugs in the wild/
(Orthogonal but also see https://mbed-tls.readthedocs.io/en/latest/security-advisories/advisories/mbedtls-security-advisory-2020-09-3/).
There was a problem hiding this comment.
Huh! If it's not valid UTF-8, we should expect an error from from_utf8?
What kind of behaviour do we expect for leading zero bytes?
There was a problem hiding this comment.
Why is there a requirement that pre-shared keys are valid UTF-8? At least in RFC4279, only PSK identities are assumed to be UTF-8 (Sec. 5.1). Also Sec. 4 states that PSK are the result of Diffie Hellman computations, so no UTF-8 expected.
Oh I thought CI for @zugzwang could you also take a look when these two small comments of mine are addressed? |
|
In the meantime we decided to merge #202 since it also added DTLS support. |
|
Thank you for contribution. |
A rebased and cleaned up version of #107