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

Make access token mandatory for continuation API calls. #129

Merged
merged 3 commits into from Dec 16, 2020

Conversation

jricher
Copy link
Collaborator

@jricher jricher commented Nov 20, 2020

Closes #67

@jricher jricher added the Pending Merge PR will be merged unless there are changes to consensus label Nov 24, 2020
@jricher
Copy link
Collaborator Author

jricher commented Nov 24, 2020

This seems to follow the consensus of the working group as discussed at the meeting and on the list, changes will be merged unless there is a clear alternative proposal accepted by the working group.

new `continue` response MUST include a bound access token as well, and
this token SHOULD be a new access token.
The new `continue` response MUST include a bound access token as well, and
this token SHOULD be a new access token, invalidating the previous access token.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why SHOULD? Are there cases where there's a good reason to retain an old one? I am sure this detail is critical for any formal security proof.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I fully agree this is a MUST. Dick had another view however.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we need to call WG consensus on this, fine. We cannot have SHOULDs just because we are unable to reach agreement. Quoting RFC 2119, "This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course." Which implies that we need to say when it may be appropriate to keep the old one.

Copy link
Collaborator

@fimbault fimbault Nov 25, 2020

Choose a reason for hiding this comment

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

The best would be for Dick to reexplain his use case (and other people as well if it makes sense) and then we can decide as a group. Otherwise by default, I believe a MUST applies here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This PR doesn't change the behavior here, so we'll track whether this should be a SHOULD/MUST in a separate issue: #147

Copy link
Collaborator

@fimbault fimbault left a comment

Choose a reason for hiding this comment

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

I've looked at the rest (besides Yaron's comments), seems fine to me.

More text to be added into security considerations later when that's more filled out
@dickhardt
Copy link

How was @yaronf's comment about choosing just one method to keep it simple? Why do we need an access token and a URL?

@fimbault
Copy link
Collaborator

@dickhardt not sure i understand your question

@dickhardt
Copy link

The AS is choosing how to identify a request by a unique URL, an access token, or having only request at a time from a client. Lots of implementation choices and opportunities to make mistakes -- and overly complex.

@fimbault
Copy link
Collaborator

fimbault commented Nov 30, 2020

There seemed to be some agreement during the IETF109 call that we indeed had to chose between the 2 alternatives.

Now that leaves the question of which alternative to select. Between the 2, the editors have proposed (with explanations) to make access tokens mandatory.

Co-authored-by: Warren Parad <wparad@users.noreply.github.com>
@aaronpk aaronpk merged commit 490e798 into main Dec 16, 2020
@aaronpk aaronpk deleted the continue-access-token branch December 16, 2020 17:53
@jricher jricher removed the Pending Merge PR will be merged unless there are changes to consensus label Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Continuation API artifact structure
6 participants