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

Making the RCH hash function reference more explicit #70

Merged
merged 6 commits into from
Aug 28, 2024

Conversation

iherman
Copy link
Member

@iherman iherman commented Aug 20, 2024

This PR proposes the changes described in #69.


Preview | Diff

index.html Outdated
SHA-2 with 256 bits of output is utilized. For P-384 keys, SHA-2 with 384-bits
of output is utilized.
When the RDF Dataset Canonicalization Algorithm (RDFC-1.0) [[RDF-CANON]] is used with ECDSA
algorithms, the cryptographic hashing function used by RDFC-1.0 MUST depend on the size of
Copy link
Contributor

Choose a reason for hiding this comment

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

This first MUST seems a bit awkward now that the following sentences have the more clarifying and more obviously testable MUSTs in them. Can we do this?:

Suggested change
algorithms, the cryptographic hashing function used by RDFC-1.0 MUST depend on the size of
algorithms, the cryptographic hashing function used by RDFC-1.0 depends on the size of

Approving either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dlongley, can you re-review? I have incorporated @TallTed's change proposal, and it may cover your comment as well...

Copy link
Contributor

Choose a reason for hiding this comment

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

It does read a bit better now, but I still think the first MUST is unnecessary because it isn't specific -- it's the following two sentences that make the specific asks of implementations. I'd still change it from "MUST depend" to just "depends".

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea what happened, but it seems as @TallTed's change proposals, that I thought I accepted, did not make it into the branch (I may have pushed the "resolve" button too quickly.) I re-did his editings manually.

To avoid misunderstandings, this is the text now:

When the RDF Dataset Canonicalization Algorithm (RDFC-1.0) [[RDF-CANON]] is used
with ECDSA algorithms, the cryptographic hashing function used by RDFC-1.0 MUST
be chosen based on the size of the associated public key. For P-256 keys, the
default hashing function, SHA-2 with 256 bits of output, MUST be used. For P-384
keys, SHA-2 with 384-bits of output MUST be used, specified via the RDFC-1.0
implementation-specific parameter.

In this text I believe all MUST-s are justified. @dlongley, can you look at it again?

(Sorry for my clumsiness...)

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries! I left a suggestion and an explanation for it. I approve either way, but I think my suggestion improves things from a testability perspective.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
@msporny msporny added the CR1 label Aug 28, 2024
@msporny
Copy link
Member

msporny commented Aug 28, 2024

Editorial, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny merged commit 3989be4 into main Aug 28, 2024
2 checks passed
@msporny msporny deleted the RCH-hash-more-explicit-issue69 branch August 28, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR1 editorial The item is editorial in nature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants