Issues with IgnoreLocalPortUrl

[valkum]

Bug report

We try to use oxide-auth to test some OAuth clients in tests. During that, we encountered some issues regarding the usefulness of the current implementation of IgnoreLocalPortUrl.

localhost vs. 127.0.0.1

https://www.rfc-editor.org/rfc/rfc8252.html#section-7.3 and https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-23#section-4.1.3 call out to also support 127.0.0.1 for the special case of ignoring the port when validating the client-passed redirect_uri. As 127.0.0.1 (and probably the IPv6 localhost address) is considered to be safer as the hostname, I would expect that IgnoreLocalPortUrl also takes the loopback addresses.

Completing a flow

It does not seem to be possible to complete a authorization code flow.
During /authorization, the redirect is generated by:

1. Using the redirect_uri of the PreGrant.
2. PreGrant (for ClientMap) gets the redirect_uri from BoundClient (primitives/registrar.rs#negotiate)
3. BoundClient (for ClientMap) gets the redirect_uri from the registered client (primitives/registrar.rs#bound_redirect)
   This results in a browser being redirected to the wrong location.

Reproduction

Relevant environment (http frontend library, OS, network setup?)

Steps to reproduce the behavior:

1. Using the actix example, setup the generic Endpoint using

IgnoreLocalPortUrl::new("http://localhost/endpoint")
   .unwrap()
   .into(),

2. Open http://localhost:8020/authorize?response_type=code&client_id=LocalClient&redirect_uri=http://localhost:8021/endpoint

Expected behaviour

According to rfc8252, the redirect should happen based on the requests' redirect_uri.

One simple fix would be to change bound_redirect in registrar.rs to assign the following to registered_url

match registered {
   RegisteredUrl::IgnorePortOnLocalhost(_) => {
      RegisteredUrl::Exact(url.clone().into_owned())
   }
   url @ _ => url.clone(),
}

While this fixes the underlying issue, I am not sure of the consequences of this change. Having RegisteredUrl in PreGrant conveys, semantically by the type, that the content is the URL that is registered with the Client, but the field name and usage of the field suggests otherwise.

Tracking pull request

• A pull request does not yet exist

[HeroicKatora]

This is in reference to the section here:

oxide-auth/oxide-auth/src/primitives/registrar.rs

Lines 786 to 800 in 303e85d

	let registered_url = match bound.redirect_uri {
	None => client.redirect_uri.clone(),
	Some(ref url) => {
	let original = std::iter::once(&client.redirect_uri);
	let alternatives = client.additional_redirect_uris.iter();
	if let Some(registered) = original
	.chain(alternatives)
	.find(|&registered| *registered == *url.as_ref())
	{
	registered.clone()
	} else {
	return Err(RegistrarError::Unspecified);
	}
	}
	};

, correct?

The issue is that the argument is never utilized as the return value. This is a bug introduced with the logic having been designed around the older RFCs first. A cleaner fix might add this behavior to RegisteredUrl as a method, such that the IgnorePort variant can return its comparison argument ("parse, don't validate"). The current implementation of == doesn't allow this.

fn check_permitted(&self: &RegisteredUrl, other: &ExactUrl) -> Option<ExactUrl> {
    // ..
}

I also agree that the name in the attribute is confusing with that usage. A breaking change would simply carry both pieces of information into the returned object, the registered one as RegisteredUrl and the permitted redirect uri as an ExactUrl.

[HeroicKatora]

Coincidentally the fix to this logic might also unblock the addition of wildcard registrations.

[ctron]

I just ran into the same issue. Is this already fixed? If not, what would it take to get it done?

[HeroicKatora]

The main effort should be expanding the test suite such that the new match is considered.

The fix proposed seems already promising and implementable as is, changing the line (

oxide-auth/oxide-auth/src/primitives/registrar.rs

Line 797 in acf956b

registered.clone()

) to sometimes return the provided URL instead of the registered one. The equality comparison should already have take care of any other semantic ambiguity. (It was caution to prompted the initial implementation of cloning the registered URI instead of the client-controlled one in the first place).

The block assigning registered_url might be suitable for being extracted to a (public) method on Client for use by other implementors of the trait which may run into this issue, too.

[ctron]

Thanks to your pointer it seemed quite easy. I hope PR #190 is what you had in mind.