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

add mls-tools unix socket util #1879

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

add mls-tools unix socket util #1879

wants to merge 41 commits into from

Conversation

erikolsson
Copy link
Contributor

@erikolsson erikolsson commented Dec 19, 2024

MLS Tools

A command line util grpc service for performing hyper specific operations requiring https://github.com/awslabs/mls-rs.
The util reads from stdin and the response is parsed by the caller from stdout.

For now, there are two validation steps, that mainly focus on mls state consistency. We will later be able to add additional checks that focus on security.

  • InitialGroupInfoRequest validates the group info message for MLS group inception. among other things, check that:
    • ensure that the group info message contains the ExternalPubExt (required for external joins)
    • epoch == 0
    • external group snapshot contains tree
  • ExternalJoinRequest validates an external join. Among other things:
    • roll up external group snapshot and process (apply) non-rolled-up commits
    • proposedGroupInfoMessage.epoch == currentEpoch + 1
    • proposedCommit.epoch == currentEpoch
    • we are still missing checks here, but it's a start.

The rust code still needs more tests — will be addressed as we actually start hooking this up.

The util will also be used for snapshotting the MLS state.

Copy link

vercel bot commented Dec 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
river-sample-app ⬜️ Ignored (Inspect) Visit Preview Dec 23, 2024 10:12am

@erikolsson erikolsson changed the title add mls-tools command line util add mls-tools unix socket util Dec 19, 2024
@erikolsson
Copy link
Contributor Author

edit: with amazing help from @bas-vk , this is now a service reachable from grpc over unix sockets!

@erikolsson erikolsson marked this pull request as draft December 19, 2024 13:35
@erikolsson erikolsson marked this pull request as ready for review December 20, 2024 14:25
@erikolsson
Copy link
Contributor Author

erikolsson commented Dec 20, 2024

Some thoughts:

  • Am I overengineering the justfile? Can't all the nodes just share the same instance of mls_service when running locally? That way we can just killall mls_service and be done with it. Also simplifies the the call site (mls_service.go)
  • I'm fine hardcoding the socket address "unix:/tmp/mls_service" in core/node/mls_service/mls_service.go for now, there should be no need to run multiple instances of MLS services
  • I'm not sure how I should make sure that this runs in the node — where should I hook in supervisord?

@bas-vk @texuf @sergekh2

@@ -96,6 +96,7 @@ jobs:
--build-arg VER_VERSION=$RELEASE_VERSION \
--build-arg VER_BRANCH=$BRANCH \
--build-arg VER_COMMIT=$COMMIT_HASH \
--build-context mls=../mls \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is needed for the dockerfile to allow copying from a parent dir

@@ -0,0 +1,325 @@
use mls_rs::extension::built_in::ExternalPubExt;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the contents of this file aren't really important at the moment, we will probably end up tweaking it a lot once we have the glue in place

@texuf
Copy link
Contributor

texuf commented Dec 20, 2024

Some thoughts:

  • Am I overengineering the justfile? Can't all the nodes just share the same instance of mls_service when running locally? That way we can just killall mls_service and be done with it. Also simplifies the the call site (mls_service.go)

  • I'm fine hardcoding the socket address "unix:/tmp/mls_service" in core/node/mls_service/mls_service.go for now, there should be no need to run multiple instances of MLS services

  • I'm not sure how I should make sure that this runs in the node — where should I hook in supervisord?

@bas-vk @texuf @sergekh2

I'm surprised that it's a service. I expected some kind of static library. Is there state?

@texuf
Copy link
Contributor

texuf commented Dec 20, 2024

Some thoughts:

  • Am I overengineering the justfile? Can't all the nodes just share the same instance of mls_service when running locally? That way we can just killall mls_service and be done with it. Also simplifies the the call site (mls_service.go)

  • I'm fine hardcoding the socket address "unix:/tmp/mls_service" in core/node/mls_service/mls_service.go for now, there should be no need to run multiple instances of MLS services

  • I'm not sure how I should make sure that this runs in the node — where should I hook in supervisord?

@bas-vk @texuf @sergekh2

I'm surprised that it's a service. I expected some kind of static library. Is there state?

Locally, in my opinion, each node should behave as close as possible to how it's going to behave remotely. So each node should get its own service.

@jakzale
Copy link
Contributor

jakzale commented Dec 23, 2024

I'm surprised that it's a service. I expected some kind of static library. Is there state?

AFAIU there is nothing preventing us from loading mls-related code as a static library, but running it via ProtoBuf RPC was just faster to do. We can revisit it post v1.

@bas-vk
Copy link
Contributor

bas-vk commented Dec 23, 2024

I'm surprised that it's a service. I expected some kind of static library. Is there state?

AFAIU there is nothing preventing us from loading mls-related code as a static library, but running it via ProtoBuf RPC was just faster to do. We can revisit it post v1.

Correct, this is a quick & easy method that allows Erik to move forward. The cargo project is organised that we can easily switch to different methods such as ffi or cgo.

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.

4 participants