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

pyjwt: improves code coverage #9832

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jesslatimer
Copy link
Contributor

This change adds;

  • a fuzz corpus
  • fuzzing for jwk
  • each function consumes own data

@google-cla
Copy link

google-cla bot commented Feb 28, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jesslatimer jesslatimer marked this pull request as draft February 28, 2023 22:20
@jesslatimer jesslatimer marked this pull request as ready for review March 1, 2023 20:49
@jesslatimer jesslatimer force-pushed the master branch 2 times, most recently from c3e60a2 to 38570d6 Compare March 20, 2023 23:42
@jonathanmetzman
Copy link
Contributor

@DavidKorczynski do you approve?

Copy link
Collaborator

@DavidKorczynski DavidKorczynski left a comment

Choose a reason for hiding this comment

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

I think in general when we go for making changes in existing fuzzers we should try to make it so existing seeds do not exhibit different behaviour. This PR and e.g #9967 does that -- could you perhaps address this (just in this PR not the ones that's merged) @jesslatimer ?

"""Check payload == decoded(encoded(payload)) using RS256"""
try:
payload = json.loads(data)
keyfile = json.loads(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make payload and keyfile different (i.e different bytes for json.loads).

@jesslatimer
Copy link
Contributor Author

I think in general when we go for making changes in existing fuzzers we should try to make it so existing seeds do not exhibit different behaviour. This PR and e.g #9967 does that -- could you perhaps address this (just in this PR not the ones that's merged) @jesslatimer ?

Not a problem at all, but could you please clarify what you would like changed @DavidKorczynski? Is the payload / keyfile an unrelated change? Sorry, still learning the ins and outs of fuzz testing.

@DavidKorczynski
Copy link
Collaborator

Not a problem at all, but could you please clarify what you would like changed @DavidKorczynski? Is the payload / keyfile an unrelated change? Sorry, still learning the ins and outs of fuzz testing.

The main point is to ensure a state where if the data buffer to the fuzzer entrypoint is the same as before this modification then the fuzzer should at least exercise the same code as before this modification. "At least the same" means that the code of the existing fuzzer should execute as before plus any additional new code. The reason for this is that otherwise existing bugs will not be reproducible and the existing corpus is likely not valid anymore.

The change in this fuzzer modification that causes this friction is that given the same data as prior to this modification, test_decoding and test_roundtrip will behave differently. Leaving the logic of those functions and how they're called and then using a similar strategy e.g. test_roundtrip_with_RS256(data) for the new targets would work well.

This change adds;
- a fuzz corpus
- fuzzing for jwk
- each function consumes own data

Signed-off-by: Jessica Latimer <[email protected]>
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