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.

Sign up for GitHub

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

Sign get chunks #1022

Merged
merged 10 commits into from
Dec 19, 2024
Merged

Sign get chunks #1022

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8bf1fa0
DA nodes sign GetChunks() requests.
cody-littley Dec 17, 2024
5ce2082
checkin
cody-littley Dec 17, 2024
73583d8
Incremental changes.
cody-littley Dec 17, 2024
7309c0e
Merge branch 'master' into sign-get-chunks
cody-littley Dec 17, 2024
c818bee
Fixed unit tests.
cody-littley Dec 17, 2024
a073a28
Made reccomended changes.
cody-littley Dec 18, 2024
8bf552b
Incremental progress.
cody-littley Dec 18, 2024
cb1e42c
Augment random utils.
cody-littley Dec 18, 2024
4743516
Made suggested changes, test improvements.
cody-littley Dec 18, 2024
4c12228
Merge branch 'master' into sign-get-chunks
cody-littley Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 52 additions & 8 deletions api/clients/v2/relay_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package clients
import (
"context"
"fmt"
"github.com/Layr-Labs/eigenda/core"
"github.com/Layr-Labs/eigenda/relay/auth"
"sync"

relaygrpc "github.com/Layr-Labs/eigenda/api/grpc/relay"
Expand All @@ -12,9 +14,14 @@ import (
"google.golang.org/grpc"
)

// MessageSigner is a function that signs a message with a private BLS key.
type MessageSigner func(ctx context.Context, data [32]byte) (*core.Signature, error)

type RelayClientConfig struct {
Sockets map[corev2.RelayKey]string
UseSecureGrpcFlag bool
OperatorID *core.OperatorID
MessageSigner MessageSigner
}

type ChunkRequestByRange struct {
Expand Down Expand Up @@ -62,8 +69,8 @@ var _ RelayClient = (*relayClient)(nil)

// NewRelayClient creates a new RelayClient that connects to the relays specified in the config.
// It keeps a connection to each relay and reuses it for subsequent requests, and the connection is lazily instantiated.
func NewRelayClient(config *RelayClientConfig, logger logging.Logger) (*relayClient, error) {
if config == nil || len(config.Sockets) <= 0 {
func NewRelayClient(config *RelayClientConfig, logger logging.Logger) (RelayClient, error) {
if config == nil || len(config.Sockets) <= 0 || config.OperatorID == nil || config.MessageSigner == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we move this check config.OperatorID == nil || config.MessageSigner == nil to GetChunks methods?
Other methods like GetBlob should work with nil operator ID and nil signer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved these checks out of the constructor. They will cause the methods that get chunks to return an error if they are not properly configured.

return nil, fmt.Errorf("invalid config: %v", config)
}

Expand Down Expand Up @@ -97,7 +104,25 @@ func (c *relayClient) GetBlob(ctx context.Context, relayKey corev2.RelayKey, blo
return res.GetBlob(), nil
}

func (c *relayClient) GetChunksByRange(ctx context.Context, relayKey corev2.RelayKey, requests []*ChunkRequestByRange) ([][]byte, error) {
// signGetChunksRequest signs the GetChunksRequest with the operator's private key
// and sets the signature in the request.
func (c *relayClient) signGetChunksRequest(ctx context.Context, request *relaygrpc.GetChunksRequest) error {
hash := auth.HashGetChunksRequest(request)
Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm, all the values of the request are being hashed right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as long as I didn't make a mistake when coding the hashing function. Makes me nervous writing these types of things by hand due to the possibility of human error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems good area to focus for auditors

hashArray := [32]byte{}
copy(hashArray[:], hash)
signature, err := c.config.MessageSigner(ctx, hashArray)
if err != nil {
return fmt.Errorf("failed to sign get chunks request: %v", err)
}
request.OperatorSignature = signature.Serialize()
return nil
}

func (c *relayClient) GetChunksByRange(
ctx context.Context,
relayKey corev2.RelayKey,
requests []*ChunkRequestByRange) ([][]byte, error) {

if len(requests) == 0 {
return nil, fmt.Errorf("no requests")
}
Expand All @@ -118,18 +143,29 @@ func (c *relayClient) GetChunksByRange(ctx context.Context, relayKey corev2.Rela
},
}
}
res, err := client.GetChunks(ctx, &relaygrpc.GetChunksRequest{

request := &relaygrpc.GetChunksRequest{
ChunkRequests: grpcRequests,
})
OperatorId: c.config.OperatorID[:],
}
err = c.signGetChunksRequest(ctx, request)
if err != nil {
return nil, err
}

res, err := client.GetChunks(ctx, request)
if err != nil {
return nil, err
}

return res.GetData(), nil
}

func (c *relayClient) GetChunksByIndex(ctx context.Context, relayKey corev2.RelayKey, requests []*ChunkRequestByIndex) ([][]byte, error) {
func (c *relayClient) GetChunksByIndex(
ctx context.Context,
relayKey corev2.RelayKey,
requests []*ChunkRequestByIndex) ([][]byte, error) {

if len(requests) == 0 {
return nil, fmt.Errorf("no requests")
}
Expand All @@ -150,9 +186,17 @@ func (c *relayClient) GetChunksByIndex(ctx context.Context, relayKey corev2.Rela
},
}
}
res, err := client.GetChunks(ctx, &relaygrpc.GetChunksRequest{

request := &relaygrpc.GetChunksRequest{
ChunkRequests: grpcRequests,
})
OperatorId: c.config.OperatorID[:],
}
err = c.signGetChunksRequest(ctx, request)
if err != nil {
return nil, err
}

res, err := client.GetChunks(ctx, request)

if err != nil {
return nil, err
Expand Down
11 changes: 10 additions & 1 deletion inabox/tests/integration_v2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,18 @@ var _ = Describe("Inabox v2 Integration", func() {
}
}

// These values are only used by the relay client to get chunks, which we do not call from the relay
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks just because of missing test coverage, then shall we add a call to test GetChunks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked to @ian-shim about this. He said the calls to GetChunks() are made from the operator nodes, not from this client instance. We are only using this client instance to call GetBlobs().

// client below. So ok to use dummy values here.
var dummyOperatorID core.OperatorID
dummySigner := func(ctx context.Context, data [32]byte) (*core.Signature, error) {
return nil, nil
}

// Test retrieval from relay
relayClient, err := clients.NewRelayClient(&clients.RelayClientConfig{
Sockets: relays,
Sockets: relays,
OperatorID: &dummyOperatorID,
MessageSigner: dummySigner,
}, logger)
Expect(err).To(BeNil())

Expand Down
4 changes: 4 additions & 0 deletions node/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,8 @@ func NewNode(
relayClient, err = clients.NewRelayClient(&clients.RelayClientConfig{
Sockets: relayURLs,
UseSecureGrpcFlag: config.UseSecureGrpc,
OperatorID: &config.ID,
MessageSigner: n.SignMessage,
}, logger)

if err != nil {
Expand Down Expand Up @@ -460,6 +462,8 @@ func (n *Node) RefreshOnchainState(ctx context.Context) error {
relayClient, err := clients.NewRelayClient(&clients.RelayClientConfig{
Sockets: relayURLs,
UseSecureGrpcFlag: n.Config.UseSecureGrpc,
OperatorID: &n.Config.ID,
MessageSigner: n.SignMessage,
}, n.Logger)
if err != nil {
n.Logger.Error("error creating relay client", "err", err)
Expand Down
9 changes: 8 additions & 1 deletion node/node_v2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,15 @@ func TestRefreshOnchainStateSuccess(t *testing.T) {
relayURLs := map[v2.RelayKey]string{
0: "http://localhost:8080",
}

messageSigner := func(ctx context.Context, data [32]byte) (*core.Signature, error) {
return nil, nil
}

relayClient, err := clients.NewRelayClient(&clients.RelayClientConfig{
Sockets: relayURLs,
Sockets: relayURLs,
OperatorID: &c.node.Config.ID,
MessageSigner: messageSigner,
}, c.node.Logger)
require.NoError(t, err)
// set up non-mock client
Expand Down
6 changes: 0 additions & 6 deletions relay/auth/authenticator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"errors"
"fmt"
"strings"
"sync"
"time"

Expand Down Expand Up @@ -107,11 +106,6 @@ func (a *requestAuthenticator) AuthenticateGetChunksRequest(
request *pb.GetChunksRequest,
now time.Time) error {

if strings.HasPrefix(origin, "127.0.0.1") {
// TODO(ian-shim): Remove this block once we have a way to authenticate requests.
return nil
}

if a.isAuthenticationStillValid(now, origin) {
// We've recently authenticated this client. Do not authenticate again for a while.
return nil
Expand Down
2 changes: 1 addition & 1 deletion relay/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ func defaultConfig() *Config {
ChunkCacheSize: 1024 * 1024,
ChunkMaxConcurrency: 32,
MaxKeysPerGetChunksRequest: 1024,
AuthenticationDisabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

if we set this to false, does integration test fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disabling authentication doesn't cause signed requests to be rejected, it just causes signatures to be entirely ignored by the relay.

That being said, disabling authentication in the relay tests was a bit of a shortcut (the bad kind). I've modified the code in this test file to properly enable authentication.

RateLimits: limiter.Config{
MaxGetBlobOpsPerSecond: 1024,
GetBlobOpsBurstiness: 1024,
Expand All @@ -47,7 +48,6 @@ func defaultConfig() *Config {
GetChunkBytesBurstinessClient: 2 * 1024 * 1024,
MaxConcurrentGetChunkOpsClient: 1,
},
AuthenticationDisabled: true,
Timeouts: TimeoutConfig{
GetBlobTimeout: 10 * time.Second,
GetChunksTimeout: 10 * time.Second,
Expand Down
Loading