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

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

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

luqmana
Copy link
Contributor

@luqmana luqmana commented Aug 12, 2022

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.

… for http2 instead of the http1 host header.
@luqmana luqmana requested a review from plotnick August 12, 2022 00:37
Copy link
Contributor

@plotnick plotnick left a comment

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
Copy link
Contributor Author

luqmana commented Aug 12, 2022

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
Copy link
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
Copy link
Contributor Author

luqmana commented Aug 12, 2022

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.

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.

2 participants