Ensure that input validation between API and Apply is consistent

[serathius]

Recent fixes for bugs detected by fuzzing has resulted in improved validation for Apply, for example #13602. Errors during Apply result in panic as raft is unable to progress. Fuzz tests detect those panics within apply function, however this not the only place that fix should be fixed.

Detecting invalid raft message during apply is very late stage. Such messages should be rejected on much earlier stage. Recent fixes has showed that we have no good mechanism to ensure consistency validating messages.

Goal: Ensure that API rejects all requests that would be rejected during apply or caused panic.

Ideas for implementation:

• We could write a fuzzer test that will generate a lot of randomly filled structs representing API rpc. With rpcs we can push them through both validators and check if they result any inconsistencies.

cc @ptabor

[samueleresca]

Hi @serathius, Is this issue available for being picked up? I have experience in fuzzing and I'm looking for some contributions GoLang. Are these the RPC API: https://github.com/etcd-io/etcd/blob/main/server/etcdserver/api/ to fuzz?

[serathius]

@samueleresca Yes feel free to pick those issues up.

Are these the RPC API: https://github.com/etcd-io/etcd/blob/main/server/etcdserver/api/ to fuzz?

This directory contains all Etcd APIs and some unrelated code (candidate for cleanup). For fuzzying we would like to focus on V3 grpc API in https://github.com/etcd-io/etcd/tree/main/server/etcdserver/api/v3rpc.

Problem that we want to solve is inconsistency in request validation between different stages of processing request. First when request comes to etcdserver, second request is applied in all members of the cluster. In both cases request is validated, however if those two validations is inconsistent it might case whole etcd cluster to panic.

For example Range request is:

• first validated in API

  etcd/server/etcdserver/api/v3rpc/key.go

  Lines 114 to 128 in cd9764a

  	func checkRangeRequest(r *pb.RangeRequest) error {
  	if len(r.Key) == 0 {
  	return rpctypes.ErrGRPCEmptyKey
  	}
  	if _, ok := pb.RangeRequest_SortOrder_name[int32(r.SortOrder)]; !ok {
  	return rpctypes.ErrGRPCInvalidSortOption
  	}
  	if _, ok := pb.RangeRequest_SortTarget_name[int32(r.SortTarget)]; !ok {
  	return rpctypes.ErrGRPCInvalidSortOption
  	}
  	return nil
  	}

• and later validated when applying the change

  etcd/server/etcdserver/txn/txn.go

  Lines 179 to 194 in cd9764a

  	if sortOrder != pb.RangeRequest_NONE {
  	var sorter sort.Interface
  	switch {
  	case r.SortTarget == pb.RangeRequest_KEY:
  	sorter = &kvSortByKey{&kvSort{rr.KVs}}
  	case r.SortTarget == pb.RangeRequest_VERSION:
  	sorter = &kvSortByVersion{&kvSort{rr.KVs}}
  	case r.SortTarget == pb.RangeRequest_CREATE:
  	sorter = &kvSortByCreate{&kvSort{rr.KVs}}
  	case r.SortTarget == pb.RangeRequest_MOD:
  	sorter = &kvSortByMod{&kvSort{rr.KVs}}
  	case r.SortTarget == pb.RangeRequest_VALUE:
  	sorter = &kvSortByValue{&kvSort{rr.KVs}}
  	default:
  	lg.Panic("unexpected sort target", zap.Int32("sort-target", int32(r.SortTarget)))
  	}

  (will panic if sort target is invalid)

Goal of the task is ensure that every request that passes etcd validation in API, will not cause the apply code to panic. I proposed to setup a fuzz test that will generate a requests which can be then passed to API validation, if it passed then it should be apply able. Test should fail if there is a request that passes validation but causes panic in apply stage.

[samueleresca]

@serathius I opened a draft PR with an example on the RangeRequest - let me know if it makes sense so I can continue with the other input validation.

[serathius]

Thanks, reviewed.