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

Address some items from httpdir early review #540

Merged
merged 8 commits into from
Jan 17, 2024
Merged

Conversation

tgeoghegan
Copy link
Collaborator

@tgeoghegan tgeoghegan commented Jan 2, 2024

Mark Nottingham (@mnot) was kind enough to share early review feedback on DAP on behalf of httpdir (1). This PR addresses several of the items there. See individual commit messages for discussion.

Per httpdir early review of DAP ([1]), use "resource" or "server" as
appropriate instead of "endpoint".

[1]: https://datatracker.ietf.org/doc/review-ietf-ppm-dap-09-httpdir-early-nottingham-2023-12-29/
@tgeoghegan tgeoghegan marked this pull request as draft January 2, 2024 18:52
@tgeoghegan

This comment was marked as outdated.

Per httpdir early review of DAP ([1]), make a clear distinction between
a client in the DAP protocol ("Client") and a client in the HTTP
protocol ("HTTP client"). Audit usage of "client" and "server"
throughout the document to disambiguate.

[1]: https://datatracker.ietf.org/doc/review-ietf-ppm-dap-09-httpdir-early-nottingham-2023-12-29/
Per httpdir early review of DAP ([1]), distinguish between the HTTP and
HTTPS requirements, and add an explicit reference to RFC 8446 for TLS.

[1]: https://datatracker.ietf.org/doc/review-ietf-ppm-dap-09-httpdir-early-nottingham-2023-12-29/
Per httpdir early review of DAP ([1]), Clients should be free to retry
HTTP GETs on HPKE configs.

[1]: https://datatracker.ietf.org/doc/review-ietf-ppm-dap-09-httpdir-early-nottingham-2023-12-29/
Per httpdir early review of DAP ([1]), be less prescriptive about
Cache-Control in HPKE config responses.

[1]: https://datatracker.ietf.org/doc/review-ietf-ppm-dap-09-httpdir-early-nottingham-2023-12-29/
@tgeoghegan tgeoghegan changed the title Audit usage of "endpoint" Address some items from httpdir early review Jan 2, 2024
@tgeoghegan tgeoghegan marked this pull request as ready for review January 2, 2024 21:08
endpoints can be found.
* `helper_aggregator_endpoint`: A URL relative to which the Helper's API
endpoints can be found.
* `leader_aggregator_server`: A URL relative to which the Leader's API resources
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure server is appropriate here (or for helper_aggregator_server below) -- I think either {leader,helper}_aggregator_endpoint or perhaps {leader,helper}_aggregator_url would be clearer. I'd expect a server to indicate a hostname, while this is a full URL including a protocol & path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(I'd go with "endpoint", but I think that an endpoint might also indicate a particular HTTP verb; if that's correct, I'd go with "URL.")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

_url seems better since that's what the text on the right of the colon says. I want to avoid "endpoint" as the httpdir review flagged that word.

@@ -316,14 +316,18 @@ Batch interval:
of the reports in the batch.

Client:
: A party that uploads a report.
: DAP protocol role identifying a party that uploads a report. Note the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
: DAP protocol role identifying a party that uploads a report. Note the
: A DAP protocol role identifying a party that uploads a report. Note the

(editorial, matching structure of other items in this list)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. "The" feels a bit more natural than "A" to me.

possible by distributing the computation among the servers in such a way that,
as long as at least one of them executes the protocol honestly, no input is ever
seen in the clear by any server.
small set of aggregator servers. The aggregators' goal is to compute some
Copy link
Collaborator

Choose a reason for hiding this comment

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

(This comment follows from off-PR discussion with/suggestion by @jbr.)

"small set of aggregator servers" is correct but perhaps imprecise -- nowadays, there are exactly two aggregator servers, the Leader & the Helper. Maybe "two aggregator servers, called the Leader & the Helper"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense! I left out the discussion of leader and helper because those terms aren't used again in the introduction and they get introduced properly in the glossary.

HTTPS provides server authentication and confidentiality. Use of HTTPS is
REQUIRED.
Communications between DAP participants are carried over HTTP {{!RFC9110}}. Use
of HTTPS {{!RFC8446}} is REQUIRED to provide server authentication and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the correct reference? RFC8446 defines TLS, not HTTPS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, good point. RFC 9110 obsoletes RFC 2818 "HTTP Over TLS", so maybe the reference to 9110 suffices?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense to me.

@tgeoghegan tgeoghegan merged commit 7623751 into main Jan 17, 2024
2 checks passed
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.

3 participants