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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Artifacts] Save md5 hash for each artifact upload #1494

Merged
merged 5 commits into from
Aug 15, 2023

Conversation

konradpabjan
Copy link
Contributor

Tracking: https://github.com/github/c2c-actions-checks/issues/1699

This builds up on the work that was done in #1479, #1481, #1486 and #1487 and #1488

We want to save a hash of the artifact upload for integrity. Unfortunately the response from uploadStream does not have an MD5 hash value. There is a crc64 value in the raw headers but it doesn't appear to be officially documented in the azure blob NPM package so we're not going to try to use it as a result.

We can't also call blobClient.properties to get a hash after the upload finished since the token only has permissions to create and write, not read.

I'm using a solution that I found here. We pipe the zip stream to a crypto stream on the side and after the upload finishes we can read the hash and save if with the finalize call. When we get to download artifact we can then return this value and do integrity checks and a host of other things.

Here is an example run with the code running E2E after doing npm link and setting everything up with a fork that I've been using for testing: https://github.com/bbq-beets/testing-artifacts-v4/actions/runs/5869253502/job/15913849975

image

I verified afterwards if the hash of the zip is correct by downloading the artifact and manually inspecting it and things work 馃憤

@konradpabjan
Copy link
Contributor Author

Failing tests are unrelated, just like on my last PR. See #1488 (comment)

@konradpabjan konradpabjan marked this pull request as ready for review August 15, 2023 16:30
@konradpabjan konradpabjan requested a review from a team as a code owner August 15, 2023 16:30
Copy link
Contributor

@bethanyj28 bethanyj28 left a comment

Choose a reason for hiding this comment

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

Functionality looks good! Just had some logging nits!


hashStream.end()
md5Hash = hashStream.read() as string
core.info(`MD5 hash of uploaded artifact zip is ${md5Hash}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, will printing the hash get a little noisy? Maybe we could make this a debug log?

Copy link
Contributor Author

@konradpabjan konradpabjan Aug 15, 2023

Choose a reason for hiding this comment

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

I'd prefer to at last initially keep this in the logs. We're not going to immediately expose the hash via our REST APIs for artifacts unless we do more work so there is no real way for users to get this information unless we log it. When we expose this potentially in our APIs then we could make this debug but for now it's good information to display

packages/artifact/src/internal/upload/blob-upload.ts Outdated Show resolved Hide resolved
packages/artifact/src/internal/upload/upload-artifact.ts Outdated Show resolved Hide resolved
@konradpabjan konradpabjan merged commit c9dab8c into main Aug 15, 2023
7 of 10 checks passed
@konradpabjan konradpabjan deleted the konradpabjan/upload-artifact-md5 branch August 15, 2023 17:40
at-wat pushed a commit to at-wat/actions-toolkit that referenced this pull request Aug 31, 2023
* Hash artifact upload using md5

* Add imports

* Small tweaks

* PR feedback

* PR Feedback
@AnnAngela
Copy link

@konradpabjan Sorry for commenting here, but I found out that this pr introduced a new dependency "crypto" in https://github.com/actions/toolkit/pull/1494/files#diff-3a2287638a98ab8c93525fe63e6e7e35a2e3be09bd2e4db80173c68ed83c72f8R47, which is marked as deprecated and hold nothing for use. Maybe this dependency can be removed?

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.

None yet

4 participants