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

cog: update checksum #162013

Merged
merged 2 commits into from
Feb 9, 2024
Merged

cog: update checksum #162013

merged 2 commits into from
Feb 9, 2024

Conversation

stefanb
Copy link
Member

@stefanb stefanb commented Feb 7, 2024

It seems v0.9.4 it was re-tagged again after #160886, similar to

This affects

cc: @mattt, @zeke: can you confirm that retagging was done on 0.9.4 ?


  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

It seems v0.9.4 it was re-tagged again, similar to
* Homebrew#158001

This affects
* Homebrew#161976
* Homebrew#157782
@github-actions github-actions bot added the go Go use is a significant feature of the PR or issue label Feb 7, 2024
@stefanb stefanb mentioned this pull request Feb 7, 2024
1 task
@chenrui333 chenrui333 added upstream issue An upstream issue report is needed checksum mismatch SHA-256 doesn't match the download CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Feb 7, 2024
@chenrui333
Copy link
Member

@stefanb can you also file an upstream issue on this? Thanks!

@stefanb
Copy link
Member Author

stefanb commented Feb 7, 2024

Created upstream ticket

@zeke
Copy link

zeke commented Feb 7, 2024

can you confirm that retagging was done on 0.9.4 ?

I'm not sure about this one. I'll defer to @mattt and @technillogue 🙏🏼

@mattt
Copy link
Contributor

mattt commented Feb 7, 2024

@stefanb @chenrui333 Hey, sorry that Cog seems to be causing problems for y'all.

As far as I can tell, this wasn't an intentional change like #158001.

I just looked into this and can't account for why we're seeing a checksum mismatch.

  • We have GitHub protections for v* tags (active for 8 months), so 0.9.4 wasn't retagged.
  • The release was created by this action run, and both have matching timestamps of Jan 24, 2024 at 2:17 PM PST.
  • The source artifacts for that release have a timestamp of "Jan 24, 2024 at 1:58 PM PST", which matches the timestamp of the tagged commit.
  • The binary artifacts were also generated at Jan 24, 2024 at 2:17 PM PST, which corresponds with the timing of the action run.
  • cog 0.9.4 #160886 was opened Jan 24, 2024 at 4:38 PM, which looks right.

Any ideas about what might be happening?

@stefanb stefanb mentioned this pull request Feb 7, 2024
@chenrui333
Copy link
Member

chenrui333 commented Feb 8, 2024

Looked at the action run against the release commit (v0.9.4), I did not find anything weird either. So weird.

I would suggest let's move forward and it is happening again, we can involve with github team to find out why.

chenrui333
chenrui333 previously approved these changes Feb 8, 2024
@chenrui333 chenrui333 added the ready to merge PR can be merged once CI is green label Feb 8, 2024
Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

I'm not okay with "let's ignore the potentially catastrophic security problem"

@SMillerDev SMillerDev requested a review from a team February 8, 2024 12:20
@stefanb
Copy link
Member Author

stefanb commented Feb 8, 2024

I was also checking the workflow to see if it is committing a version number and retagging or some such tricks, but couldn't spot anything obvious.

@chenrui333
Copy link
Member

I'm not okay with "let's ignore the potentially catastrophic security problem"

@SMillerDev I dont see anything obviously wrong, and nor the maintainers can find anything. What is your suggestion then?

@chenrui333 chenrui333 added maintainer feedback Additional maintainers' opinions may be needed and removed ready to merge PR can be merged once CI is green labels Feb 8, 2024
@p-linnane
Copy link
Member

p-linnane commented Feb 8, 2024

This is the exact scenario this check is meant to catch, and possibly one of the most concerning one's we've seen in some time since upstream doesn't seem to know why this changed. This PR should not be merged until it's determined what happened here by the upstream maintainers. I'm also strongly against merging any version bumps for this software until this is resolved. Homebrew will not willingly accept a change that upstream can't account for due to he incredibly real threat of a supply chain attack. My hope is that the upstream maintainers are incredibly concerned by this and going to investigate until a cause is found.

@woodruffw
Copy link
Member

Triaging: We're coming up on a year past https://github.blog/2023-02-21-update-on-the-future-stability-of-source-code-archives-and-hashes/, although not quite (GH made a commitment not to change these hashes until 2024-02-21). So it's possible this is an accident on their side. However, I strongly agree with @p-linnane that we need to treat this as malicious until we confirm that 🙂

@stefanb
Copy link
Member Author

stefanb commented Feb 8, 2024

So this means that in such cases homebrew should fallback to git commit hash and if that matches it should update the archive hash in the formula?

@SMillerDev
Copy link
Member

Most of all we need to find out why it changed. Once that is known we can determine how to move forward.

@Bo98
Copy link
Member

Bo98 commented Feb 8, 2024

This isn't a GitHub change.

One can reproduce how a git tarball works via:

$ brew install gzip
$ git checkout main
$ git -c tar.tgz.command="$(brew --prefix gzip)/bin/gzip -cn" archive v0.9.4 --prefix=cog-0.9.4/ -o test.tgz
$ shasum -a 256 test.tgz
5f455da636ec6dd6c81fd46fb721e261da1912ec42e2c547496c9cc8bae78773  test.tgz

If you reset main back to be level with v0.9.4, you get the original sha256:

$ git checkout main
$ git reset --hard v0.9.4
$ git -c tar.tgz.command="$(brew --prefix gzip)/bin/gzip -cn" archive v0.9.4 --prefix=cog-0.9.4/ -o test.tgz
$ shasum -a 256 test.tgz      
d3df9a9b9af79f6309f3acf31d6a3363270c0a5a7838ee75d4a02a8ed92fb0ae  test.tgz

Why? This is because:

--- a/python/.git_archival.txt
+++ b/python/.git_archival.txt
@@ -1,4 +1,4 @@
 node: d25544500937e9428ffaa0e9e4b37f763b7f7694
 node-date: 2024-01-24T13:58:15-08:00
 describe-name: v0.9.4
-ref-names: tag: v0.9.4
+ref-names: HEAD -> main, tag: v0.9.4

The %D format used in https://github.com/replicate/cog/blob/a81c7a4338be7596e64f3c3efe363852d6a4193d/python/.git_archival.txt#L4 is not a stable format and can change depending on HEAD.

ref-names looks like it is actually only necessary to have if using a Git older than 2.32 - describe-name is used instead on modern Git. I'm not sure if that's a concern for cog or not, but removing ref-names could be the fix here. An alternative would be to upload your own stable source archive on releases.

@p-linnane
Copy link
Member

Big thank you @Bo98 for sorting this out. @mattt & @zeke, is this something you'll be able to resolve upstream? Otherwise we will continue to see this type of mismatch and breakage.

@zeke
Copy link

zeke commented Feb 8, 2024

Thanks for digging into that @Bo98. I've opened a PR to try to resolve this: replicate/cog#1522

@stefanb
Copy link
Member Author

stefanb commented Feb 9, 2024

Will merging replicate/cog#1522 only affect the hashes of the future cog release archives or also the existing ones (eg current 0.9.4)?

@chenrui333
Copy link
Member

Thank you, @Bo98, for the throughput check.

@nickstenning
Copy link
Contributor

nickstenning commented Feb 9, 2024

This isn't a GitHub change.

I think I'm missing some context here. You say it's not a GitHub change, but what process created the diff for this pull request? Given that we don't upload our own source tarballs, it seems like there are two possibilities:

  1. It's directly the hash of the GitHub-generated tarball, i.e. https://github.com/replicate/cog/archive/refs/tags/v0.9.4.tar.gz
  2. The archive is created as @Bo98 indicated by Homebrew infrastructure.

I'm guessing it's (1). If so, doesn't that mean that it is a GitHub change? Because they switched from generating the archive on main with the tag's index + working copy, to now generating the archive on main with main's working copy?

For reference:

Generating on main

$ git checkout main
Already on 'main'
Your branch is up to date with 'origin/main'.
$ git reset --hard main
HEAD is now at 36a94ea Update deprecated list type usage (#1327)
$ git -c tar.tgz.command="$(brew --prefix gzip)/bin/gzip -cn" archive v0.9.4 --format=tgz --prefix=cog-0.9.4/  | openssl sha256
SHA2-256(stdin)= 5f455da636ec6dd6c81fd46fb721e261da1912ec42e2c547496c9cc8bae78773
$ git -c tar.tgz.command="$(brew --prefix gzip)/bin/gzip -cn" archive v0.9.4 --format=tgz --prefix=cog-0.9.4/  | tar -O -x cog-0.9.4/python/.git_archival.txt
node: d25544500937e9428ffaa0e9e4b37f763b7f7694
node-date: 2024-01-24T13:58:15-08:00
describe-name: v0.9.4
ref-names: tag: v0.9.4

Generating on main with index + working copy from v0.9.4

$ git checkout main
$ git reset --hard v0.9.4
HEAD is now at d255445 fix: ignore "train" part of schema errors during predict runs in production for backward compatibility (#1496)
$ git -c tar.tgz.command="$(brew --prefix gzip)/bin/gzip -cn" archive v0.9.4 --format=tgz --prefix=cog-0.9.4/  | openssl sha256
SHA2-256(stdin)= d3df9a9b9af79f6309f3acf31d6a3363270c0a5a7838ee75d4a02a8ed92fb0ae
$ git -c tar.tgz.command="$(brew --prefix gzip)/bin/gzip -cn" archive v0.9.4 --format=tgz --prefix=cog-0.9.4/  | tar -O -x cog-0.9.4/python/.git_archival.txt
node: d25544500937e9428ffaa0e9e4b37f763b7f7694
node-date: 2024-01-24T13:58:15-08:00
describe-name: v0.9.4
ref-names: HEAD -> main, tag: v0.9.4

@nickstenning
Copy link
Contributor

nickstenning commented Feb 9, 2024

Looking into this I'm ever more convinced that this is a change in how GitHub generates the tarballs.

According to the history of cog.rb:

  1. The tarball for v0.9.3 had a SHA256 hash of bb3329b6c7daeef166501883946231a58675770ba624e1450c4be00b549be77a
  2. The tarball for v0.9.2 had a SHA256 hash of 853befc8934b44e2b1ae6088aad16cc4ad9d5c22f19ed754fc576c0f64aed21f.

Neither of those hashes are correct any more:

~ $ curl -SsL -o- https://github.com/replicate/cog/archive/refs/tags/v0.9.3.tar.gz | openssl sha256
SHA2-256(stdin)= 5a97356b165a8be41241e0ffc9d769c594166ef8b4d537301f7f87af4d7184b8
~ $ curl -SsL -o- https://github.com/replicate/cog/archive/refs/tags/v0.9.2.tar.gz | openssl sha256
SHA2-256(stdin)= ea97dd086b100bb804c99ddaed8d352a6d276ca6e606f5410138feeaa201c0c0

So while the solution here for us is to change our export-subst configuration to use a more stable output, the problem here was created by GitHub yet again changing the data in the automatically generated tarballs, albeit this time in a way that affects fewer people -- namely those who have export-subst configurations that are sensitive to (e.g.) Git version.

@nickstenning
Copy link
Contributor

Aha! Maybe I'm wrong. What appears to matter here is whether HEAD and the treeish being archived are the same or not.

So it's possible this has always been broken, and when the latest commit on main is the tag commit, it generates one output (the ref-names: tag: v0.9.4 variant), and as soon as one more commit is pushed to main, the output changes to ref-names: HEAD -> main, tag: v0.9.4.

If that's the case, then this problem will have existed for every cog release for quite some time.

Either way, replicate/cog#1522 will fix this for future releases.

@Bo98
Copy link
Member

Bo98 commented Feb 9, 2024

Aha! Maybe I'm wrong. What appears to matter here is whether HEAD and the treeish being archived are the same or not.

So it's possible this has always been broken, and when the latest commit on main is the tag commit, it generates one output (the ref-names: tag: v0.9.4 variant), and as soon as one more commit is pushed to main, the output changes to ref-names: HEAD -> main, tag: v0.9.4.

If that's the case, then this problem will have existed for every cog release for quite some time.

Either way, replicate/cog#1522 will fix this for future releases.

This is correct, yes. It's not a GitHub change as it has always worked this way. Because cog generally had a fairly quick release cycle, a new release often came out before anyone even notices. We would generally only notice a cog checksum failure on the larger CI tests we do with new minor Go releases twice a year. It was noticed in the gap between 0.8.6 and 0.9.0 but that coincidentally could be explained for other reasons: #158001.

@nickstenning
Copy link
Contributor

Right, thank you for confirming. We'll make sure that replicate/cog#1522 merges before we cut any new releases. For now I'd say this PR is unblocked and the correct thing to do.

@nickstenning
Copy link
Contributor

Upstream fix is now merged. Hopefully between this and us enabling tag protection this shouldn't ever happen again.

@Bo98 Bo98 removed the maintainer feedback Additional maintainers' opinions may be needed label Feb 9, 2024
Copy link
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

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

Thank you all for sorting this out, and being understanding with our need to be extremely cautious in these situations given the footprint of Homebrew.

@p-linnane p-linnane removed the upstream issue An upstream issue report is needed label Feb 9, 2024
Copy link
Contributor

github-actions bot commented Feb 9, 2024

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Feb 9, 2024
@BrewTestBot BrewTestBot added this pull request to the merge queue Feb 9, 2024
Merged via the queue into Homebrew:master with commit 5f76121 Feb 9, 2024
12 checks passed
@stefanb stefanb deleted the patch-1 branch February 10, 2024 05:25
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 12, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
checksum mismatch SHA-256 doesn't match the download CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. go Go use is a significant feature of the PR or issue outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants