From 7fdc0c8e95c4157dd9e3ce3f9a4efe370554a19c Mon Sep 17 00:00:00 2001 From: Matthew Pendrey Date: Tue, 20 Aug 2024 15:06:46 +0100 Subject: [PATCH] ks-404 validate ids used in transmission schedule (#14116) --- .changeset/slimy-cars-sparkle.md | 5 +++ .../ccip/launcher/test_helpers.go | 2 +- core/capabilities/remote/dispatcher.go | 1 - core/capabilities/remote/target/client.go | 9 ++++- core/capabilities/remote/target/server.go | 3 +- core/capabilities/remote/trigger_publisher.go | 5 ++- core/capabilities/remote/utils.go | 24 ------------ core/capabilities/remote/utils_test.go | 12 ------ .../local_target_capability_test.go | 16 ++++---- .../capabilities/transmission/transmission.go | 12 ++++-- .../transmission/transmission_test.go | 22 ++++++----- core/capabilities/validation/validation.go | 38 +++++++++++++++++++ .../validation/validation_test.go | 19 ++++++++++ 13 files changed, 103 insertions(+), 65 deletions(-) create mode 100644 .changeset/slimy-cars-sparkle.md create mode 100644 core/capabilities/validation/validation.go create mode 100644 core/capabilities/validation/validation_test.go diff --git a/.changeset/slimy-cars-sparkle.md b/.changeset/slimy-cars-sparkle.md new file mode 100644 index 00000000000..aa7658ae908 --- /dev/null +++ b/.changeset/slimy-cars-sparkle.md @@ -0,0 +1,5 @@ +--- +"chainlink": patch +--- + +#internal ks-404 validate ids before using as seed of transmission schedule diff --git a/core/capabilities/ccip/launcher/test_helpers.go b/core/capabilities/ccip/launcher/test_helpers.go index a2ebf3fdba9..e1b47fa3521 100644 --- a/core/capabilities/ccip/launcher/test_helpers.go +++ b/core/capabilities/ccip/launcher/test_helpers.go @@ -24,7 +24,7 @@ var ( p2pID1 = getP2PID(1) p2pID2 = getP2PID(2) defaultCapCfgs = map[string]registrysyncer.CapabilityConfiguration{ - defaultCapability.ID: registrysyncer.CapabilityConfiguration{}, + defaultCapability.ID: {}, } defaultRegistryDon = registrysyncer.DON{ DON: getDON(1, []ragep2ptypes.PeerID{p2pID1}, 0), diff --git a/core/capabilities/remote/dispatcher.go b/core/capabilities/remote/dispatcher.go index dab4f6c98bf..bed485c286e 100644 --- a/core/capabilities/remote/dispatcher.go +++ b/core/capabilities/remote/dispatcher.go @@ -13,7 +13,6 @@ import ( "github.com/smartcontractkit/chainlink-common/pkg/services" "github.com/smartcontractkit/chainlink-common/pkg/types/core" - "github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/types" remotetypes "github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/types" "github.com/smartcontractkit/chainlink/v2/core/logger" diff --git a/core/capabilities/remote/target/client.go b/core/capabilities/remote/target/client.go index 4273169d23e..8572efed155 100644 --- a/core/capabilities/remote/target/client.go +++ b/core/capabilities/remote/target/client.go @@ -12,6 +12,7 @@ import ( "github.com/smartcontractkit/chainlink/v2/core/capabilities/remote" "github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/target/request" "github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/types" + "github.com/smartcontractkit/chainlink/v2/core/capabilities/validation" "github.com/smartcontractkit/chainlink/v2/core/logger" ) @@ -172,8 +173,12 @@ func (c *client) Receive(ctx context.Context, msg *types.MessageBody) { } func GetMessageIDForRequest(req commoncap.CapabilityRequest) (string, error) { - if !remote.IsValidWorkflowOrExecutionID(req.Metadata.WorkflowID) || !remote.IsValidWorkflowOrExecutionID(req.Metadata.WorkflowExecutionID) { - return "", errors.New("workflow ID and workflow execution ID in request metadata are invalid") + if err := validation.ValidateWorkflowOrExecutionID(req.Metadata.WorkflowID); err != nil { + return "", fmt.Errorf("workflow ID is invalid: %w", err) + } + + if err := validation.ValidateWorkflowOrExecutionID(req.Metadata.WorkflowExecutionID); err != nil { + return "", fmt.Errorf("workflow execution ID is invalid: %w", err) } return req.Metadata.WorkflowID + req.Metadata.WorkflowExecutionID, nil diff --git a/core/capabilities/remote/target/server.go b/core/capabilities/remote/target/server.go index 56cad3739b6..5324475b192 100644 --- a/core/capabilities/remote/target/server.go +++ b/core/capabilities/remote/target/server.go @@ -14,6 +14,7 @@ import ( "github.com/smartcontractkit/chainlink/v2/core/capabilities/remote" "github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/target/request" "github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/types" + "github.com/smartcontractkit/chainlink/v2/core/capabilities/validation" p2ptypes "github.com/smartcontractkit/chainlink/v2/core/services/p2p/types" "github.com/smartcontractkit/chainlink/v2/core/logger" @@ -210,7 +211,7 @@ func (r *server) getMessageHash(msg *types.MessageBody) ([32]byte, error) { func GetMessageID(msg *types.MessageBody) (string, error) { idStr := string(msg.MessageId) - if !remote.IsValidID(idStr) { + if !validation.IsValidID(idStr) { return "", fmt.Errorf("invalid message id") } return idStr, nil diff --git a/core/capabilities/remote/trigger_publisher.go b/core/capabilities/remote/trigger_publisher.go index 23b778f6018..4aac821bc14 100644 --- a/core/capabilities/remote/trigger_publisher.go +++ b/core/capabilities/remote/trigger_publisher.go @@ -10,6 +10,7 @@ import ( "github.com/smartcontractkit/chainlink-common/pkg/capabilities/pb" "github.com/smartcontractkit/chainlink-common/pkg/services" "github.com/smartcontractkit/chainlink/v2/core/capabilities/remote/types" + "github.com/smartcontractkit/chainlink/v2/core/capabilities/validation" "github.com/smartcontractkit/chainlink/v2/core/logger" p2ptypes "github.com/smartcontractkit/chainlink/v2/core/services/p2p/types" ) @@ -102,8 +103,8 @@ func (p *triggerPublisher) Receive(_ context.Context, msg *types.MessageBody) { p.lggr.Errorw("sender not a member of its workflow DON", "capabilityId", p.capInfo.ID, "callerDonId", msg.CallerDonId, "sender", sender) return } - if !IsValidWorkflowOrExecutionID(req.Metadata.WorkflowID) { - p.lggr.Errorw("received trigger request with invalid workflow ID", "capabilityId", p.capInfo.ID, "workflowId", SanitizeLogString(req.Metadata.WorkflowID)) + if err = validation.ValidateWorkflowOrExecutionID(req.Metadata.WorkflowID); err != nil { + p.lggr.Errorw("received trigger request with invalid workflow ID", "capabilityId", p.capInfo.ID, "workflowId", SanitizeLogString(req.Metadata.WorkflowID), "err", err) return } p.lggr.Debugw("received trigger registration", "capabilityId", p.capInfo.ID, "workflowId", req.Metadata.WorkflowID, "sender", sender) diff --git a/core/capabilities/remote/utils.go b/core/capabilities/remote/utils.go index 7e303eefc8f..c77ef679160 100644 --- a/core/capabilities/remote/utils.go +++ b/core/capabilities/remote/utils.go @@ -19,8 +19,6 @@ import ( const ( maxLoggedStringLen = 256 - validWorkflowIDLen = 64 - maxIDLen = 128 ) func ValidateMessage(msg p2ptypes.Message, expectedReceiver p2ptypes.PeerID) (*remotetypes.MessageBody, error) { @@ -115,25 +113,3 @@ func SanitizeLogString(s string) string { } return s + tooLongSuffix } - -// Workflow IDs and Execution IDs are 32-byte hex-encoded strings -func IsValidWorkflowOrExecutionID(id string) bool { - if len(id) != validWorkflowIDLen { - return false - } - _, err := hex.DecodeString(id) - return err == nil -} - -// Trigger event IDs and message IDs can only contain printable characters and must be non-empty -func IsValidID(id string) bool { - if len(id) == 0 || len(id) > maxIDLen { - return false - } - for i := 0; i < len(id); i++ { - if !unicode.IsPrint(rune(id[i])) { - return false - } - } - return true -} diff --git a/core/capabilities/remote/utils_test.go b/core/capabilities/remote/utils_test.go index 177ab5a7d14..12135073362 100644 --- a/core/capabilities/remote/utils_test.go +++ b/core/capabilities/remote/utils_test.go @@ -129,15 +129,3 @@ func TestSanitizeLogString(t *testing.T) { } require.Equal(t, longString[:256]+" [TRUNCATED]", remote.SanitizeLogString(longString)) } - -func TestIsValidWorkflowID(t *testing.T) { - require.False(t, remote.IsValidWorkflowOrExecutionID("too_short")) - require.False(t, remote.IsValidWorkflowOrExecutionID("nothex--95ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0")) - require.True(t, remote.IsValidWorkflowOrExecutionID("15c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0")) -} - -func TestIsValidTriggerEventID(t *testing.T) { - require.False(t, remote.IsValidID("")) - require.False(t, remote.IsValidID("\n\n")) - require.True(t, remote.IsValidID("id_id_2")) -} diff --git a/core/capabilities/transmission/local_target_capability_test.go b/core/capabilities/transmission/local_target_capability_test.go index 93bf708ccef..cdca854986b 100644 --- a/core/capabilities/transmission/local_target_capability_test.go +++ b/core/capabilities/transmission/local_target_capability_test.go @@ -54,8 +54,8 @@ func TestScheduledExecutionStrategy_LocalDON(t *testing.T) { name: "position 0; oneAtATime", position: 0, schedule: "oneAtATime", - low: 300 * time.Millisecond, - high: 400 * time.Millisecond, + low: 200 * time.Millisecond, + high: 300 * time.Millisecond, }, { name: "position 1; oneAtATime", @@ -68,15 +68,15 @@ func TestScheduledExecutionStrategy_LocalDON(t *testing.T) { name: "position 2; oneAtATime", position: 2, schedule: "oneAtATime", - low: 0 * time.Millisecond, - high: 100 * time.Millisecond, + low: 300 * time.Millisecond, + high: 400 * time.Millisecond, }, { name: "position 3; oneAtATime", position: 3, schedule: "oneAtATime", - low: 100 * time.Millisecond, - high: 300 * time.Millisecond, + low: 0 * time.Millisecond, + high: 100 * time.Millisecond, }, { name: "position 0; allAtOnce", @@ -121,8 +121,8 @@ func TestScheduledExecutionStrategy_LocalDON(t *testing.T) { req := capabilities.CapabilityRequest{ Config: m, Metadata: capabilities.RequestMetadata{ - WorkflowID: "mock-workflow-id", - WorkflowExecutionID: "mock-execution-id-1", + WorkflowID: "15c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0", + WorkflowExecutionID: "32c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce1", }, } diff --git a/core/capabilities/transmission/transmission.go b/core/capabilities/transmission/transmission.go index b41be5bcaa5..88ce0fa3edd 100644 --- a/core/capabilities/transmission/transmission.go +++ b/core/capabilities/transmission/transmission.go @@ -4,10 +4,10 @@ import ( "fmt" "time" - "github.com/pkg/errors" - "github.com/smartcontractkit/libocr/permutation" + "github.com/smartcontractkit/chainlink/v2/core/capabilities/validation" + "github.com/smartcontractkit/chainlink-common/pkg/capabilities" "github.com/smartcontractkit/chainlink-common/pkg/values" "github.com/smartcontractkit/chainlink/v2/core/services/p2p/types" @@ -56,8 +56,12 @@ func GetPeerIDToTransmissionDelay(donPeerIDs []types.PeerID, req capabilities.Ca return nil, fmt.Errorf("failed to extract transmission config from request: %w", err) } - if req.Metadata.WorkflowID == "" || req.Metadata.WorkflowExecutionID == "" { - return nil, errors.New("workflow ID and workflow execution ID must be set in request metadata") + if err = validation.ValidateWorkflowOrExecutionID(req.Metadata.WorkflowID); err != nil { + return nil, fmt.Errorf("workflow ID is invalid: %w", err) + } + + if err = validation.ValidateWorkflowOrExecutionID(req.Metadata.WorkflowExecutionID); err != nil { + return nil, fmt.Errorf("workflow execution ID is invalid: %w", err) } transmissionID := req.Metadata.WorkflowID + req.Metadata.WorkflowExecutionID diff --git a/core/capabilities/transmission/transmission_test.go b/core/capabilities/transmission/transmission_test.go index fba233eadb0..aaa367e78cf 100644 --- a/core/capabilities/transmission/transmission_test.go +++ b/core/capabilities/transmission/transmission_test.go @@ -36,20 +36,21 @@ func Test_GetPeerIDToTransmissionDelay(t *testing.T) { "one", "oneAtATime", "100ms", - "mock-execution-id", + "15c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0", map[string]time.Duration{ "one": 300 * time.Millisecond, - "two": 100 * time.Millisecond, - "three": 0 * time.Millisecond, + "two": 0 * time.Millisecond, + "three": 100 * time.Millisecond, "four": 200 * time.Millisecond, }, }, + { "TestAllAtOnce", "one", "allAtOnce", "100ms", - "mock-execution-id", + "15c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0", map[string]time.Duration{ "one": 0 * time.Millisecond, "two": 0 * time.Millisecond, @@ -57,17 +58,18 @@ func Test_GetPeerIDToTransmissionDelay(t *testing.T) { "four": 0 * time.Millisecond, }, }, + { "TestOneAtATimeWithDifferentExecutionID", "one", "oneAtATime", "100ms", - "mock-execution-id2", + "16c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce1", map[string]time.Duration{ - "one": 0 * time.Millisecond, - "two": 200 * time.Millisecond, - "three": 100 * time.Millisecond, - "four": 300 * time.Millisecond, + "one": 300 * time.Millisecond, + "two": 100 * time.Millisecond, + "three": 200 * time.Millisecond, + "four": 0 * time.Millisecond, }, }, } @@ -83,7 +85,7 @@ func Test_GetPeerIDToTransmissionDelay(t *testing.T) { capabilityRequest := capabilities.CapabilityRequest{ Config: transmissionCfg, Metadata: capabilities.RequestMetadata{ - WorkflowID: "mock-workflow-id", + WorkflowID: "17c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0", WorkflowExecutionID: tc.workflowExecutionID, }, } diff --git a/core/capabilities/validation/validation.go b/core/capabilities/validation/validation.go new file mode 100644 index 00000000000..67ee3a504cf --- /dev/null +++ b/core/capabilities/validation/validation.go @@ -0,0 +1,38 @@ +package validation + +import ( + "encoding/hex" + "errors" + "unicode" +) + +const ( + validWorkflowIDLen = 64 + maxIDLen = 128 +) + +// Workflow IDs and Execution IDs are 32-byte hex-encoded strings +func ValidateWorkflowOrExecutionID(id string) error { + if len(id) != validWorkflowIDLen { + return errors.New("must be 32 bytes long") + } + _, err := hex.DecodeString(id) + if err != nil { + return errors.New("must be a hex-encoded string") + } + + return nil +} + +// Trigger event IDs and message IDs can only contain printable characters and must be non-empty +func IsValidID(id string) bool { + if len(id) == 0 || len(id) > maxIDLen { + return false + } + for i := 0; i < len(id); i++ { + if !unicode.IsPrint(rune(id[i])) { + return false + } + } + return true +} diff --git a/core/capabilities/validation/validation_test.go b/core/capabilities/validation/validation_test.go new file mode 100644 index 00000000000..205898652fd --- /dev/null +++ b/core/capabilities/validation/validation_test.go @@ -0,0 +1,19 @@ +package validation + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestIsValidWorkflowID(t *testing.T) { + require.NotNil(t, ValidateWorkflowOrExecutionID("too_short")) + require.NotNil(t, ValidateWorkflowOrExecutionID("nothex--95ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0")) + require.NoError(t, ValidateWorkflowOrExecutionID("15c631d295ef5e32deb99a10ee6804bc4af13855687559d7ff6552ac6dbb2ce0")) +} + +func TestIsValidTriggerEventID(t *testing.T) { + require.False(t, IsValidID("")) + require.False(t, IsValidID("\n\n")) + require.True(t, IsValidID("id_id_2")) +}