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

Clarify how capabilities negotiation works in Safe Extensions framework. #31

Closed
wants to merge 8 commits into from

Conversation

rohanmahy
Copy link
Contributor

@rohanmahy rohanmahy commented Oct 4, 2024

Note: this diff is easier to read when viewed as diffs for its separate commits. The first Commit is just a plain move of one section.

  • Moved the text in "Storing State in Extensions" earlier and renamed
  • describe the original "ExtensionTypes" as "Core struct extensions"
  • added a section on capabilities handling and negotiation
  • instruct IANA to add a "Safe" column to "MLS Extension Types"
  • instruct IANA that a safe extension can have N/A in the Message(s) column.

@rohanmahy rohanmahy marked this pull request as ready for review October 5, 2024 03:35
Copy link
Contributor

@kkohbrok kkohbrok left a comment

Choose a reason for hiding this comment

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

I hadn't realized that we were missing the section on extension negotiation, so thanks for adding it.

Is there a reason you want the extension WireFormat, Proposal, etc. to be part of the safe extension API? Our thinking was to keep it separate, because it might also be useful for non-safe extensions.

@rohanmahy
Copy link
Contributor Author

Is there a reason you want the extension WireFormat, Proposal, etc. to be part of the safe extension API?

Yes! Safe WireFormats, Safe Proposals, and Safe Credentials share the mls-extension_message, extension*proposal, and extension_credential types respectively; all of these MUST use the ExtensionContent struct; and all of them use the capabilities negotiation I described. The whole reason I wrote this PR was because it was simply not possible to write rules for safe extension of one these types without stepping into rules that should be defined in the framework.

Our thinking was to keep it separate, because it [my emphasis] might also be useful for non-safe extensions.

I'm not sure what the it is. RFC9420 already says what you need to do for non-safe extensions of these types. If the creator of a non-safe extension wants to reuse some structs defined the Safe Extensions framework, I don't see any problem with this, but they absolutely MUST NOT use the IANA registered mls-extension_message, extension*proposal, and extension_credential types. If you wanted to make it easier to find/reference the ExtensionState struct for example, I would suggest we get this PR merged and then we can have a clean PR proposing the move. Otherwise the conflicts get ugly and the PR is harder to review.

@kkohbrok
Copy link
Contributor

What I meant was: Why shouldn't unsafe extension authors make use of mls-extension_message, extension*proposal and extension_credential? Is there a safety or a negotiation consideration I'm not seeing?

@rohanmahy
Copy link
Contributor Author

rohanmahy commented Oct 14, 2024 via email

@kkohbrok
Copy link
Contributor

If the other client doesn't know the extension, how can its semantics (and thus its safety) be relevant to it?

@rohanmahy
Copy link
Contributor Author

rohanmahy commented Oct 14, 2024 via email

@kkohbrok
Copy link
Contributor

  1. If an extension can't be safely ignored, it should be a required capability, shouldn't it?
  2. If the application wants to do anything with the extension, why wouldn't it signal support for it? Also, why would an application expect to be shown the data for safe extensions, but not for unsafe ones?

@rohanmahy
Copy link
Contributor Author

rohanmahy commented Oct 15, 2024 via email

@kkohbrok
Copy link
Contributor

In a scenario where the MLS implementation doesn't know the extension, i.e. it's not in the capabilities then why wouldn't the application show proposals, etc. to the application regardless whether the extension is safe or not? If the implementation of the unknown extension can't be delegated to the application, the extension really should have been in the required capabilities.

@rohanmahy
Copy link
Contributor Author

In a scenario where the MLS implementation doesn't know the extension, i.e. it's not in the capabilities then why wouldn't the application show proposals, etc. to the application regardless whether the extension is safe or not? If the implementation of the unknown extension can't be delegated to the application, the extension really should have been in the required capabilities.

Let's take the example of an AssociatedParties "feature". It has:

  • one GroupContext
  • 3 Proposals (adding, removing, and updating this GC)
  • A label for a new secret

The "feature" has a single ExtensionType say it is type 0xBEEF. The proposals are extension_proposal, extension_path_proposal, and extension_path_proposal types respectively. After each of these would be the ExtensionType 0xBEEF and then inside the extension_path_proposal, a selector in the TLS struct to select between the remove and update variants of the struct.

@kkohbrok
Copy link
Contributor

I see what you're getting at. I think this an issue beyond safe extensions that we best discuss in the next meeting.

@rohanmahy
Copy link
Contributor Author

I see what you're getting at. I think this an issue beyond safe extensions that we best discuss in the next meeting.

I think the group will have a hard time reviewing it properly inside a PR. If we have consensus from the editors can we merge this to have something concrete to discuss?

@kkohbrok
Copy link
Contributor

Looking at it more closely, maybe I don't understand what you're getting at. I understand the AP extension example and that's indeed how the mechanism is intended, but why can't we use the same mechanism (i.e. bundle multiple proposals, WireFormats, etc. behind a single extension type) with a non-safe extension?

@rohanmahy
Copy link
Contributor Author

Looking at it more closely, maybe I don't understand what you're getting at. I understand the AP extension example and that's indeed how the mechanism is intended, but why can't we use the same mechanism (i.e. bundle multiple proposals, WireFormats, etc. behind a single extension type) with a non-safe extension?

I think we should be making it clear and easy to make safe extensions. The creator of a non-safe extension could use the same or similar mechanism, but we can't be prescriptive about what non-safe extension can and can't do. They don't HAVE TO. However for safe extensions we can say, they have to do a specific thing. That's why I don't particularly care to spend a lot of energy on considerations for non-safe extensions. It's not that I don't want non-safe extensions to do better, it's that they are free to ignore anything we do say now (the horse is already out of the barn).

@kkohbrok
Copy link
Contributor

Fair enough, then I'm okay with this PR.

I don't think, however, that just because an extension is safe, it can be safely ignored by both client and application. For example, if a client receives a commit with an extension proposal from a safe extension where the proposal is invalid according to the extension's semantics. A client that is knows the extension will consider the commit invalid. A client that doesn't will consider the commit invalid.

@rohanmahy
Copy link
Contributor Author

Fair enough, then I'm okay with this PR.

Great!

I don't think, however, that just because an extension is safe, it can be safely ignored by both client and application. For example, if a client receives a commit with an extension proposal from a safe extension where the proposal is invalid according to the extension's semantics. A client that is knows the extension will consider the commit invalid. A client that doesn't will consider the commit invalid.

Agreed. All proposals used would have to be in required_capabilities because a member needs to be able to understand if they are valid or not.

@raphaelrobert
Copy link
Member

Closing in favor of #36.

@rohanmahy rohanmahy deleted the ext-types branch January 31, 2025 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants