Skip to content

nexus: Update device_auth endpoint to use the authority psuedo-header for http2 instead of the http1 host header.#1582

Merged
luqmana merged 1 commit into
mainfrom
luqmana/http2-host
Aug 12, 2022
Merged

nexus: Update device_auth endpoint to use the authority psuedo-header for http2 instead of the http1 host header.#1582
luqmana merged 1 commit into
mainfrom
luqmana/http2-host

Conversation

@luqmana

@luqmana luqmana commented Aug 12, 2022

Copy link
Copy Markdown
Contributor

The cli (or anything using hyper underneath) may use HTTP2 when using https which made for some confusing failures. Anyways, the Host header is no more in HTTP2 and we should instead be using the :authority psuedo-header. Grabbing it off the request URI seems to be the way to do it in hyper.

@luqmana
nexus: Update device_auth endpoint to use the authority psuedo-header…
29b5029 
… for http2 instead of the http1 host header.
@luqmana luqmana requested a review from plotnick August 12, 2022 00:37
plotnick
plotnick approved these changes Aug 12, 2022
View reviewed changes

@plotnick plotnick left a comment

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.

Thanks for catching this! I admit I've not really kept up with the details of the latest HTTP standards; HTTP/1.1 still feels like the "new" one to me 😁

So, this seems fine as-is, but my follow-up question is whether it would be better to use the URI authority and scheme in all cases. There's a TODO in DeviceAuthRequest::into_response (the consumer of the extracted host) to make it handle HTTPS as well, and so it might be better to just pass the whole request.uri() into that function, and let it construct the new absolute URI from the old one. Thinking about it, I'm not sure there's a good reason to prefer the Host header in any case, and it would be somewhat simpler and more consistent.

I can push a commit that does that if you think it's a good idea, or we can open a new PR for it.

@luqmana

luqmana commented Aug 12, 2022

Copy link
Copy Markdown
Contributor Author

Unfortunately, we can't just always just use the authority in the URL because it is different across HTTP 2 and 1.1

HTTP/2.0:

Request {
    method: GET,
    uri: https://local.oxide-preview.com/test,
    version: HTTP/2.0,
    headers: {
        "user-agent": "curl/7.68.0",
        "accept": "*/*",
    },
    body: Body(
        Streaming,
    ),
}

HTTP/1.1

Request {
    method: GET,
    uri: /test,
    version: HTTP/1.1,
    headers: {
        "host": "local.oxide-preview.com",
        "user-agent": "curl/7.68.0",
        "accept": "*/*",
    },
    body: Body(
        Empty,
    ),
}

@plotnick

plotnick commented Aug 12, 2022

Copy link
Copy Markdown
Contributor

Ah, that both sucks and explains why I didn't do it that way in the first place (I couldn't remember exactly). Thanks for checking!

@luqmana

luqmana commented Aug 12, 2022

Copy link
Copy Markdown
Contributor Author

I'll merge this for now since the cli can't login over https rn but I think I'm going to try updating dropshot to make it possible to distinguish http vs https in request handlers.

@luqmana luqmana merged commit 418f2df into main Aug 12, 2022
@luqmana luqmana deleted the luqmana/http2-host branch August 12, 2022 17:34
This was referenced Aug 12, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@plotnick plotnick plotnick approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@luqmana @plotnick