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

Drop the iso_checksum_type & iso_checksum_url fields #8437

Merged
merged 61 commits into from
May 28, 2020

Conversation

azr
Copy link
Contributor

@azr azr commented Dec 2, 2019

In favor of simply using iso_checksum that will know what to do.

In favor of simply using iso_checksum that will know what to do.
@rgl
Copy link
Contributor

rgl commented Jan 6, 2020

basing this on the hash length is not correct because there are several hash functions that can have the same output length, e.g., SHA-2 256 vs SHA-3 256.

@azr
Copy link
Contributor Author

azr commented Feb 10, 2020

Hey @rgl, thanks for your concern; I nerd sniped myself a little to check some of what you said. So basically what I found out is that sha-256 is currently at sha-4 and previous versions of sha ( sha-1, 2, 3 ) have been 'deprecated' and by default go implements "sha-04 - 256" from FIPS "180-04" : https://golang.org/src/crypto/sha256/sha256.go?s=3821:3841#L6 FIPS-04.

So that's one thing; but this doesn't prevent hashing algorithms from having the same length; but it is still possible to tell the go-getter which algorithm you want from the hashing string, for example:

By using something like ./foo.txt?checksum=md5:b7d96c89d09d9e204f5fedc4d5d55b21 and a file containing checksums can also tell what checksum is being used, ( or not and then we will fallback to guessing based on length of hash ).

@rgl
Copy link
Contributor

rgl commented Feb 10, 2020

Hey @rgl, thanks for your concern; I nerd sniped myself a little to check some of what you said. So basically what I found out is that sha-256 is currently at sha-4 and previous versions of sha ( sha-1, 2, 3 ) have been 'deprecated' and by default go implements "sha-04 - 256" from FIPS "180-04" : https://golang.org/src/crypto/sha256/sha256.go?s=3821:3841#L6 FIPS-04.

Don't known what is this "sha-04 - 256" you are talking about. The hashing algorithm is called SHA-2 and the other one is SHA-3; both have variants with the same output length (e.g. 256-bit).

So that's one thing; but this doesn't prevent hashing algorithms from having the same length; but it is still possible to tell the go-getter which algorithm you want from the hashing string, for example:

By using something like ./foo.txt?checksum=md5:b7d96c89d09d9e204f5fedc4d5d55b21 and a file containing checksums can also tell what checksum is being used, ( or not and then we will fallback to guessing based on length of hash ).

Ah, so I can use, e.g., iso_checksum = 'sha3-256:xxxx', so its all good :-)

@azr azr added this to the 1.6.0 milestone Apr 30, 2020
@azr
Copy link
Contributor Author

azr commented May 19, 2020

Okay I had to update the tests but this looks good to review ; basically foo is not a valid base64 encoded string ( because it contains an 'o' ) and therefore is invalid; so I used 0B0F137F17AC10944716020B018F8126 (or 0b0F137F17AC10944716020B018F8126 ) everywhere, this comes from md5("packer")

Copy link
Contributor

@sylviamoss sylviamoss left a comment

Choose a reason for hiding this comment

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

Looks really good to me :D

// "md5:{$checksum}", "sha1:{$checksum}", "sha256:{$checksum}",
// "sha512:{$checksum}" or "file:{$path}". Here is a list of valid checksum
// values:
// * md5:090992ba9fd140077b0661cb75f7ce13
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!!

CHANGELOG.md Outdated Show resolved Hide resolved
}

return true
req := &getter.Request{
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!!!

} else if c.ISOChecksum != "" {
q := u.Query()
q := u.Query()
if c.ISOChecksum != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

at this point this is always true, so I guess you can remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is also a possibility that the checksum is set from the iso url.

@@ -28,13 +24,11 @@ require (
github.com/antchfx/xquery v0.0.0-20170730121040-eb8c3c172607 // indirect
github.com/approvals/go-approval-tests v0.0.0-20160714161514-ad96e53bea43
github.com/armon/go-metrics v0.0.0-20190430140413-ec5e00d3c878 // indirect
github.com/aws/aws-sdk-go v1.25.41
github.com/aws/aws-sdk-go v1.30.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading their changelong it appeared that they had some changes to the ec2 API which we use but it doesn't say anything about backward incompatibilities and it wasn't too many changes. For me is ok updating the version, unless you have another concern.

@azr azr force-pushed the drop_iso_checksum_type_field branch from 85c9657 to 8e432c9 Compare May 26, 2020 14:26
@SwampDragons
Copy link
Contributor

Just noticed -- this needs a fixer.

Copy link
Contributor

@SwampDragons SwampDragons left a comment

Choose a reason for hiding this comment

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

We need a fixer for this! If there's a checksum_type, we need to delete it from the config and prepend it to the iso_checksum field.

@azr
Copy link
Contributor Author

azr commented May 28, 2020

Okay I think I can merge this one 🙂

@azr azr merged commit 0fa60c6 into master May 28, 2020
@azr azr deleted the drop_iso_checksum_type_field branch May 28, 2020 09:02
@azr
Copy link
Contributor Author

azr commented May 28, 2020

Ah bummer I wanted to change the commit message

@SwampDragons
Copy link
Contributor

This is probably the most major backwards incompatibility we've introduced in my time working on Packer. It will affect basically every template. And while packer fix feels like an easy lift for a power user, not all of our users even know that feature exists.

I'm wondering if there's a way we can automatically run fixers as part of a build if Prepare() fails, to recover the build.

@ghost
Copy link

ghost commented Jun 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jun 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants