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

Block resumption psks #817

Merged
merged 7 commits into from
Jun 10, 2024
Merged

Block resumption psks #817

merged 7 commits into from
Jun 10, 2024

Conversation

nplasterer
Copy link
Contributor

@nplasterer nplasterer commented Jun 6, 2024

Closes #678

https://www.rfc-editor.org/rfc/rfc9420.html#section-12.1
https://openmls.tech/openmls/doc/openmls/schedule/psk/enum.Psk.html

  • Block any ReInit proposal
  • Block any Welcome with a PreSharedKeyID of type resumption with usage reinit

@nplasterer nplasterer self-assigned this Jun 6, 2024
@nplasterer
Copy link
Contributor Author

@franziskuskiefer do you mind taking a look at this? I'm not sure if this is correct way to block psks or if there are different functions I should be using.

@@ -326,6 +328,9 @@ impl MlsGroup {
let welcome = deserialize_welcome(&welcome_bytes)?;

let join_config = build_group_join_config();
if join_config.number_of_resumption_psks > 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just noting I had to make this public so I could use it xmtp/openmls#30

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this into a getter instead of making it public? Then we can upstream this change. (I'd just prefer not making the fields pub directly.)

Copy link
Contributor

@franziskuskiefer franziskuskiefer Jun 6, 2024

Choose a reason for hiding this comment

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

This isn't doing what you think it's doing I'm afraid. This only prevents you from joining a group that allow resumption PSKs. It's a property you probably want to set when creating groups to avoid generating unused resumption PSKs.

But to achieve what you want, you need to look into the group secrets. That's actually not possible yet. But it is with openmls/openmls#1586. I just added a function there to get the psks. On the result from that function you can get the processed_welcome.psks().psk(), which returns a PSK enum. There you can error out when it contains a resumption psk (or any, depending on what you want to achieve).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled in your PR to my fork and I seem to be able to get the psks now. 🙏

Copy link
Contributor

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

The proposal check looks good. The other one needs to be different I think (see comment).

@@ -326,6 +328,9 @@ impl MlsGroup {
let welcome = deserialize_welcome(&welcome_bytes)?;

let join_config = build_group_join_config();
if join_config.number_of_resumption_psks > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this into a getter instead of making it public? Then we can upstream this change. (I'd just prefer not making the fields pub directly.)

@@ -326,6 +328,9 @@ impl MlsGroup {
let welcome = deserialize_welcome(&welcome_bytes)?;

let join_config = build_group_join_config();
if join_config.number_of_resumption_psks > 0 {
Copy link
Contributor

@franziskuskiefer franziskuskiefer Jun 6, 2024

Choose a reason for hiding this comment

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

This isn't doing what you think it's doing I'm afraid. This only prevents you from joining a group that allow resumption PSKs. It's a property you probably want to set when creating groups to avoid generating unused resumption PSKs.

But to achieve what you want, you need to look into the group secrets. That's actually not possible yet. But it is with openmls/openmls#1586. I just added a function there to get the psks. On the result from that function you can get the processed_welcome.psks().psk(), which returns a PSK enum. There you can error out when it contains a resumption psk (or any, depending on what you want to achieve).

Comment on lines 336 to 338
if processed_welcome.psks().is_empty() {
return Err(GroupError::NoPSKSupport);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may eventually want to allow external psks depending on how we want to do conversation history. But in the meantime I'm okay with blocking both resumption and external psks.

@nplasterer nplasterer marked this pull request as ready for review June 7, 2024 04:53
@nplasterer nplasterer requested a review from a team as a code owner June 7, 2024 04:53
Copy link
Contributor

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

lgtm!

xmtp_mls/src/groups/validated_commit.rs Show resolved Hide resolved
xmtp_mls/src/groups/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@richardhuaaa richardhuaaa left a comment

Choose a reason for hiding this comment

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

I've looked at the streaming test failure, and the error message seems unusual (Storage(NotFound)). Given other tests pass, I don't think it's related to your changes. Feel free to ignore/land, I'll look at it separately

@nplasterer nplasterer merged commit 6e8dcd1 into main Jun 10, 2024
4 checks passed
@nplasterer nplasterer deleted the np/block-psks branch June 10, 2024 18:27
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.

Block resumption PSK from being used
4 participants