-
Notifications
You must be signed in to change notification settings - Fork 169
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 authenticated dispersal interface, client+server implementations #167
Add authenticated dispersal interface, client+server implementations #167
Conversation
@@ -11,6 +11,8 @@ service Disperser { | |||
// processing status of the blob. | |||
rpc DisperseBlob(DisperseBlobRequest) returns (DisperseBlobReply) {} | |||
|
|||
rpc DisperseBlobAuthenticated(stream AuthenticatedRequest) returns (stream AuthenticatedReply); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this compare to having a separate interface (like the GetBlobStatus
) for authentication (and adding auth data to the DisperserBlobRequest
in the DisperseBlob
interface)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting idea. In the final flow, we'll likely be having the requester sign the kzg commitment (for payment verification purposes, see doc: https://docs.google.com/document/d/1w2H38txbe21ex8bwxNMAcjRq4K5WHq0NCv6jYy8yo54/edit#heading=h.2ty1ce44eg7b), so I think it is probably necessary to couple them together. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this coupling need to actually tie the dispersal request and authentication request together, i.e. can they be two interfaces?
It will also be great to document the client-server communication protocol here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having them coupled allows the following flow:
- Client sends request
- Disperser calculates kzg commitment and sends back to client with random point opening
- Client verifies kzg commitment (by opening random point) and then signs it
If we want to have decoupled interfaces, it seems like it would have to be a different authentication model (not signing the kzg commitment.) BUt I think the design document makes a good case for why we will need this is the long term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add some comments to the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, sg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should find a way to add an integration test for this endpoint.
It seems like there are few missing pieces which are commented out. Do you intend to add them in a follow up PRs or update this one?
clients/disperser_client.go
Outdated
func (c *disperserClient) DisperseBlobAuthenticated(ctx context.Context, data []byte, quorumID, quorumThreshold, adversityThreshold uint32) (*disperser.BlobStatus, []byte, error) { | ||
|
||
addr := fmt.Sprintf("%v:%v", c.config.Hostname, c.config.Port) | ||
conn, err := grpc.Dial(addr, grpc.WithTransportCredentials(insecure.NewCredentials())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it ever use TLS connection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Adding.
disperser/apiserver/server.go
Outdated
|
||
} | ||
|
||
func (s *DispersalServer) disperseBlob(ctx context.Context, blob *core.Blob, authenticated bool, challengeParam uint32) (*pb.DisperseBlobReply, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are authenticated
and challengeParam
used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed these for now.
@@ -11,6 +11,8 @@ service Disperser { | |||
// processing status of the blob. | |||
rpc DisperseBlob(DisperseBlobRequest) returns (DisperseBlobReply) {} | |||
|
|||
rpc DisperseBlobAuthenticated(stream AuthenticatedRequest) returns (stream AuthenticatedReply); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this coupling need to actually tie the dispersal request and authentication request together, i.e. can they be two interfaces?
It will also be great to document the client-server communication protocol here.
api/proto/disperser/disperser.proto
Outdated
message AuthenticatedRequest { | ||
oneof payload { | ||
DisperseBlobRequest disperse_request = 1; | ||
ChallengeReply challenge_reply = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AuthenticationRequest/AuthenticationReply (or AuthRequest/AuthReply)? The "challenge" is more like the approach we take here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed the paramters. Let me know if this looks better!
Why are these changes needed?
Adds an authenticated dispersal interface, as well as basic client and server implementations. These will be outfitted with signing and authentication from #166.
Checks