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

Consistency between "optional that MUST contain a value when …" and selects #574

Closed
TWal opened this issue Feb 2, 2022 · 7 comments
Closed

Comments

@TWal
Copy link
Contributor

@TWal TWal commented Feb 2, 2022

In the draft, we often need some value to be present in a structure under certain conditions.

Sometimes, it is done using an optional<…>, and the prose says that it MUST contain a value under these conditions, e.g. for:

  • confirmation_tag
  • membership_tag

Sometimes, it is done using a select, e.g. for:

  • GroupContext in MLSPlaintextTBS

It looks like we need to make a choice here.

@bifurcation
Copy link
Collaborator

@bifurcation bifurcation commented Feb 2, 2022

It seems like the select pattern only works when you have an indicator to use as selector, as opposed to some broader context. For confirmation_tag and membership_tag, you might be able to use sender.type, but it seems a bit messy.

@bifurcation
Copy link
Collaborator

@bifurcation bifurcation commented Feb 3, 2022

Discussion on virtual interim:

  • @TWal elaborates:
    • For confirmation_tag could select on content_type
    • For membership_tag could select on sender.type
  • @bifurcation agrees that failing on parsing is a pretty compelling rationale for these cases

@bifurcation
Copy link
Collaborator

@bifurcation bifurcation commented Feb 7, 2022

I just looked at implementing this on top of #523, and it actually looked a little non-trivial. If we use select in MLSMessageAuth, then we'll need to know the content_type from elsewhere when parsing the MLSMessageAuth.

struct {
    // SignWithLabel(., "MLSMessageContentTBS", MLSMessageContentTBS)
    opaque signature<0..2^16-1>;

    select (content_type) { // <---- where does this come from?
        case application: struct{};
        case proposal: struct{};

        // MAC(confirmation_key, GroupContext.confirmed_transcript_hash)
        case commit: MAC confirmation_tag;
    }
} MLSMessageAuth;

That might not be terrible to implement, but it does mean that MLSMessageAuth is no longer independently parseable.

No similar concerns about membership_tag -- that's within MLSPlaintext, so you can refer to content.sender.sender_type.

struct {
    MLSMessageContent content;
    MLSMessageAuth auth;

    select (content.sender.sender_type) {
        case preconfigured: struct{};
        case new_member: struct{};

        // MAC(membership_key, MLSMessageContentTBM)
        case member: MAC membership_tag;
    }
} MLSPlaintext;

@TWal
Copy link
Contributor Author

@TWal TWal commented Feb 7, 2022

You are right… We maybe we could template MLSMessageAuth on the ContentType?

Something like

template <ContentType content_type>
struct {
    // SignWithLabel(., "MLSMessageContentTBS", MLSMessageContentTBS)
    opaque signature<0..2^16-1>;

    select (content_type) {
        case application: struct{};
        case proposal: struct{};

        // MAC(confirmation_key, GroupContext.confirmed_transcript_hash)
        case commit: MAC confirmation_tag;
    }
} MLSMessageAuth;

and

struct {
    MLSMessageContent content;
    MLSMessageAuth<content.content_type> auth;

    select (content.sender.sender_type) {
        case preconfigured: struct{};
        case new_member: struct{};

        // MAC(membership_key, MLSMessageContentTBM)
        case member: MAC membership_tag;
    }
} MLSPlaintext;

But that might look a bit convoluted…

@TWal
Copy link
Contributor Author

@TWal TWal commented Feb 7, 2022

Note that a structure with a similar problem already exists: MLSCiphertextContent.

With the same style, we could have:

struct {
    // SignWithLabel(., "MLSMessageContentTBS", MLSMessageContentTBS)
    opaque signature<0..2^16-1>;

    select (MLSMessageContent.content_type) {
        case application: struct{};
        case proposal: struct{};

        // MAC(confirmation_key, GroupContext.confirmed_transcript_hash)
        case commit: MAC confirmation_tag;
    }
} MLSMessageAuth;

@TWal
Copy link
Contributor Author

@TWal TWal commented Feb 8, 2022

Note that independently of #523 we also have a similar problem for MLSPlaintextCommitAuthData.

@TWal
Copy link
Contributor Author

@TWal TWal commented Feb 15, 2022

Fixed in #523

@TWal TWal closed this Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants