Specify the HTTP Gateway protocol #20

nomeata · 2022-04-01T12:22:42Z

We have had the HTTP gateway for a while now, but

we didn’t have a public record of the expected Canister interface, with the authorative Candid fragment
we already have multiple implementations (icx-proxy and the service worker), and people we looking at the implementations to understand how thing work.
we see new features being implemented in one, but not the other implementation, and they start to diverge
some features are only implementation-defined (e.g. the certification fallback to index.html)
the interaction of new features (e.g. certificate, upgrade to update and streaming) is ill or at least vaguely defined.

So I think we urgently need a proper specification for the HTTP Gateway protocol, and a place to host this. The IC Interface Spec is a natural place, so here is a possible start.

This PR is descriptive, not prescriptive, with regard to the status quo: I (try to) specify the existing behavior of our Gateway implementations, not define a new and different protocol.

Preview available, pushed manually until #9 or similar has been fixed.

nomeata · 2022-04-01T12:23:31Z

Draft until external contributions are possible.

@paulyoung, can you also review this? It would be helpful if you could double check this. Who else should be pinged?

nomeata · 2022-04-01T12:30:30Z

@jplevyak, right now the spec says that all headers are passed through by the boundary node, but that’s not really true, is it? What header filtering/mangling/addition should we specify/document here?

spec/index.adoc

jzxchiang1 · 2022-04-02T05:24:52Z

spec/index.adoc

+|identity | rdmx6-jaaaa-aaaaa-aaadq-cai
+|nns | qoctq-giaaa-aaaaa-aaaea-cai
+|dscvr | h5aet-waaaa-aaaab-qaamq-cai
+|personhood | g3wsl-eqaaa-aaaan-aaaaa-cai


Where is personhood specified?

I don't see it here.

Not really convinced this canister resolution belongs in the HTTP Gateway protocol, although it is good to know in general I suppose.

I saw it for the first time yesterday in https://github.com/dfinity/ic/blob/a254116be472f81b85b76429ac8d987f80dd4411/typescript/service-worker/src/sw/http_request.ts#L17. I'll leave it to the Dfinity people to say if this should be there or not. But it demonstrates the need for a authorative list, at least until we have a dynamic system in place.

spec/index.adoc

jzxchiang1 · 2022-04-02T05:38:14Z

spec/http-gateway.did

+ status_code: nat16;
+ headers: vec HeaderField;
+ body: blob;
+ upgrade : opt bool;


Typo: upgrade -> update

Unfortunately(?) this field is called upgrade, as in “upgrade the call from query to an update call”. Very confusing… oh well.

See https://github.com/dfinity/icx-proxy/pull/6/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR259

Probably to late to change, but would promote be a better name?

Probably. Or resend_as_update_call. Or, in line with the pattern for the streaming callback, it could even pass a func ref.

In a way it's not too late: we can change this while keeping backward conpat in the implementations (there is only one so far) relatively easily (the proxy can simply keep both in it's internal expected type). I just wonder if we should do it with this PR, or maybe as a separate step, to get this in first.

spec/http-gateway.did

spec/index.adoc

jzxchiang1 · 2022-04-02T05:43:04Z

spec/http-gateway.did

+service : {
+ http_request: (request: HttpRequest) -> (HttpResponse) query;
+ http_request_update: (request: HttpRequest) -> (HttpResponse);
+}


I think it'd be clearer to make reference to the streaming callback here, but I understand why you didn't. (It's optional and also dynamically specified in HttpResponse.)

The name is arbitrary, right? So I wouldn't quite know how to include it.

jplevyak · 2022-04-02T06:04:18Z

@frederikrothenberger and @Daniel-Bloom-dfinity are current working this.

nomeata · 2022-04-02T09:49:05Z

spec/index.adoc

+
+* The HTTP response status code is taken from the `status_code` field.
+
+* The HTTP response headers are taken from the `headers` field.


Is the boundary node mangling or filtering the headers in any way that should be noted here?

The filtering we have is in dfinity/ic/ic-os/boundary-guestos/rootfs/etc/nginx/conf.d/001-ic-nginx.conf.

The short of it is we add these response headers (and thus their implications) to every HTTP call response:

access-control-allow-origin: * access-control-allow-methods: GET, POST, HEAD, OPTIONS access-control-allow-headers: DNT,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range,Cookie access-control-expose-headers: Content-Length,Content-Range x-cache-status: MISS

I guess the question is: Why set these? Shouldn’t the canister have control over them?

The short answer is "it's complicated". The longer answer is "because headers are not yet certified". As such we currently limit headers a bit to improve security. Once we get certified headers, we should not have any more need for this limitation.

Ok, good enough for this PR I guess.

origyndev · 2022-04-02T15:22:33Z

If we are making suggestions here I’d love for there to be a “return and upgrade” feature that returns the result of the query to the client but then calls update for recording the serving of the request in the canister. The reply to this call can be discarded as the request has already been served.

nomeata · 2022-04-02T18:41:56Z

If we are making suggestions here

I suggest we don't, for now. Let's focus in finalizing a spec for the status quo. Might be hard enough.

Once thats done additional changes can be nicely discussed in their own PRs against master. Or maybe on the forum first.

nomeata · 2022-04-05T09:24:55Z

I’ve manually pushed a rendering of this to netlify, to get a preview until #9 or similar has been fixed.

paulyoung · 2022-04-06T21:37:57Z

I'll try to review this soon but in the meantime I wanted to share https://forum.dfinity.org/t/boundary-node-http-response-headers/10747 in case it's of interest or provides some additional insight.

Who else should be pinged?

Based on the above I think @lastmjs would be interested in this.

spec/index.adoc

Co-authored-by: Paul Young <84700+paulyoung@users.noreply.github.com>

dfinity-droid-prod · 2022-04-08T16:01:13Z

Dear @nomeata,

In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA¹.

If you decide to agree with it, please visit this issue and read the instructions there.

— The DFINITY Foundation

Contributor License Agreement ↩

paulyoung · 2022-04-08T17:14:55Z

Pinging @SuddenlyHazel for visibility since this set of functionality came up in conversations today.

nomeata · 2022-04-09T13:46:23Z

Ok, with the CLA requirement out of the way, this could now be merged. Who on the DFINITY side is owning the process of shepherding and eventually merging community contributions? @Dfinity-Bjoern?

jwiegley · 2022-04-11T15:51:04Z

@nomeata It is Björn and myself.

Ali-Piccioni · 2022-04-14T14:20:28Z

update cla

frederikrothenberger · 2022-04-21T07:30:52Z

spec/index.adoc

+
+2. The value of the header must be a structured header according to RFC 8941 with fields `certificate` and `tree`, both being byte sequences.
+
+3. The `certificate` must be a valid certificate as per <<certification>>, signed by the root key. If the certificate contains a subnet delegation, the delegation must be valid for the given canister. The timestamp in `/time` must be recent. The subnet state tree in the certificate must reveal the canister’s <<state-tree-certified-data,certified data>>.


Currently neither icx-proxy nor the service worker check the certificate /time (which is bad). Can you give some more specific measure of "recent"? 5 minutes?

That’s probably for the security team to decide. 5 min should be ample (until we get into edge node caching, then things become tricky, because a cache is hardly different from an attacker serving old data).

frederikrothenberger

Thanks for writing this spec.

service-worker: implement support for upgrade flag Implements support for the `upgrade` flag as per HTTP gateway specification: dfinity/interface-spec#20 See merge request dfinity-lab/public/ic!4476

Dfinity-Bjoern

That's great! Thank you very much @nomeata for contributing this!

nomeata · 2022-05-03T08:18:58Z

Note that I am not allowed to merge, so someone else will have to press that button (and ideally presses the “Squash and merge” button and manually cleans up the PR description or copies the PR description into the commit message – mergify would do that for us but that’s not enabled at the moment.)

Specify the HTTP Gateway protocol

96db2ed

jzxchiang1 suggested changes Apr 2, 2022

View reviewed changes

jplevyak requested review from Daniel-Bloom-dfinity and frederikrothenberger April 2, 2022 06:03

nomeata added 3 commits April 2, 2022 11:45

Review comments

6a01a17

Align table

fbaa36c

Section titles

0c433a3

nomeata commented Apr 2, 2022

View reviewed changes

nomeata added 5 commits April 5, 2022 11:06

Improve description of the StreamingToken type, and add stern warning

f7e6120

Add note about headers

f7e9bc8

Move to the right section

84f308f

Document additional headers

153179d

Fix adoc

e89a332

More adoc fixes

1455268

paulyoung reviewed Apr 6, 2022

View reviewed changes

Apply suggestions from Paul

8ce8f7e

Co-authored-by: Paul Young <84700+paulyoung@users.noreply.github.com>

frederikrothenberger mentioned this pull request Apr 7, 2022

feat: Allow deserialization of candid values with unknown types dfinity/agent-js#555

Merged

4 tasks

Ali-Piccioni marked this pull request as ready for review April 8, 2022 16:00

Ali-Piccioni requested a review from a team as a code owner April 8, 2022 16:00

Ali-Piccioni closed this Apr 8, 2022

Ali-Piccioni reopened this Apr 8, 2022

dfinity-droid-prod bot added the cla:pending label Apr 8, 2022

dfinity-droid-prod bot added cla:agreed and removed cla:pending labels Apr 8, 2022

nomeata mentioned this pull request Apr 9, 2022

Making future types renameable dfinity/candid#337

Open

Merge branch 'master' into joachim/http-gateway

130f6e9

frederikrothenberger reviewed Apr 21, 2022

View reviewed changes

frederikrothenberger approved these changes Apr 21, 2022

View reviewed changes

Dfinity-Bjoern approved these changes May 3, 2022

View reviewed changes

Add changelog entry

861074a

Daniel-Bloom-dfinity approved these changes May 3, 2022

View reviewed changes

Merge branch 'master' into joachim/http-gateway

d2992b0

jwiegley merged commit a6c6f1c into dfinity:master May 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify the HTTP Gateway protocol #20

Specify the HTTP Gateway protocol #20

nomeata commented Apr 1, 2022 •

edited

Loading

nomeata commented Apr 1, 2022

nomeata commented Apr 1, 2022

jzxchiang1 Apr 2, 2022

nomeata Apr 2, 2022

jzxchiang1 Apr 2, 2022

nomeata Apr 2, 2022

crusso Apr 6, 2022

nomeata Apr 7, 2022

jzxchiang1 Apr 2, 2022

nomeata Apr 2, 2022

jplevyak commented Apr 2, 2022

nomeata Apr 2, 2022

Daniel-Bloom-dfinity Apr 4, 2022

nomeata Apr 5, 2022

Daniel-Bloom-dfinity Apr 6, 2022

nomeata Apr 7, 2022

origyndev commented Apr 2, 2022

nomeata commented Apr 2, 2022

nomeata commented Apr 5, 2022

paulyoung commented Apr 6, 2022 •

edited

Loading

dfinity-droid-prod bot commented Apr 8, 2022

paulyoung commented Apr 8, 2022

nomeata commented Apr 9, 2022

jwiegley commented Apr 11, 2022

Ali-Piccioni commented Apr 14, 2022

frederikrothenberger Apr 21, 2022

nomeata Apr 21, 2022

frederikrothenberger left a comment

Dfinity-Bjoern left a comment

nomeata commented May 3, 2022


		* The HTTP response status code is taken from the `status_code` field.

		* The HTTP response headers are taken from the `headers` field.


		2. The value of the header must be a structured header according to RFC 8941 with fields `certificate` and `tree`, both being byte sequences.

		3. The `certificate` must be a valid certificate as per <<certification>>, signed by the root key. If the certificate contains a subnet delegation, the delegation must be valid for the given canister. The timestamp in `/time` must be recent. The subnet state tree in the certificate must reveal the canister’s <<state-tree-certified-data,certified data>>.

Specify the HTTP Gateway protocol #20

Specify the HTTP Gateway protocol #20

Conversation

nomeata commented Apr 1, 2022 • edited Loading

nomeata commented Apr 1, 2022

nomeata commented Apr 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplevyak commented Apr 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

origyndev commented Apr 2, 2022

nomeata commented Apr 2, 2022

nomeata commented Apr 5, 2022

paulyoung commented Apr 6, 2022 • edited Loading

dfinity-droid-prod bot commented Apr 8, 2022

Footnotes

paulyoung commented Apr 8, 2022

nomeata commented Apr 9, 2022

jwiegley commented Apr 11, 2022

Ali-Piccioni commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikrothenberger left a comment

Choose a reason for hiding this comment

Dfinity-Bjoern left a comment

Choose a reason for hiding this comment

nomeata commented May 3, 2022

nomeata commented Apr 1, 2022 •

edited

Loading

paulyoung commented Apr 6, 2022 •

edited

Loading