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

Ensure that input validation between API and Apply is consistent #14561

Merged
merged 4 commits into from
Oct 24, 2022
Merged

Ensure that input validation between API and Apply is consistent #14561

merged 4 commits into from
Oct 24, 2022

Conversation

samueleresca
Copy link
Contributor

@samueleresca samueleresca commented Oct 7, 2022

Fixes #13617
The fuzz input is passed to the input validation. In case the input validation passes - the application of that request shouldn't panic.

Adding a fuzzing script and a GitHub action for executing fuzzing with a 5 min time limit

@samueleresca samueleresca changed the title Adding a draft of fuzzing on Range request. Ensure that input validation between API and Apply is consistent Oct 7, 2022
@serathius
Copy link
Member

Looks great, would be good setup CI to run fuzz test. I think we should run:

  • Short 5-10m run o every pull request to verify that fuzz works
  • Long 1-2h once a day to do a proper fuzzing

What do you think? Do you need help with that? We doing the first one should be enough for now.

@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2022

Codecov Report

Merging #14561 (bbda804) into main (e5790d2) will decrease coverage by 0.15%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #14561      +/-   ##
==========================================
- Coverage   75.60%   75.44%   -0.16%     
==========================================
  Files         457      457              
  Lines       37267    37267              
==========================================
- Hits        28176    28117      -59     
- Misses       7334     7383      +49     
- Partials     1757     1767      +10     
Flag Coverage Δ
all 75.44% <ø> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
server/storage/mvcc/watchable_store.go 84.42% <0.00%> (-9.06%) ⬇️
client/pkg/v3/tlsutil/tlsutil.go 83.33% <0.00%> (-8.34%) ⬇️
client/pkg/v3/testutil/leak.go 62.83% <0.00%> (-7.08%) ⬇️
client/v3/leasing/util.go 91.66% <0.00%> (-6.67%) ⬇️
raft/rafttest/node.go 95.00% <0.00%> (-5.00%) ⬇️
server/storage/mvcc/kvstore_compaction.go 95.65% <0.00%> (-4.35%) ⬇️
server/etcdserver/txn/util.go 75.47% <0.00%> (-3.78%) ⬇️
client/v3/concurrency/election.go 79.68% <0.00%> (-2.35%) ⬇️
server/etcdserver/api/v3rpc/interceptor.go 75.52% <0.00%> (-2.09%) ⬇️
server/storage/mvcc/kvstore.go 89.00% <0.00%> (-1.42%) ⬇️
... and 17 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@samueleresca
Copy link
Contributor Author

I think we should run:

  • Short 5-10m run o every pull request to verify that fuzz works
  • Long 1-2h once a day to do a proper fuzzing

What do you think? Do you need help with that? We doing the first one should be enough for now.

@serathius I have added the GitHub Action for fuzzing:

I'm proceeding by covering the others input validation methods.

Any other feedback is obvs welcome


log_callout -e "\\nExecuting v3rpc with target $FUZZ_TARGET \\n"

cd $BASE_PATH
Copy link
Member

Choose a reason for hiding this comment

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

This script requires providing multiple non-trivial environment variables for it to work. This script should only be called via github action, but also locally to reproduce issues. I think we should make the script independent so it can be called without to much hardship.

Can we have the script include a good defaults for running? Or if the default cannot be provided, could we add validation to give better 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.

I updated the script:

  • FUZZ_TIME now defaults to 300s
  • TARGET_PATH display an error in the case that is unset
  • FUZZ_TARGET display an error in the case that is unset

Let me know if you prefer another setup

Comment on lines 22 to 5
# To fix according to newer version of go:
# go get golang.org/dl/gotip
# gotip download
# GO_CMD="gotip"
GO_CMD="go"
Copy link
Member

Choose a reason for hiding this comment

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

We no longer use gotip.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@serathius
Copy link
Member

Note, you could clean up the commit history? Looks like you have been pulling other commits as PR has multiple borrowed commits from other PRs. I think it would be best if we squash everything into one commit. Please let me know if you need any help.

@serathius serathius marked this pull request as ready for review October 14, 2022 07:41
Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

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

Overall looks good. We should be good to expand list of methods covered in future PR.

Would be great if we could ensure that any new method added is required to have a fuzz test.

@ahrtr
Copy link
Member

ahrtr commented Oct 14, 2022

cc @AdamKorcz I recall that you added some fuzz test a couple of months back. Could you take a look at this PR as well? Thanks.

@AdamKorcz
Copy link
Contributor

@ahrtr Thanks for the tag.

@samueleresca:

  1. Do these fuzz tests replace any of the ones running on OSS-Fuzz? https://github.com/cncf/cncf-fuzzing/tree/main/projects/etcd
  2. Could you also add these to the build script for OSS-Fuzz? https://github.com/cncf/cncf-fuzzing/blob/main/projects/etcd/build.sh

@serathius
Copy link
Member

serathius commented Oct 14, 2022

Hey @AdamKorcz, this is independent fuzzing to address one of the bugs in the api we found. I don't think it was reported by OSS fuzz. I proposed to implement fuzz to prevent future regressions.

Unfortunately the OSS-Fuzz was developed outside of the etcd repo, so they:

  • Were not reviewed by maintainers, we don't know the code, we don't know what is covered and what not.
  • Are apparently broken https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=51781
  • We cannot reliably maintain them as they are not integrated in the etcd CI and only maintainers have access to their reports.
  • Maintainers do not have capacity to maintain external project.

As so I would recommend that we migrate OSS-Fuzz code into the etcd repo. We can use this PR as the bases for the test. This should allow us to at test that the fuzz test build during CI.

strategy:
fail-fast: false
matrix:
Target: [FuzzRangeRequest, FuzzPutRequest, FuzzDeleteRangeRequest]
Copy link
Member

Choose a reason for hiding this comment

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

Each time when any contributor add a new fuzz case, then they need to update this file?

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. Unfortunately out-of-box fuzzing requires a specific target method to start the fuzzing. In case you try to use something like go test -fuzz Fuzz* the returned error is:

testing: will not fuzz, -fuzz matches more than one fuzz test: [FuzzRangeRequest FuzzPutRequest FuzzDeleteRangeRequest]

Let me know if there is a better way of doing that

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to get all targets defined in the scripts/fuzzing.sh for now, so that we can manually easily run all fuzz tests locally.

We can update the script/fuzzing.sh something like below,

TARGETS="FuzzRangeRequest  FuzzPutRequest  FuzzDeleteRangeRequest"
for target in ${TARGETS}; do
    run pushd ${target_path}
        go test -fuzz ${target}
    run popd
done

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 proceeded by updating the script as requested. Note that, the fuzzing for the different targets won't run in parallel. The fuzz process will start from FuzzRangeRequest and finishing with FuzzDeleteRangeRequest with the specified timeout


_, _, err := txn.Txn(ctx, zaptest.NewLogger(t), request, false, s, &lease.FakeLessor{})
if err != nil {
t.Logf("Check: %s | Apply: %s", errCheck, err)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we fail the test?

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 goal of the test is to verify that the Tnx function won't panic in case the validation check passes. it is okay in this context if the Tnx handles the request with an error. In case of panic, the test should fail.

Copy link
Member

Choose a reason for hiding this comment

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

OK.

@ahrtr
Copy link
Member

ahrtr commented Oct 14, 2022

Note: Golang supports fuzzing starting from 1.18,
https://go.dev/security/fuzz/

@samueleresca
Copy link
Contributor Author

Note: Golang supports fuzzing starting from 1.18, https://go.dev/security/fuzz/

I'm already using the out-of-the-box fuzzing coming from golang

@ahrtr
Copy link
Member

ahrtr commented Oct 16, 2022

I see lots of unrelated commits in this PR, please rebase this PR.

Samuele Resca and others added 4 commits October 23, 2022 13:46
Signed-off-by: Samuele Resca <sr7@ad.datcon.co.uk>
Signed-off-by: Samuele Resca <samuele.resca@gmail.com>
Signed-off-by: Samuele Resca <sr7@ad.datcon.co.uk>
Signed-off-by: Samuele Resca <samuele.resca@gmail.com>
Signed-off-by: Samuele Resca <samuele.resca@gmail.com>
Signed-off-by: Samuele Resca <samuele.resca@gmail.com>
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you @samueleresca

Comment on lines +11 to +17
for target in ${TARGETS}; do
log_callout -e "\\nExecuting fuzzing with target ${target} in $target_path with a timeout of $fuzz_time\\n"
run pushd "${target_path}"
$GO_CMD test -fuzz "${target}" -fuzztime "${fuzz_time}"
run popd
log_success -e "\\COMPLETED: fuzzing with target $target in $target_path \\n"
done
Copy link
Member

Choose a reason for hiding this comment

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

Minor comment: It seems that we only need to execute pushd and popd once outside the for loop.

Of course, it isn't a big deal, feel free to fix it in a separate PR.

@ahrtr ahrtr merged commit 0d46a6e into etcd-io:main Oct 24, 2022
@ahrtr ahrtr mentioned this pull request Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Ensure that input validation between API and Apply is consistent
5 participants