Skip to content

Remove redundant definitions. See #1311#1312

Merged
richanna merged 4 commits intomasterfrom
ioggstream-1311
Dec 2, 2020
Merged

Remove redundant definitions. See #1311#1312
richanna merged 4 commits intomasterfrom
ioggstream-1311

Conversation

@ioggstream
Copy link
Copy Markdown
Contributor

@ioggstream ioggstream commented Oct 24, 2020

This PR

  • uses sf-integer and sf-decimal to avoid re-defining Integer String and Decimal String

Note

This PR does not aim at deciding the precision of the associated fields, but only to delegate their definition to Structured Fields.
*created and *expires precision can be discussed in another PR :)

expires
:
: OPTIONAL. The `expires` parameter is a Decimal containing the signature's Expiration Time, expressed as the canonicalized value of the `*expires` content identifier, as defined in Section 2. If the signature does not have an Expiration Time, this parameter MUST be omitted. If not specified, the signature's Expiration Time is undefined.
: OPTIONAL. The `expires` parameter is a `sf-decimal` containing the signature's Expiration Time,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
: OPTIONAL. The `expires` parameter is a `sf-decimal` containing the signature's Expiration Time,
: OPTIONAL. The `expires` parameter is a `sf-integer` containing the signature's Expiration Time,

I believe this was a decimal initially because I wanted to support millisecond and nanosecond precision... the use case was high frequency trading. In time, it became evident that if people want that sort of resolution, they can create their own X-Header (or the like... High-Precision-Expiration: 234.3342).

Copy link
Copy Markdown
Contributor Author

@ioggstream ioggstream Oct 27, 2020

Choose a reason for hiding this comment

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

@msporny thanks! we are discussing create/expires here #1280

In this PR I just wanted to get rid of duplicate definitions leaving the semantic discussion to other issues. Specifically, there are other parts defining expires as a decimal. Probably we should - in other PRs - address the definition of those parameters in a one-and-only place.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

#1305

I believe this was a decimal initially because I wanted to support millisecond and nanosecond precision... the use case was high frequency trading. In time, it became evident that if people want that sort of resolution, they can create their own X-Header (or the like... High-Precision-Expiration: 234.3342).

Thanks for sharing the historic reasons behind this!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A non-standard High-Precision-Expiration header field doesn't enable sub-second expiration precision. The signature's expiration time is defined by the *expires parameter. Profiling specs can't change that, they can only add additional expiration semantics on top of that. We should not encourage that though, as it introduces confusion – which expiration rules apply? Do they both have to pass, or just one? etc.

If we want to eliminate decimals, let's express both *created and *expires in milliseconds.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm amazed. Is the implication here that signers and verifiers have clocks synchronized with sub-second precision?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@martinthomson

1- I agree with your consideration
2- In this PR I just wanted to get rid of duplicate definitions leaving the semantic discussion to other issues. Specifically, there are other parts defining expires as a decimal.
3- We should - in other PRs - discuss the precision of *created and *expires which imho should be expressed with seconds precision, but I don't think should be discussed in this PR :D

this PR is mostly an editorial one which just removes redundant definitions.

Once we agree we can delegate definitions

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@martinthomson wrote:

I'm amazed. Is the implication here that signers and verifiers have clocks synchronized with sub-second precision?

Yes, this is true in high-frequency trading systems (for example)... some of which have highly tuned HTTP stacks and are looking at QUIC + HTTP/3 with interest. The legal implications here are that, in some of those systems, that you're supposed to execute orders as you receive them and sometimes the digital signature time matters (from a contractual basis). Here's what a couple of hundred milliseconds in a high-frequency trading system looked like back in 2013 (when we started on this spec):

https://youtu.be/B_k_elbBz8c?t=103

Things move much more quickly now, which is why we might want to drop this use case and get them to create and sign over their own header that has nanosecond (or better) precision.

All that to say, we could probably back off on the millisecond precision and go to second-based precision if we wanted to support constrained devices that may not be able to do 64-bit integers?

Also, agree with @ioggstream -- all of this is for another PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@richanna wrote:

A non-standard High-Precision-Expiration header field doesn't enable sub-second expiration precision. The signature's expiration time is defined by the *expires parameter. Profiling specs can't change that, they can only add additional expiration semantics on top of that. We should not encourage that though, as it introduces confusion – which expiration rules apply? Do they both have to pass, or just one?

Hmm, I wish things worked like that. :)

For example, the high frequency traders won't create a profiling spec, they'll do whatever they want to with this spec as it's base. Notably, they won't sign over expires (since it's optional) and will instead add their own header with nanosecond precision, sign over that, and use the standard signature verification thing PLUS their own verification algorithm that takes their header into account.

That's not necessarily a bad thing, per se... and I admit that this is a corner case... but we should be careful to not prevent this sort of usage of the specification.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the implication here that signers and verifiers have clocks synchronized with sub-second precision?

Yes. In addition to @msporny's high-frequency trading example, this level of clock synchronization is not uncommon within distributed systems.

For example, the high frequency traders won't create a profiling spec, they'll do whatever they want to with this spec as it's base. Notably, they won't sign over expires (since it's optional) and will instead add their own header with nanosecond precision, sign over that, and use the standard signature verification thing PLUS their own verification algorithm that takes their header into account.

Yes, and that results in a non-standard, non-interoperable implementation. That's what I want to avoid by including support for sub-second expiration times in the spec.

Comment thread draft-ietf-httpbis-message-signatures.md Outdated
Comment thread draft-ietf-httpbis-message-signatures.md Outdated
@richanna richanna merged commit d6009b5 into master Dec 2, 2020
@richanna richanna deleted the ioggstream-1311 branch December 2, 2020 00:28
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.

5 participants