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

Fix: #1941. Editorial alignments #1973

Merged
merged 18 commits into from Mar 1, 2022
Merged

Fix: #1941. Editorial alignments #1973

merged 18 commits into from Mar 1, 2022

Conversation

ioggstream
Copy link
Contributor

@ioggstream ioggstream commented Feb 16, 2022

This PR

  • same boilerplate text for C-Digest and R-Digest
  • avoid ABNF in favor of Types
  • representation-data -> representation data
  • representatio-data-digest -> digests (clarified in Notational Convetions);
  • integrity fields -> Integrity fields (@LPardue let me know if you meant it lowercase, it's ok to me in any way);
  • digest algorithm -> hashing algorithm,

Note: it'd be cool if we could write something like

~~~ abnf
R-Digest = sf-dictionary
member-value = sf-binary

but it's ok anyway

@LPardue
Copy link
Contributor

LPardue commented Feb 16, 2022

Thanks for these.

I'm still unsure of the best way to reference structured fields things. I've seen all kinds of permutations across drafts, so I sent an email to the list to see if anyone has an opinion we might be able to align to. I agree that being able to express the type constraints in ABNF would be nice but I don't think that's standard and might be a step too far.

Comment on lines 527 to 532
Any mangling of digest fields, including de-duplication of `dict-member`s
or combining different field values (see {{Section 5.2 of SEMANTICS}})
might affect signature validation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Dictionaries already describe how to treat duplicate entries. So we probably need a separate issue to fix up this paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once a field is signed, even a compliant mangling invalidates the signature. I see this line as a simple advice to the users.

draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
STRUCTURED-FIELDS}}):
`Representation-Digest` is a Structured Fields `Dictionary` (see {{Section 3.2 of
STRUCTURED-FIELDS}}) where:
* members cannot have parameters;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is something I sidestepped during the initial change to SF. The SF grammar allows parameters, so member-values can have them. The SF says

   Both Items and Inner Lists allow parameters as an extensibility
   mechanism; this means that values can later be extended to
   accommodate more information, if need be.  To preserve forward
   compatibility, field specifications are discouraged from defining the
   presence of an unrecognized parameter as an error condition.

When updating Digest we decided parameters could be a security problem. If we want prohibit the use of SF parameters, then we need some form of normative text to state that and probably justify why. I'd suggest doing that in a separate paragraph.

Copy link
Contributor

Choose a reason for hiding this comment

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

Furthermore, if we move the parameter discussion elsewhere, then we can get rid of the list format and use a prose style similar to what's already there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LPardue I added a paragraph in the security consideration.

ioggstream and others added 12 commits February 28, 2022 10:22
Co-authored-by: Lucas Pardue <lucaspardue.24.7@gmail.com>
Co-authored-by: Lucas Pardue <lucaspardue.24.7@gmail.com>
Co-authored-by: Lucas Pardue <lucaspardue.24.7@gmail.com>
Co-authored-by: Lucas Pardue <lucaspardue.24.7@gmail.com>
Co-authored-by: Lucas Pardue <lucaspardue.24.7@gmail.com>
Co-authored-by: Lucas Pardue <lucaspardue.24.7@gmail.com>
Co-authored-by: Lucas Pardue <lucaspardue.24.7@gmail.com>
@@ -346,7 +347,8 @@ communicate digests that are calculated using a hashing algorithm applied to
the actual message content (see {{Section 6.4 of SEMANTICS}}). It is a
Structured Fields Dictionary (see {{Section 3.2 of STRUCTURED-FIELDS}})
where:
* members cannot have parameters;

* members MUST NOT have parameters (see {{sec-agility}});
Copy link
Contributor

Choose a reason for hiding this comment

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

This point is not important enough to be the first one, it's a distraction to most readers.

I would strongly prefer we use prose style to discuss keys and values and punt the parameter discussion to a following paragraph

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LPardue as a non-native speaker, I just feel prose a bit harder to read, but sure! Feel free to edit this PR :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Furthermore, prohibiting parameters suffers can lead to the same fate of validation DoS.

So the text that discusses normative behaviour of parameters has to be more nuanced than a single line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood but it's likely that such an editorial comment would come up during our later review anyway. I'll take a go and implementing my suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take a go and implementing my suggestion.

🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parameters will be managed in another PR

@ioggstream
Copy link
Contributor Author

@LPardue I deferred parameters to another PR. After you refactor the list as prose, feel free to merge :)

Copy link
Contributor

@LPardue LPardue left a comment

Choose a reason for hiding this comment

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

Thanks @ioggstream, theres a lot of good fixes in here and the effort is appreciated.

@LPardue LPardue merged commit 6ac0282 into main Mar 1, 2022
@LPardue LPardue deleted the ioggstream-1941 branch March 1, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants