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

Relay authentication #911

Merged
merged 38 commits into from
Nov 21, 2024
Merged

Conversation

cody-littley
Copy link
Contributor

Why are these changes needed?

Add authentication for GetChunks() requests.

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
@cody-littley cody-littley self-assigned this Nov 19, 2024
@cody-littley cody-littley marked this pull request as draft November 19, 2024 19:04
@cody-littley cody-littley marked this pull request as ready for review November 19, 2024 21:06
@@ -34,7 +34,7 @@ message GetChunksRequest {

// If this is an authenticated request, this should hold the ID of the requester. If this
// is an unauthenticated request, this field should be empty.
uint64 requester_id = 2;
bytes requester_id = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

From the code below, this is interpreted as operator ID. It'll be helpful to document it here so the users know how to set it.

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 renamed this field to operator_id. Is that sufficient, or do you think I should add additional documentation?

request *pb.GetChunksRequest,
now time.Time) error {

if a == nil {
Copy link
Contributor

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 check the authenticator from outside?

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 way I have it here is wrong (it didn't pass unit tests). I removed this check and instead now make it in the outside context.

operatorID := core.OperatorID(request.RequesterId)
operator, ok := operators[operatorID]
if !ok {
return errors.New("operator not found")
Copy link
Contributor

Choose a reason for hiding this comment

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

It'll be helpful to include the block number in error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

if err != nil {
return fmt.Errorf("failed to get current block number: %w", err)
}
operators, err := a.ics.GetIndexedOperators(context.Background(), blockNumber)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are 2 RPC calls here, it'd be helpful to cache the operator state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

caching added

if !ok {
return errors.New("operator not found")
}
key := operator.PubkeyG2
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd cache operatorID->G2, this is a mapping that'll not change (hence highly valuable to cache)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. This will break if we ever allow operators to change keys, but that's a bridge we can cross in the future.

@@ -34,7 +34,7 @@ message GetChunksRequest {

// If this is an authenticated request, this should hold the ID of the requester. If this
// is an unauthenticated request, this field should be empty.
uint64 requester_id = 2;
bytes requester_id = 2;

// If this is an authenticated request, this field will hold a signature by the requester
// on the chunks being requested.
Copy link
Contributor

Choose a reason for hiding this comment

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

Document how this signature should be computed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  // If this is an authenticated request, this field will hold a BLS signature by the requester
  // on the hash of this request. Relays may choose to reject unauthenticated requests.
  //
  // The following describes the schema for computing the hash of this request
  // This algorithm is implemented in golang using relay.auth.HashGetChunksRequest().
  //
  // All integers are encoded as unsigned 4 byte big endian values.
  //
  // Perform a keccak256 hash on the following data in the following order:
  // 1. the operator id
  // 2. for each chunk request:
  //    a. if the chunk request is a request by index:
  //       i. the blob key
  //       ii. the start index
  //       iii. the end index
  //    b. if the chunk request is a request by range:
  //       i. the blob key
  //       ii. each requested chunk index, in order
  bytes operator_signature = 3;

Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
@cody-littley cody-littley mentioned this pull request Nov 20, 2024
5 tasks
Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

lgtm! few comments

relay/auth/authenticator.go Outdated Show resolved Hide resolved
relay/auth/authenticator.go Outdated Show resolved Hide resolved
relay/auth/authenticator.go Show resolved Hide resolved
relay/auth/request_signing.go Show resolved Hide resolved
relay/auth/request_signing.go Outdated Show resolved Hide resolved
return nil, fmt.Errorf("failed to create eth writer: %w", err)
}

idx, err := coreindexer.CreateNewIndexer(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this using a built-in indexer? Should we use a graph indexer instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted to use a thegraph indexer

relay/server.go Outdated Show resolved Hide resolved
}
key := operator.PubkeyG2

a.keyCache.Store(operatorID, key)
Copy link
Contributor

Choose a reason for hiding this comment

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

It may just preload the cache by storing all operators's pubkeys returned from GetIndexedOperators. This will reduce num of RPCs from 2*NumOperators to 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done.

func (a *requestAuthenticator) PreloadCache() error {
	blockNumber, err := a.ics.GetCurrentBlockNumber()
	if err != nil {
		return fmt.Errorf("failed to get current block number: %w", err)
	}
	operators, err := a.ics.GetIndexedOperators(context.Background(), blockNumber)
	if err != nil {
		return fmt.Errorf("failed to get operators: %w", err)
	}

	for operatorID, operator := range operators {
		a.keyCache.Add(operatorID, operator.PubkeyG2)
	}

	return nil
}

Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
Signed-off-by: Cody Littley <[email protected]>
}, nil
}

func (a *requestAuthenticator) PreloadCache() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make it private and run in the the NewRequestAuthenticator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change made

Signed-off-by: Cody Littley <[email protected]>
@cody-littley cody-littley merged commit 953397b into Layr-Labs:master Nov 21, 2024
6 checks passed
@cody-littley cody-littley deleted the relay-authentication branch November 21, 2024 14:49
@@ -221,6 +264,10 @@ var optionalFlags = []cli.Flag{
MaxGetChunkBytesPerSecondClientFlag,
GetChunkBytesBurstinessClientFlag,
MaxConcurrentGetChunkOpsClientFlag,
IndexerPullIntervalFlag,
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this, right?

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.

3 participants