Conversation
cmester0
force-pushed
the
psk_tests
branch
4 times, most recently
from
f34f01b to
c736bd3
Compare
jschneider-bensch
left a comment
jschneider-bensch
left a comment
There was a problem hiding this comment.
Thanks, for adding the test, Lasse!
PSK modes are not well supported in Bertie at the moment, as you also saw.
I have some questions below, though.
| tlserr(parse_failed()) | ||
| } else { | ||
| Ok(()) | ||
| Ok((Bytes::from(&ch[4..4+len_tkt]), Bytes::from([0; 0]))) |
There was a problem hiding this comment.
This always returns a zero-length binder. Is that correct?
There was a problem hiding this comment.
No, this is probably not correct. However, it does set the value of the binder, which is then replaced later. So the binder does have the correct value (I think) after processing the client hello. The update should probably be delayed to the point, where we have the binder value, instead of this.
| match (ciphersuite.psk_mode, psko, bindero) { | ||
| (true, Some(k), Some(binder)) => { | ||
| let mk = derive_binder_key(&ciphersuite.hash, k)?; | ||
| let binder = hmac_tag(&ciphersuite.hash, &mk, &th_trunc)?; |
There was a problem hiding this comment.
Why are you recomputing the binder here? This means the HMAC verification on the next line will never fail.
There was a problem hiding this comment.
The binder value seemed to be incorrect from the client side, as the value was never tagged. I could not find the correct place to do this call, but you are correct that this will never fail. We should probably add some tests to catch this kind of error.
3 participants
This PR addresses running bertie with PSK.
Type of change
Motivation and Context
Test for running Bertie with PSK failed, and these changes makes the test of PSK parse.
Changes
The changes are primarily formatting regarding pre-shared keys.
Checklist
Fixes #