Skip to content
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

Pre-share key test #138

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Pre-share key test #138

wants to merge 1 commit into from

Conversation

cmester0
Copy link
Collaborator

@cmester0 cmester0 commented Jan 27, 2025

This PR addresses running bertie with PSK.

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • Automation

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

  • I have linked an issue to this PR
  • I have described the changes
  • I have read and understood the code of conduct, contribution guidelines, and contributor license agreement
  • I have added tests for all changes
  • I have tested that all tests pass locally and all checks pass
  • I have updated documentation if necessary

Fixes #

@cmester0 cmester0 requested a review from a team as a code owner January 27, 2025 15:04
@cmester0 cmester0 force-pushed the psk_tests branch 3 times, most recently from f4d0db9 to f34f01b Compare January 27, 2025 15:09
Copy link
Contributor

@jschneider-bensch jschneider-bensch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -190,7 +191,7 @@ fn check_psk_shared_key(algs: &Algorithms, ch: &[U8]) -> Result<(), TLSError> {
if ch.len() - 5 - len_id != algs.hash().hash_len() {
tlserr(parse_failed())
} else {
Ok(())
Ok((Bytes::from(&ch[4..4+len_tkt]), Bytes::from([0; 0])))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This always returns a zero-length binder. Is that correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -602,6 +602,7 @@ fn process_psk_binder_zero_rtt(
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)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you recomputing the binder here? This means the HMAC verification on the next line will never fail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants