Look up header names in lowercase

[andrei-21]

UPDATE:
Look up header names in lowercase

Since the http library downcase header names:
https://docs.rs/http/latest/http/header/index.html#headername

Since reqwest downcases header names:

Header names are normalized to lower case. In other words, when
creating a HeaderName with a string, even if upper case characters are
included, when getting a string representation of the HeaderName, it
will be all lower case. This allows for faster HeaderMap comparison
operations.

https://docs.rs/reqwest/latest/reqwest/header/index.html#headername

[ldk-reviews-bot]

👋 Thanks for assigning @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[tankyleo]

TIL thank you ! Let's also do the same in the JWT implementation

[tankyleo]

Hmm rather than iterating over all the headers here, I think we can safely make the assumption that all the keys will be lowercased, and do get("authorization") directly. WDYT ?

[tankyleo]

Since the Authorizer trait is internal to vss-server, I think we should do this, and add a quick doc in the Authorizer trait definition along the lines of "implementations can assume all header names to be lower case"

[andrei-21]

Makes sense, done.

[tankyleo]

nit: Let's use this instead thank you:

	/// Verifies authentication and authorization based on request headers.
	/// Returns [`AuthResponse`] for an authenticated and authorized user or [`VssError::AuthError`]
	/// for an unauthorized request.
+   ///
+   /// Implementations can assume that all header names are downcased.
	```

[andrei-21]

Done.

[tankyleo]

Here let's do k.to_string(); HeaderName::to_string will always return a lowercased string. I am confident of this, but please double check on your side thank you.

Let's also add a short comment to note that HeaderName always yields lowercase strings.

[andrei-21]

Right. It works on my end like this.

[tankyleo]

LGTM, can you update the commit message to reflect the latest code ? Thank you, we don't actually do any downcasing anymore ourselves, since http::header::HeaderName already yields all header names in lowercase.

[tnull]

Thanks for pointing this out, valid concern. However, I don't think we want to lean on any particular client-side behavior here, especially not from reqwest which we're about to drop.

The issue is real though, as RFC 2616 states: "Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive."

So rather than trusting that the client sends downcased header fields only, we need to downcase whatever we get before comparing/looking it up. In particular, AUTHORIZATION is just as valid as authorization.

[tnull]

I don't think it makes sense to make this part of the public API, we should just do case-insensitive comparisons.

[andrei-21]

Thanks for pointing this out, valid concern. However, I don't think we want to lean on any particular client-side behavior here, especially not from reqwest which we're about to drop.

The issue is real though, as RFC 2616 states: "Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive."

So rather than trusting that the client sends downcased header fields only, we need to downcase whatever we get before comparing/looking it up. In particular, AUTHORIZATION is just as valid as authorization.

That is not about the client side (I misinterpreted it at the beginning).
The problem was that HeaderName converted to a string gives a downcased string, but the code was looking for Authorization header.

[tnull]

That is not about the client side (I misinterpreted it at the beginning). The problem was that HeaderName converted to a string gives a downcased string, but the code was looking for Authorization header.

Ah, so it's not (only) reqwest, but also our hyper dependency that auto-downcases. In that case

a) I think it would be good to add comment (and potentially even a debug-assertion) at the point where we convert HeaderMap to HashMap to ensure this will always hold and sub-sequent comparisons with the lowercased strings is fine (even if we'd ever switch away from hyper specifically)
b) still think this shouldn't be part of the public interface documentation

[andrei-21]

b) still think this shouldn't be part of the public interface documentation

I see several options:

1. Do not downcase, do not assume the case, but just do case-insensitive comparison (it was my initial idea, but tankyleo pointed that we can safely assume this since HeaderName is in lowercase)
2. Make a special type struct LowercaseString(String) and use it as hashmap keys (thus the assumption is enforced by the compiler)
3. Keep it as it is now, knowing that a header name is not a random string

[tnull]

b) still think this shouldn't be part of the public interface documentation

I see several options:

1. Do not downcase, do not assume the case, but just do case-insensitive comparison (it was my initial idea, but tankyleo pointed that we can safely assume this since HeaderName is in lowercase)
2. Make a special type struct LowercaseString(String) and use it as hashmap keys (thus the assumption is enforced by the compiler)
3. Keep it as it is now, knowing that a header name is not a random string

Well, I'd still be in favor of what I described above: following Postel's robustness principle, ensure and document we do downcase in our code, but not make it part of the public API whatsoever.

[andrei-21]

Hm, not sure I understand.
handle_request() makes sure that header names are downcased? Signing implementation performs a case-insensitive lookup?

[tnull]

Hm, not sure I understand. handle_request() makes sure that header names are downcased?

Where does it do that? You mean because http's HeaderName implementation currently does that? As we realized in this PR, it's not obvious, so we should a) add a comment there why it's safe to subsequently make downcased comparisons and probably b) even add a debug_assert ensuring we see the expected behavior.

I could imagine that at some point we might want to switch away from hyper/http and its particular behavior. So it would be good to have some checks in place that ensure we don't re-encounter the same issue in the future.

[andrei-21]

Is it what you meant: https://github.com/lightningdevkit/vss-server/compare/a9c08bbfc63f66ddd4eea7873cb302be0793ed29..6ff3f8f31e289dc2999d848367e9e201672102f3?

[tnull]

Is it what you meant: https://github.com/lightningdevkit/vss-server/compare/a9c08bbfc63f66ddd4eea7873cb302be0793ed29..6ff3f8f31e289dc2999d848367e9e201672102f3?

Yup, pretty much!