server-util: Use http-auth crate to parse XMatrix

[zecakeh]

Fixes #1801.

[Kladki]

For finding the XMatrix challenge, would it be better to replace

ruma/crates/ruma-server-util/src/authorization.rs

Lines 50 to 63 in c50064a

	let mut xmatrix = None;
	for challenge in parser {
	let challenge = challenge?;
	if challenge.scheme == XMatrix::SCHEME {
	xmatrix = Some(challenge);
	break;
	}
	}
	let Some(xmatrix) = xmatrix else {
	return Err(XMatrixParseError::NotFound);
	};

with

let Some(xmatrix) = parser
    .into_iter()
    .filter_map(Result::ok)
    .find(|challenge| challenge.scheme == XMatrix::SCHEME)
else {
    return Err(XMatrixParseError::NotFound);
};

Also I am not sure how we should be handling multiple X-Matrix challenges, or if http-auth will do that for us.

Note that a response can have multiple challenges with the same auth-scheme but with different realms.

This should be handled by http-auth then, as this is mentioned in the RFC.

[zecakeh]

Thanks for the review! Next time, could you leave your comments directly on the code? It's often easier to have discussions in threads.

Indeed, we could simplify it. Sometimes I find chains hard to follow but here it should be fine.

Also I am not sure how we should be handling multiple X-Matrix challenges, or if http-auth will do that for us.

Is that a thing we should support? I don't think it was supported before. I don't see anything about it in the spec. http-auth returns a Vec so I assume it doesn't try to deduplicate schemes, but we only handle the first one in the list.

[zecakeh]

Indeed, we could simplify it. Sometimes I find chains hard to follow but here it should be fine.

So the reason I didn't do a chain is actually because we want to return any parser error we get, which your chain doesn't do. Ideally we would need Iterator::try_find(), but that is still experimental. I could maybe write something with Iterator::try_fold(), but that would probably not be as easy to follow.

[avdb13]

Thanks for the review! Next time, could you leave your comments directly on the code? It's often easier to have discussions in threads.

Indeed, we could simplify it. Sometimes I find chains hard to follow but here it should be fine.

Also I am not sure how we should be handling multiple X-Matrix challenges, or if http-auth will do that for us.

Is that a thing we should support? I don't think it was supported before. I don't see anything about it in the spec. http-auth returns a Vec so I assume it doesn't try to deduplicate schemes, but we only handle the first one in the list.

You probably want to call while let Some(_) = parser.into_iter().next() and match on the result. Folding with a fallible function seems fine to me too.

[Kladki]

I think that's about it, everything else looks good. 👍

[Kladki]

#1805 (comment)

Also I am not sure how we should be handling multiple X-Matrix challenges, or if http-auth will do that for us.

Is that a thing we should support? I don't think it was supported before. I don't see anything about it in the spec. http-auth returns a Vec so I assume it doesn't try to deduplicate schemes, but we only handle the first one in the list.

According to MSC4029, we should validate that all X-Matrix signatures are valid. It is currently just an MSC though, so we don't have to follow it.

[Kladki]

Now I think about it, HeaderValue is only ever meant to represent a single value anyways, so why do we iterate over challenges?

Nvm.

MDN also says that there is only supposed to be a single scheme as well for a given header value.

[Kladki]

scottlamb/http-auth#2

[zecakeh]

But you could in theory have several key and sig. I don't know what format Synapse supports (according to the MSC).

[Kladki]

Sure, but the thing is that this method can only used to extract a single header (since it only parses the value, i.e. not Authorization).

[zecakeh]

You could have (which afaiu is supported by the RFC):

X-Matrix origin="yours.org",destination="mine.org",key="key1",sig="sig1",key="key2",sig="sig2"

Or if what Synapse does is support several challenges in the value (even if it's not supported in the Authorization header according to the RFC), it could be:

X-Matrix origin="yours.org",destination="mine.org",key="key1",sig="sig1", X-Matrix origin="yours.org",destination="mine.org",key="key2",sig="sig2"

Both are still a single header value.

[Kladki]

The former is not allowed

[Kladki]

Hmm, maybe the latter is though. 🤔

The latter is supported on WWW-Authenticate, but not on Authorization.

[zecakeh]

No the latter isn't, not for the Authorization header. In any case, if the MSC says that Synapse supports several signatures, it must not be RFC-compliant. The only RFC-compliant way to have one would be to have a list inside the value of the parameter, but then that is not a backwards-compatible change.

In any case, as long as the MSC is not clearer about the syntax, I believe we should just handle the first credentials we see, and ignore other ones for now for forwards-compatibility.

[Kladki]

I added another comment to the MSC, to ask how this would possibly occur.

[zecakeh]

Woops I had forgotten to push the commit.

[Kladki]

I was wondering if that was the case, or if you were just waiting to resolve the last thread. 😅

[Kladki]

The sig value could violate these conditions, should we make a new type for it so it can be validated?

[Kladki]

Example:

    #[test]
    fn xmatrix_auth_1_3_extended_ascii() {
        let header = HeaderValue::from_static("X-Matrix origin=\"origin.hs.example.com\"  ,     destination=\"destination.hs.example.com\",key=\"ed25519:key1\", sig=\"ABCDEFÇ...\"");
        let credentials = XMatrix::try_from(&header).unwrap();

        assert_eq!(credentials.origin, "origin.hs.example.com");
        assert_eq!(credentials.destination.unwrap(), "destination.hs.example.com");
        assert_eq!(credentials.key, "ed25519:key1");
        assert_eq!(credentials.sig, "ABCDEFÇ...");
    }

---- authorization::tests::xmatrix_auth_1_3_extended_ascii stdout ----
thread 'authorization::tests::xmatrix_auth_1_3_extended_ascii' panicked at /home/alpha/.local/share/cargo/registry/src/index.crates.io-6f17d22bba15001f/http-1.1.0/src/header/value.rs:95:17:
index out of bounds: the len is 0 but the index is 0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    authorization::tests::xmatrix_auth_1_3_extended_ascii

[Kladki]

Note: once we ensure that the conversion cannot fail, we should probably switch the TryFrom to From, and then move the expect() in there.

[Kladki]

Hmm, I'm not sure anymore, since the following passes fine:

    #[test]
    fn xmatrix_auth_1_3_extended_ascii() {
        let creds = XMatrix {
            origin: "origin.hs.example.com".try_into().unwrap(),
            destination: Some("destination.hs.example.com".try_into().unwrap()),
            key: "ed25519:key1".try_into().unwrap(),
            sig: "ABCDEFÇ...".to_owned(),
        };

        let _header = creds.encode();
    }

[Kladki]

Seems like it is escaped (when?):

"X-Matrix destination=\"destination.hs.example.com\",key=\"ed25519:key1\",origin=\"origin.hs.example.com\",sig=\"ABCDEF\xc3\x87...\""

So is there a reason to have an expect() in encode instead of just moving it to From?

[Xiretza]

I don't see any evidence of escaping there, that's just String's Debug implementation.

[Kladki]

Ah, seems you are correct, my bad.

[Xiretza]

This breaks unquoted colons in values, which the spec says should be supported, and which were explicitly supported with the previous parser:

For compatibility with older servers, the recipient should allow colons to be included in values without requiring the value to be enclosed in quotes.

It also doesn't escape quotes and backslashes in quoted fields, solved by #1830.

The test case from #1830 covers both issues: X-Matrix origin=example.com:1234,key="ed25519:key1",sig="abc\"def\\",

[Kladki]

About the colon issue, see #1805 (comment)

And yeah you seem to be right about the escaping.