Validation error: unexpected issuer URI

[findesgh]

When I do

oidc create public test --issuer https://example.com/auth/oauth2/realms/root --client-id foo

on an internal corporate issuer I'm working with, I get (cf. the additional port 443):

Validation error: unexpected issuer URI `https://example.com:443/auth/oauth2/realms/root` (expected `https://example.com/auth/oauth2/realms/root`)

Doing

oidc create public test --issuer https://example.com:443/auth/oauth2/realms/root --client-id foo

results in the same error.

I'm not overly familiar with Rust nor with OIDC, but I've looked into this a little bit and AFAICT, there's four places I could raise this issue with:

1. oidc-cli
2. rust-url (which you use to parse the issuer URL provided on the CLI)
3. openidconnect-rs (since it ultimately raises the above error)
4. the corporate issuer (they've included the superfluous port in their config)

I hope you don't mind that I'm starting with you 😛.

From a user perspective I feel like I should be able to specify the superfluous port in my issuer URL and have oidc-cdi consider it. AFAICT this information is lost as soon as rust-url parses the URL because it never keeps default ports. I can't tell if there's a reasonable way for you to work around that.

rust-url implements the URL standard, which says that default ports are to be set to null. So it does what it's supposed to do.

I can't tell, if openidconnect-rs should ignore default ports when comparing URLs. Happy to raise the issue there, however, if you think that's the right place.

I'll definitely try 4., but I don't expect too much there, even if what they do is in violation of some standard (which I haven't been able to verify up until this point). God knows what might break if they change that so they'll probably not do it without a very good reason and I fear my discomfort in not being able to use oidc-cli is not good enough of a reason 😛.

[ctron]

Hm, I get the idea. However, I'd also say that this repo is the last I'd see responsible for this 😬 (yes, I might be biased here 🤣 )

I really would not want to introduce any special handling. Not even sure how to responsibly do that. However, why not try using :443 as part of the command line argument?

[findesgh]

I understand that. Introducing special handling is generally not desirable.

What do you mean when you say "use :443 as part of the command line argument"? I would understand that as meaning to try

oidc create public test --issuer https://example.com:443/auth/oauth2/realms/root --client-id foo

which I already did to no avail (see above). Or do you mean something else?

[ctron]

Ah sorry, I missed that. No, that is what I meant.

Right now, I don't have a good answer or idea how to fix this (in this crate). I'd just leave it open until we find a good solution, as I think it's a valid issue.

[TobiX]

This seems like a general normalization problem to me and happens on other OAuth2 providers, for example GitLab and Forgejo:

09:23:28 [ERROR]   1: Validation error: unexpected issuer URI `https://gitlab.com` (expected `https://gitlab.com/`)

and

09:34:48 [ERROR]   1: Validation error: unexpected issuer URI `https://codeberg.org` (expected `https://codeberg.org/`)

(Or is this a different issue?)

AFAIK OIDC is quite strict on normalization (see https://openid.net/specs/openid-connect-discovery-1_0.html#IdentifierNormalization) and Forgejo already had issues with that: https://codeberg.org/forgejo/forgejo/issues/7941

(This would also make @findesgh's internal issuer spec-non-compliant which puts us between a rock and a hard place 😔 )

[findesgh]

I've looked into this a bit more and I'd say the problems are related. It seems that issues of this kind are somewhat common with openidconnect-rs (ramosbugs/openidconnect-rs#226 and ramosbugs/openidconnect-rs#231, ramosbugs/openidconnect-rs#77).

AFAICT ramosbugs/openidconnect-rs#77 (comment) summarizes it quite well.

Applied to this project, it would read something like this: as encouraged by openidconnect-rs, oidc-cli uses Url to create an IssuerUrl. This results in normalizations being applied to the user-supplied URL. When comparing this IssuerUrl with what the issuer claims its URL is, openidconnect-rs, however, relies on string comparison. This means that superfluous default ports or missing trailing slashes result in an error which can not be addressed by the user by supplying a correspondingly adapted URL.

This could IMHO be fixed/worked around by oidc-cli by creating the IssuerUrl from the raw user-supplied String-URL (which is possible and, it seems, encouraged for by the author of openidconnect-rs for issues of this kind: ramosbugs/openidconnect-rs#77 (comment)). Probably not the most beautiful solution, however, since (as noted in ramosbugs/openidconnect-rs#77 (comment)) intent is lost when storing a URL in a string.

Moreover, the user wouldn't immediately receive a clear error, when they supply an invalid URL. This problem could, of course, be addressed by parsing the string and discarding the result if successful (I have no idea what the "rusty" way of doing this would look like).

I believe that ramosbugs/openidconnect-rs#77 (comment) indicates that upstream considers this not be an issue and that we should correspondingly not expect any changes from their side.

Regarding spec-conformity: I don't know if what my or your @TobiX issuer do is compliant, but I'd argue that enabling users to adapt to problems like ours by correspondingly modifying the URLs they supply can't hurt. I'm not an expert, though, and certainly biased here 😛.

[TobiX]

I hate the OIDC spec for this, because it runs afoul with how most URL libraries work... The spec says:

OpenID Providers supporting Discovery MUST make a JSON document available at the path formed by concatenating the string /.well-known/openid-configuration to the Issuer.

There is no URL semantic here, just stupid string concatenation...

OTOH, 4.1 says:

If the Issuer value contains a path component, any terminating / MUST be removed before appending /.well-known/openid-configuration

So the spec isn't even consistent...

[ctron]

Thank you all for looking into this.

I try to summarize, so we can plan next steps:

• Most likely, any dependency that oidc-cli has, will not fix the issue -> if possible, we should do something about it
• openidconnect-rs accepts a raw string (as part of IssuerUrl, and this is the way we could achieve such a workaround
• In order to rule out invalid URLs early, we could parse it (just for testing) and then keep the string instead

If that summary is correct, I believe a fix should be simple.

[ctron]

Ok, I created a PR under this assumption. You should be able to test this using:

cargo install --git https://github.com/ctron/oidc-cli oidc-cli --branch feature/dont_parse_url_1

[findesgh]

Tested and works for my case, thank you!

The only thing I've noticed is that error messages for invalid URLs are now not as pretty as they used to be (which makes sense, I guess, since they're not immediately parsed into a Url anymore).

For example

oidc create confidential test --issuer example.com --client-id test --client-secret asdf

on the feature branch will result in

06:00:53 [ERROR] invalid issuer URL
06:00:53 [ERROR]   1: relative URL without a base

while on main it results in

error: invalid value 'example.com' for '--issuer <ISSUER>': relative URL without a base

For more information, try '--help'.

[findesgh]

I'm not familiar with how this works in detail, but just a thought: couldn't the issuer in CreateCommon be of type IssuerUrl?

[ctron]

Ah yes. I guess, we could sort that out as well.

[ctron]

Ok, pushed an update. Maybe you can re-try. Using the approach you suggested.