Sign get chunks #1022
Sign get chunks #1022
Conversation
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Signed-off-by: Cody Littley <cody@eigenlabs.org>
inabox/tests/integration_v2_test.go
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
This looks just because of missing test coverage, then shall we add a call to test GetChunks?
There was a problem hiding this comment.
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().
api/clients/v2/relay_client.go
Outdated
| 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 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
relay/server_test.go
Outdated
| @@ -30,6 +30,7 @@ func defaultConfig() *Config { | |||
| ChunkCacheSize: 1024 * 1024, | |||
| ChunkMaxConcurrency: 32, | |||
| MaxKeysPerGetChunksRequest: 1024, | |||
| AuthenticationDisabled: true, | |||
There was a problem hiding this comment.
if we set this to false, does integration test fail?
There was a problem hiding this comment.
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.
| // 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) |
There was a problem hiding this comment.
To confirm, all the values of the request are being hashed right ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Seems good area to focus for auditors
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Signed-off-by: Cody Littley <cody@eigenlabs.org>
Signed-off-by: Cody Littley <cody@eigenlabs.org>
| @@ -0,0 +1,68 @@ | |||
| package mock | |||
There was a problem hiding this comment.
why do we need a separate mock for relay?
don't we have an existing mock for IndexedChainState?
There was a problem hiding this comment.
The existing ChainDataMock struct has a bunch of internal logic that looks like it was written to support specific unit tests. I just need a mock implementation where I can use the pattern when X method is called with Y arguments, return Z.
| // 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) |
There was a problem hiding this comment.
Seems good area to focus for auditors
|
lgtm |
Why are these changes needed?
Enable request signing for
GetChunks()requests.Companion PR to update the relay helm chart: https://github.com/Layr-Labs/eigenda-devops/pull/1256
Checks