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

Strengthen 2119 language around tokens. #2124

Closed
wants to merge 1 commit into from
Closed

Conversation

ekr
Copy link
Collaborator

@ekr ekr commented Dec 12, 2018

Rationale:

  1. We should recommend that people use tokens.
  2. Saying "SHOULD" attempt to validate seems weak. Why isn't this a
    MUST? This is the weakest of these changes, I think.

Rationale:

1. We should recommend that people use tokens.
2. Saying "SHOULD" attempt to validate seems weak. Why isn't this a
MUST? This is the weakest of these changes, I think.
@@ -1627,7 +1627,7 @@ interface. A client needs to start the connection process over if it migrates
prior to completing the handshake.

When a server receives an Initial packet with an address validation token, it
SHOULD attempt to validate it, unless it has already completed address
MUST attempt to validate it, unless it has already completed address
Copy link
Contributor

Choose a reason for hiding this comment

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

MUST implies that it must fail if it cannot validate. During server cluster upgrades that might not be feasible. It could lead to consistent connection failures as long at the client has a token to try. It is in the servers interest to generally attempt validation so SHOULD seems reasonable to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No that's not correct. The next sentence describes how to handle failures.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go back to 2119 definitions: Are there situations in which a server might choose not to validate? Will a failure to validate cause an interoperability problem?

Copy link
Member

Choose a reason for hiding this comment

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

We also use MUST for security problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's an "unless" clause that captures the cases where a server won't validate, AFAICT. I'm ok with MUST here.

Also, the "it" is ambiguous. I would change the sentence to ".... MUST attempt to validate the token"

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

The second change I like. The first less so.

@@ -1627,7 +1627,7 @@ interface. A client needs to start the connection process over if it migrates
prior to completing the handshake.

When a server receives an Initial packet with an address validation token, it
SHOULD attempt to validate it, unless it has already completed address
MUST attempt to validate it, unless it has already completed address
Copy link
Member

Choose a reason for hiding this comment

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

We also use MUST for security problems.

@@ -1610,7 +1610,7 @@ A resumption token SHOULD be constructed to be easily distinguishable from
tokens that are sent in Retry packets as they are carried in the same field.

If the client has a token received in a NEW_TOKEN frame on a previous connection
to what it believes to be the same server, it can include that value in the
to what it believes to be the same server, it SHOULD include that value in the
Copy link
Member

Choose a reason for hiding this comment

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

I think that we need text on why it might not. I suspect that this is a MUST unless (sending from a new address) OR (you don't feel like it).

I think that the "can" is fine here.

Copy link
Contributor

Choose a reason for hiding this comment

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

... or it dropped its store of tokens. SHOULD is useful in encouraging clients to do the performant thing -- use tokens unless they really can't.

Copy link
Member

Choose a reason for hiding this comment

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

If we go with SHOULD, then we need to address the reasons that an endpoint might diverge from that. I realize that it's a cop-out, but "can" is so much easier.

@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -transport labels Dec 13, 2018
@martinthomson
Copy link
Member

Rebased and merged manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants