-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fixed: Clarify algorithm choice requirements #296
Conversation
draft/spec/index.html
Outdated
algorithm recognizes that it has no known collision vulnerabilities and is less computationally | ||
intensive to compute than <code>sha256</code>, [[Stop-Using-SHA-256]]. | ||
For content-addressing, OCFL Objects SHOULD use <code>sha512</code>. The choice of the <code>sha512</code> | ||
digest algorithm as default recognizes that it is widely available in different implementations, has no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by:
in different implementations
I would be fine leaving the sentence beginning with "The choice of..." unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @awoods that it would be better without the added "widely available in different implementations,". If others feel that we need to say something about availability then I think we need to improve the wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention was to communicate that it's a safe choice -- not exotic, implemented in several languages and environments, etc. As opposed to, say, SHA-3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that is necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. and the reason I think that may be worthwhile to include is because there is an expectation that the defaults for OCFL are 'best-practice-worthy' for digital preservation. If you don't know MD5 from SHA-512, you may want to both have the recommendation (SHOULD) but also a brief paragraph about why. Otherwise it seems like it is an arbitrary decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were to add something then it should be the last item in the list of reasons (being fit for purpose is more important) and we need to make sure that there isn't ambiguity about which implementations we are talking about (OCFL or the algorithm). Could be something like "..., and multiple implementations are available." However, I'm still not sure it is needed. Other input would be good...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... also, wide availability of the hashing algorithm was a factor in our choosing of it. BLAKE2b is superior, but it's not as widely implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahankinson : I agree with the sentiment of including elements in the OCFL specification that are "best-practice-worthy"; however, it is less clear to me that we should feel compelled to state the rationale for each element of the spec that was chosen for that reason.
Maybe as a separate pull-request that adds a statement to the Introduction would be a comprehensive way of conveying this goal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Language could go into issue #186 and I can make the update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editors call: Decision to change to ", and multiple implementations are available".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to address #296 (review)
# Conflicts: # draft/spec/index.html
Fixes #291