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

Switch to MLS #16

Merged
merged 44 commits into from
Jul 15, 2023
Merged

Switch to MLS #16

merged 44 commits into from
Jul 15, 2023

Conversation

expede
Copy link
Member

@expede expede commented Apr 15, 2023

👀 Preview

Changelog

Raw Tech

  • Switch to X25519 ECDH
    • Include rationale vs WebCrypto P-256 in FAQ
    • Add implementation recommendation for non-extractable keys
  • Leave in steps up to 3a
  • Change 2b from secret init to beginning of MLS handshake
  • Change 4b to use the MLS Credential step (double check that this is more than an ID)
  • Regular MLS from 4b onwards

Formatting

  • Renumber all the MD headers

Issues

@cla-bot cla-bot bot added the cla-signed label Apr 15, 2023
@expede expede marked this pull request as ready for review July 12, 2023 22:17
@expede expede requested a review from dholms as a code owner July 12, 2023 22:17
@expede expede requested a review from icidasset July 12, 2023 22:17
Copy link
Member

@icidasset icidasset left a comment

Choose a reason for hiding this comment

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

Very well written! Almost as if you've done this before 😜

I noticed a few typos here and there, but otherwise this is looking great. I think I have a good idea how to implement this. The MLS part isn't super clear yet (as indicated by comment on section 6), but I'm sure that'll improve once I read more about it.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Signed-off-by: Brooklyn Zelenka <[email protected]>
Signed-off-by: Brooklyn Zelenka <[email protected]>
Signed-off-by: Brooklyn Zelenka <[email protected]>
Signed-off-by: Brooklyn Zelenka <[email protected]>
Signed-off-by: Brooklyn Zelenka <[email protected]>
@expede
Copy link
Member Author

expede commented Jul 14, 2023

@icidasset

The MLS part isn't super clear yet (as indicated by comment on section 6)

I'm like 99% sure that this works from a spec perspective, but the one part that I wonder about is if we'll need to contriubte upstream to OpenMLS to allow UCANs at the credentials step.

pub enum MlsCredentialType {
    Basic(BasicCredential),
    X509(Certificate),
}

pub struct BasicCredential {
    identity: VLBytes,
}

UCAN is like a much cleaned up and modernized X.509 (though formatted VERY differently). I think that we can hack it into the existing BasicCredential if OpenMLS gives you an opportunity to inspect that payload before continuing (which it will certainly do with X.509). If not, we may need to PR something like:

pub enum MlsCredentialType {
    Basic(BasicCredential),
    X509(Certificate),
    Ucan(UcanCredential), // <---
}

...or more general...

pub enum MlsCredentialType {
    Basic(BasicCredential),
    X509(Certificate),
    Cutstom(CustomCert), // <---
}

pub struct CustomCert { // <---
    certBytes: VLBytes, // <---
}

📚 Resources

@expede expede merged commit 105e862 into main Jul 15, 2023
2 checks passed
@expede expede deleted the mls branch July 15, 2023 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double Ratchet -> MLS
2 participants