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

Packages and checksums and codes #282

Closed
zvr opened this issue Apr 23, 2020 · 16 comments
Closed

Packages and checksums and codes #282

zvr opened this issue Apr 23, 2020 · 16 comments
Assignees
Milestone

Comments

@zvr
Copy link
Member

zvr commented Apr 23, 2020

Our current v2.2/development spec has:

Now, we also use something in:

Do we want to refer to PaVeCo (3.9) or to PaChksum (3.10) ? ;-)

  • IF 3.9, we should probably re-word it.
  • IF 3.10, we should change the wrong cross-link and possibly allow more than one, due to different algorithms.

I believe it's the latter; if we agree, I can provide a PR.

@zvr zvr added this to the 2.2 milestone Apr 23, 2020
@zvr
Copy link
Member Author

zvr commented Apr 23, 2020

Actually... maybe we actually mean File Checksum and we should point to 4.4.
An external document is a file, not a package. methinks...

@goneall
Copy link
Member

goneall commented Apr 23, 2020

@zvr Good catch!

Actually... maybe we actually mean File Checksum and we should point to 4.4.

Makes sense to me - I agree this needs to be fixed and I agree it is a file checksum and refer to section 4.4. Section 3.9 is clearly wrong since it is not a package verification code.

I just checked the implementation in the SPDX tools and verify checks to makes sure the algorithm is SHA1. The code is evidence that we probably agreed to limit the algorithm to SHA1 at some point even though it isn't clearly documented in the spec.

For 2.2 - I'm thinking we add specify SHA1, but I'm flexible on changing the tools to allow any of the checksum algorithms if others disagree.

@kestewart
Copy link
Contributor

@zvr - yes, agree, it is a file checksum that is being used here, please provide a pull request.
@goneall - we're heading toward accepting multiple checksums, and given that the examples in Appendix VI have SHA256, lets just accept multiple checksums.

@goneall
Copy link
Member

goneall commented Apr 28, 2020

I submitted issues to track updates for the Java tools:

spdx/tools#223
spdx/Spdx-Java-Library#11

I don't know if there are similar issues for Python, JavaScript or GoLang

@swinslow
Copy link
Member

@goneall Currently the Golang tools don't do any parsing of the External Document References checksum -- it just treats the entire value of that tag as a string:

https://github.com/spdx/tools-golang/blob/b68821f66a8d47c441e3da9ab059205e7e30b3f4/spdx/creation_info.go#L31

so there's more there to be fixed than just allowing the other checksum algorithms :)

@kestewart kestewart assigned kestewart and unassigned kestewart Apr 28, 2020
@kestewart
Copy link
Contributor

@nishakm - thanks for offering to take a pass at this.

@kestewart kestewart self-assigned this Apr 28, 2020
@nishakm
Copy link
Contributor

nishakm commented Apr 28, 2020

@kestewart was it agreed for 2.2 to accept multiple hashing algorithms for File Checksum?

@nishakm
Copy link
Contributor

nishakm commented Apr 28, 2020

PR submitted: #293

@kestewart
Copy link
Contributor

@nishakm - yup that's the direction we're going in, so may as well accept it here explicitly.

@kestewart
Copy link
Contributor

Fixed by #293.

@nishakm
Copy link
Contributor

nishakm commented Apr 29, 2020

@nishakm - yup that's the direction we're going in, so may as well accept it here explicitly.

Section 4.4 says SHA1 is mandatory and others are optional. I didn't change that content to read that any checksum can be used. Shall I submit a PR for that?

@kestewart
Copy link
Contributor

@nishakm - yes please. Good point. Maybe phrase along the lines of any of the checksum formats defined in section 4.4 can be used? Agree we don't want this to be mandatory SHA1

@swinslow
Copy link
Member

@nishakm @kestewart My recollection when previously discussed was that for 2.2 we wanted to leave mandatory SHA1 in place, for compatibility with 2.1 (so that it wouldn't be a "breaking change" for 2.1 tooling that expects SHA1 to be present).

I'd note also that file SHA1 values are needed for the current Package Validation Code algorithm. If they won't be mandatory then either the Package Validation Code algorithm would probably need to change, or else people might be unable to calculate or confirm whether it is accurate.

I'm all kinds of in favor of changing this, but I think it was seen previously as more of a 3.0 change rather than 2.2.

@swinslow
Copy link
Member

Haha, yes, turns out there's an open issue for 3.0 about this that we've all commented on -- see #106 :)

@nishakm
Copy link
Contributor

nishakm commented Apr 29, 2020

@swinslow That was my understanding too :D. No PR then.

@goneall
Copy link
Member

goneall commented Apr 29, 2020

Thanks goodness Github issues have a better memory than I do ;)

I'll revert the changes to the tools to make SHA1 mandatory again.

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

5 participants