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

Spurious false verification failures with no error message with wrapper validation #57

Closed
big-guy opened this issue Apr 6, 2022 · 10 comments

Comments

@big-guy
Copy link
Member

big-guy commented Apr 6, 2022

We've had a few reports of a failure to verify valid wrappers:

https://github.com/jMonkeyEngine/jmonkeyengine/runs/5841232153

✗ Found unknown Gradle Wrapper JAR files:
e996d452d2645e70c01c11143ca2d3742734a28da2bf61f25c82bdc288c9e637 gradle/wrapper/gradle-wrapper.jar

This is a valid wrapper checksum. There aren't any other error message in the log, but I'm guessing this has something to do with us failing to download certain checksums.

From my understanding of how this action works, we're downloading all of the wrapper checksums every time we run the check: https://github.com/gradle/wrapper-validation-action/blob/master/src/checksums.ts#L28

This is nearly 200 checksums, but only 65 are unique.

@big-guy
Copy link
Member Author

big-guy commented Apr 6, 2022

@eskatos I think this is going to get worse as we release more versions of Gradle.

Would it make sense for us to embed the wrapper checksums for previously released versions of Gradle in the action, so we don't have to download them again? They should never change.

Or we could change the way we advertise the checksums on services.gradle.org so that we only change the URL when the wrapper actually changes? We could then only download the unique URLs.

Or we could change the way the GH action checks the checksum by only getting the checksum when we need it? IOW, instead of immediately getting all of the checksums and checking the local wrapper checksum against that set, we would go through each checksum, download it and compare it against the local wrapper. If we start from the most recent Gradle releases first, we might not have to download all of the checksums. We would still end up downloading duplicates when the wrapper hasn't changed in many releases.

Or we could add a different endpoint to services.gradle.org that's just a flat list of all of the checksums so there's only ever one list that needs to be downloaded?

WDYT?

@octylFractal
Copy link
Member

Or we could add a different endpoint to services.gradle.org that's just a flat list of all of the checksums so there's only ever one list that needs to be downloaded?

I vote for this, or an endpoint that simply validates if a wrapper checksum is correct or not, and tells you what version it came from. It's more expensive server wise, but we wouldn't have scaling issues in the action itself anymore.

@eskatos
Copy link
Member

eskatos commented Apr 6, 2022

Yeah, my favorite option would be to expose a single resource that contains all required checksums. The action would then do a single GET. I think the server load could be reduced too with some caching and the smaller number of requests.

@leonard84
Copy link
Member

We already have a list of all checksums in html form https://gradle.org/release-checksums/ should be easy to publish a json variant.

@big-guy
Copy link
Member Author

big-guy commented Apr 6, 2022

gradle.org/release-checksums is actually a different copy of the same data. Maybe we could just start exposing that on services.gradle.org and get rid of the copy from gradle.org.

@big-guy big-guy changed the title Spurious false verification failures with no error message Spurious false verification failures with no error message with wrapper validation May 9, 2022
@bgalek
Copy link

bgalek commented May 24, 2022

creating a github workflow in this repository that will tirgger on gradle release github event should be quite easy. Then we could save all the hashes into file and make an automated commit with next-version of this action (patchfix). Benefits:

  • no network calls
  • automated updates

I would be happy to make such PR if you want ;)

@ILikeYourHat
Copy link

I would like to bump this issue. We are observing this error at least once a week, many builds at once, usually happens during night. You wake up early in the morning, just to see multiple red dots with false-positives. What's the point of this action, when I started to have trust issues towards it?

@bgalek mentioned a great solution to this problem. Local definition of hashes will solve almost a half of the currently opened issues. And downloading definitions from remote could still stay as a fallback mechanism. Wdyt?

rafalh pushed a commit to rafalh/wrapper-validation-action that referenced this issue Feb 23, 2023
`HttpClient` in `typed-rest-client` does not throw for error status codes so it should be checked manually. 
Client follows redirects by default (status code 3xx) so I think comparing with status 200 should be good.
It may improve situation for people affected by gradle#57
@big-guy
Copy link
Member Author

big-guy commented Oct 30, 2023

@ljacomet is this something we could address with release coordination?

@JLLeitschuh
Copy link
Contributor

It would be awesome if you could also include checksums for snapshots too. That way people testing the bleeding edge of Gradle releases could also ensure their Gradle version is legitimate

@bigdaz
Copy link
Member

bigdaz commented Feb 7, 2024

Should be fixed by #161

@bigdaz bigdaz closed this as completed Feb 7, 2024
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

No branches or pull requests

8 participants