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

Clarify digest-algorithm syntax with parameters (or drop parameters). #850

Closed
ioggstream opened this issue Jul 9, 2019 · 18 comments
Closed

Comments

@ioggstream
Copy link
Contributor

ioggstream commented Jul 9, 2019

I expect

This spec either:

  • to provide an RFC3230 compatible example of digest-algorithm with parameters;
  • explicitly drop parameters because they are not widely used and augment the attack surface;

RFC3230 stated that:

   For some algorithms, one or more parameters may be
   supplied.

      digest-algorithm = token

   The BNF for "parameter" is as is used in RFC 2616 [4].  All digest-
   algorithm values are case-insensitive.

but:

  • provided no example of parameterized algorithms

In this draft we preserved that sentence but:

  • still did not provide an example of parameterized algorithm;
  • still did not update the reference to BNF to the latest http spec.

In #1213 there's a possible fix to support parameters, like the following

Digest: myhash=dcRDgR2GM35DluAV13PzgnG6+pvQwPywfFvAu1UeFrs=; param1=40

Notes

Use cases are parametrizable digest-algorithms like mi-sha256.

Another solution could be to deprecate field parameters in digest: parameters could still be adopted serializing them in the digest-value, and leaving the specification/serialization to the definition of the digest-algorithm. eg:

Digest: myhash=40/dcRDgR2GM35DluAV13PzgnG6+pvQwPywfFvAu1UeFrs
@ioggstream
Copy link
Contributor Author

ioggstream commented Jul 5, 2020

@LPardue @reschke I don't know of any parameter in the wild, but removing that will deviate from RFC 3230.

If we want to remove parameters, we should explicit:

  • that we will deprecate that feature (eg. because parameters can make things less secure);
  • that parameters are forbidden;
  • that if you want to use a parameterized algorithm you should encode their values in the digest-value and take care of the security considerations inside the definition of your digest-algorithm;

bikeshed=bs:8|offset:2|8E9q0okq

@reschke
Copy link
Contributor

reschke commented Jul 5, 2020

FWIW, deviating would fine in this case. One point in revising specs is to remove unused stuff.

@ioggstream
Copy link
Contributor Author

@reschke I agree. My question is whether it's better to:

  • leave digest-algorithm implementors to define a way to pass parameters via the serialization
  • standardize the way parameters are passed.

Guidance on that is welcome.

@LPardue
Copy link
Contributor

LPardue commented Aug 13, 2020

@mnot removal of Digest parameters would be a case where our update work deviates a bit further from what we might have initially had in mind. As Julian points out its fine to do so, however I want to make this more visibile before we take a decision.

@ioggstream ioggstream changed the title Clarify digest-algorithm syntax with parameters. Clarify digest-algorithm syntax with parameters (or drop parameters). Aug 17, 2020
@ioggstream
Copy link
Contributor Author

I want to make this more visibile before we take a decision

@LPardue I tweeted on that https://twitter.com/ioggstream/status/1295292991335268352 :)

To me leaving the parameter definition to the algorithm specification is fine, eg #850 (comment)

@LPardue
Copy link
Contributor

LPardue commented Aug 18, 2020

I'll send an email to the HTTP WG list too.

@reschke
Copy link
Contributor

reschke commented Aug 18, 2020

+1 for removing parameters.

@bsdphk
Copy link
Contributor

bsdphk commented Aug 18, 2020

+0 on removing parameters, but I want to strongly suggest that any examples of encoded parameters are expressed in terms of Structured Fields, even if the entire header is not compliant.

ie:

bikeshed=:SD8E9q0okq:

where the parameters are encoded in the first two base64 encoded bytes.

@LPardue
Copy link
Contributor

LPardue commented Aug 26, 2020

ioggstream added a commit that referenced this issue Sep 7, 2020
ioggstream added a commit that referenced this issue Oct 7, 2020
@ioggstream
Copy link
Contributor Author

@reschke @mnot brief recap after discussion.

We propose to deprecate parameters because:

  • we were not able to find any implementation;
  • if they are used as an input parameter for validating the checksum, an attacker can use them to steer the validation process;

The proposal:

  • deprecate parameters (a receiver MUST ignore a checksum containing parameters)
  • suggests that digest-algorithms wishing parameterization, can encode parameters inside the checksum

OT: I agree that making digest a SF would have resolved the issue with just some advice of not using parameters as input values when defining new digest-algorithms

@ioggstream
Copy link
Contributor Author

@LPardue @reschke As parameters are now obsoleted, we have to decide whether to provide instructions for the transition.
The actual draft just deprecated parameters and does not allow their presence. If you think the current behavior is fine, imho we could merge #1259 and cutoff -04

Current behavior

The presence of a parameter in any representation-data-digest will invalidate the syntax of the whole Digest value.

Hyp 1

add the parameter definition to the ABNF and explicit that they SHOULD|MUST be ignored, while the checksum should be processed

Hyp 2

add the parameter definition to the ABNF and explicit that if a representation-data-digest contains parameter only this checksum should be ignored

@reschke
Copy link
Contributor

reschke commented Oct 17, 2020

If we have no evidence of parameters in use, why would we need to consider a transition? Don't make things more complicated than they need to be.

richanna pushed a commit to richanna/http-extensions that referenced this issue Oct 20, 2020
@ioggstream
Copy link
Contributor Author

@LPardue @martinthomson

We deprecated parameters after this ml thread
but the discussion arose again after the migration to SF.

SF allows dropping the mention of them without formally deprecating, but I see some space for non interoperable behaviors.
Parameters could apply both to specific algorithms and to all algorithms, eg.

Content-Digest: sha-256=:fafafa==:; q=1

Moreover we haven't defined any parameter yet.

Some questions we might be able to answer:

  • are parameters just informative or should affect validation?
  • what should people do when unexpected parameters appear in a digests?
  • should we tweak the IANA Hash Algorithms registry? Do we need to create a new parameter registry?

@ioggstream ioggstream reopened this Mar 1, 2022
@LPardue
Copy link
Contributor

LPardue commented Mar 1, 2022

My thoughts are:

  • no parameters are defined in this document, implementations ignore unknown parameters as already instructed to by structured fields
  • we shouldn't litigate on what extension might or might not do in the future. Especially when we have no concrete use cases.
  • there is no point creating an empty parameters registry for something nobody uses. If in future an extension wants to standardise the use of a parameter, a registry might make sense then. That future work would need to establish the registry.
    • Similarly, I see no point in augmenting the algorithm table right now to accommodate parameters.

SF allows dropping the mention of them without formally deprecating, but I see some space for non interoperable behaviors.
Parameters could apply both to specific algorithms and to all algorithms, eg.

Content-Digest: sha-256=:fafafa==:; q=1

By default, receivers would ignore q=1 in your example. If the sender is expecting something to different to happen in the protocol due presence of q=1, for interoperability they have to do the work to write spec that tells the receiver what to do.

In terms of paramaterizing algorithms, based on the evidence so far, "you ain't gonna need it". And if that turns out to be wrong, then we can spend to time to debate the merits of structured fields parameters vs in-band encoding inside the Bytes. But we don't need to speculate now IMO.

@ioggstream
Copy link
Contributor Author

implementations ignore unknown parameters as already instructed to by structured fields

Ok to defer the issue to future specs using parameters: I suppose the specs don't need further changes then.

@LPardue
Copy link
Contributor

LPardue commented Mar 1, 2022

I'll make a PR that will capture how parameters are treated today and call out extensibility.

@ioggstream
Copy link
Contributor Author

@LPardue are you ok to close this issue without further changes to the document, according to your comments in #850 (comment) ?

@LPardue
Copy link
Contributor

LPardue commented May 9, 2022

I'm struggling to come up with any text to add that won't overcomplicate things. Let's close for now and see if this comes up again during last call.

@LPardue LPardue closed this as completed May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants