Handle remote checksum edge cases

[iker-barriocanal]

The GitHub target makes some assumptions to check whether the local and the remote checksums of an asset match, but the code doesn't handle when these assumptions aren't correct. See

craft/src/targets/github.ts

Lines 381 to 407 in e4f9962

	// XXX: This is a bit hacky as we rely on two things:
	// 1. GitHub issuing a redirect to S3, where they store the artifacts,
	// or at least pass those request headers unmodified to us
	// 2. AWS S3 using the MD5 hash of the file for its ETag cache header
	// when we issue a HEAD request.
	const response = await this.github.request(`HEAD ${url}`, {
	headers: {
	// WARNING: You **MUST** pass this accept header otherwise you'll
	// get a useless JSON API response back, instead of getting
	// redirected to the raw file itself.
	// And don't even think about using `browser_download_url`
	// field as it is close to impossible to authendicate for
	// that URL with a token and you'll lose hours getting 404s
	// for private repos. Consider yourself warned. --xoxo BYK
	Accept: DEFAULT_CONTENT_TYPE,
	},
	});
	// ETag header comes in quotes for some reason so strip those
	const remoteChecksum = response.headers['etag']?.slice(1, -1);
	const localChecksum = createHash('md5').update(file).digest('hex');
	if (localChecksum !== remoteChecksum) {
	logger.trace(`Checksum mismatch for "${name}"`, response.headers);
	throw new Error(
	`Uploaded asset MD5 checksum does not match local asset checksum for "${name} (${localChecksum} != ${remoteChecksum})`
	);
	}

Update: The ETag header isn't always present in the response, and this is making all releases using the github target to fail (example).

[BYK]

What are the assumptions? If we cannot get a checksum we should assume there's an error or retry?

[iker-barriocanal]

The assumptions are these in lines 382-385. I don't know what the behavior should be since I don't know why exactly it's happening, but if retrying helps to reduce that error, it's a strong alternative. One of the assumptions is we get the hash in the header, so if we don't we probably want to error, yes.

It seems this has happened again in this release, see error.

[iker-barriocanal]

Update: The ETag header isn't always present in the response, and this is making all releases using the github target to fail.

A potential solution is to download the assets when ETag isn't in the headers and check the hash locally.

[chadwhitacre]

I'm seeing an AWS facility for providing a hash when uploading, but we aren't uploading directly, GitHub is doing it for us. Looks like there's another lurking bug in that files over 5GB have their ETag computed differently. I'm also finding this caveat under ETag on Common Response Headers:

The ETag may or may not be an MD5 digest of the object data. Whether or not it is depends on how the object was created and how it is encrypted as described below:

• Objects created through the AWS Management Console or by the PUT Object, POST Object, or Copy operation:
  • Objects encrypted by SSE-S3 or plaintext have ETags that are an MD5 digest of their data.
  • Objects encrypted by SSE-C or SSE-KMS have ETags that are not an MD5 digest of their object data.
• Objects created by either the Multipart Upload or Part Copy operation have ETags that are not MD5 digests, regardless of the method of encryption.

Here's a slightly old Python implementation of this algorithm.

The ETag header isn't always present in the response, and this is making all releases using the github target to fail.

Making all releases to fail, or just those without an ETag?

Which response is the ETag not present in? We are depending on a redirect from GitHub:

craft/src/targets/github.ts

Lines 381 to 385 in e4f9962

	// XXX: This is a bit hacky as we rely on two things:
	// 1. GitHub issuing a redirect to S3, where they store the artifacts,
	// or at least pass those request headers unmodified to us
	// 2. AWS S3 using the MD5 hash of the file for its ETag cache header
	// when we issue a HEAD request.

Are we sure that GitHub is issuing the redirect as expected? I would be somewhat surprised if AWS sometimes includes and ETag and sometimes doesn't. My hunch is that it is the redirect from GitHub that is breaking. Can we turn on trace logging to verify?

craft/src/targets/github.ts

Line 403 in e4f9962

logger.trace(`Checksum mismatch for "${name}"`, response.headers);

[chadwhitacre]

I kicked out a quick workaround in #323 to get us unblocked, since SDK deployments are getting backed up behind this issue. Thoughts?

[marandaneto]

this still happens on https://github.com/getsentry/craft/releases/tag/0.27.2 sometimes, retrying works tho, but it's flaky.

[iker-barriocanal]

@marandaneto, could you link to a release that failed for this reason? I've only seen sentry-unity v0.7.0 failing since craft v0.27.2, and it doesn't look like it's this issue (it's probably github?).

[marandaneto]

@marandaneto, could you link to a release that failed for this reason? I've only seen sentry-unity v0.7.0 failing since craft v0.27.2, and it doesn't look like it's this issue (it's probably github?).

this happened a few times using craft locally, with the maven target, so unfortunately no logs right now, @romtsn also experienced it.

[romtsn]

I think 0.27.2 fixed this for me