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

issuer in discovery document contains trailing '/' #1482

Closed
leo-baltus opened this issue Jun 25, 2019 · 16 comments
Closed

issuer in discovery document contains trailing '/' #1482

leo-baltus opened this issue Jun 25, 2019 · 16 comments
Labels
feat New feature or request.
Milestone

Comments

@leo-baltus
Copy link

Describe the bug
Setting urls.self.issuer in the configuration without a trailing slash results in the issuer being advertised in the discovery document with a trailing slash. As clients need to validate this information it should be deterministic.

Reproducing the bug

docker run -p 4444:4444 \
    -e URLS_SELF_ISSUER=https://auth.example.com \
    -e DSN=memory \
    oryd/hydra:v1.0.0-rc.16 serve all --dangerous-force-http

curl -s localhost:4444/.well-known/openid-configuration | jq .issuer
"https://auth.example.com/"

I expected "https://auth.example.com"

Server logs

time="2019-06-25T11:44:32Z" level=warning msg="HTTPS disabled. Never do this in production."
time="2019-06-25T11:44:36Z" level=info msg="started handling request" method=GET remote="172.17.0.1:40196" request=/.well-known/openid-configuration
time="2019-06-25T11:44:36Z" level=info msg="completed handling request" measure#hydra/public: https://auth.example.com/.latency=814000 method=GET remote="172.17.0.1:40196" request=/.well-known/openid-configuration status=200 text_status=OK took="814µs"

Additional context
Other OP's:

curl -s https://accounts.google.com/.well-known/openid-configuration | jq .issuer
"https://accounts.google.com"

Okta https://developer.okta.com/docs/reference/api/oidc/#response-example-success-6

Only auth0 seems to add a trailing slash as far as I can see.

When migrating to hydra I would like to avoid reconfiguring all clients.

@aeneasr
Copy link
Member

aeneasr commented Jun 25, 2019

Dupe of #1041

@leo-baltus
Copy link
Author

I see. The point is that the '/' addition cannot be undone. If I wanted a trailing slash I could have said so in the configuration.

@aeneasr
Copy link
Member

aeneasr commented Jun 25, 2019

I don't understand the problem, to be honest. The issuer value is advertised in .well-known/openid-configuration and oidc compliant clients should adhere to the issuer value from there. It therefore should not matter if there's a trailing slash or not.

@leo-baltus
Copy link
Author

leo-baltus commented Jun 25, 2019

The client should verify the issuer. If you have existing clients, with some OP that does not state the trailings slash and migrate to Hydra, verification fails because of this.
Edit: removed some wording, didn't mean to give the wrong impression, sorry about that.

@aeneasr
Copy link
Member

aeneasr commented Jun 25, 2019

Again, the issuer url should be fetched from /.well-known/openid-configuration and not be hardcoded. You point your client to e.g. https://foo/.well-known/openid-configuration and then you point it to https://bar/.well-known/openid-configuration�. The client fetches the issuer value for verification from that JSON Payload.

Additionally, this allows the OP to update the configuration (e.g. new issuer url) without having to update all of the clients by hand.

If the clients consuming your OP are oidc conformant, they should verify the ID Token using that mechanism. That is the whole point of OIDC Dynamic Discovery.

@leo-baltus
Copy link
Author

How would the client know about https://bar/.well-known/openid-configuration ? So far I am assuming updating the OP should be an in-place replacement.

The error I was facing was this from line:
https://github.com/coreos/go-oidc/blob/8ae1da518bd4d9d5a5909090a184af30f336436d/oidc.go#L124

@leo-baltus
Copy link
Author

Maybe superfluous, but from https://tools.ietf.org/html/draft-ietf-oauth-discovery-01#section-5

   By default, for historical reasons, unless an application-specific
   well-known URI path suffix is registered and used for an application,
   the client for that application SHOULD use the well-known URI path
   suffix "openid-configuration" and publish the metadata document at
   the path formed by concatenating "/.well-known/openid-configuration"
   to the authorization server's issuer identifier.  As described in
   Section 5, despite the identifier
   "/.well-known/openid-configuration", appearing to be OpenID-specific,
   its usage in this specification is actually referring to a general
   OAuth 2.0 feature that is not specific to OpenID Connect.

I conclude that the '/' is part of the .well-known part, not the issuer.

@aeneasr
Copy link
Member

aeneasr commented Jun 26, 2019

How would the client know about https://bar/.well-known/openid-configuration ? So far I am assuming updating the OP should be an in-place replacement.

It's a standard. go-oidc uses that endpoint: https://github.com/coreos/go-oidc/blob/8ae1da518bd4d9d5a5909090a184af30f336436d/oidc.go#L97

The error I was facing was this from line:
https://github.com/coreos/go-oidc/blob/8ae1da518bd4d9d5a5909090a184af30f336436d/oidc.go#L124

The CoreOS client apparently uses the parameter to initialize the client with that issuer value, and compares it with the value from openid-configuration. If they mismatch, it throws an error. This is a choice by the library, not by the OpenID Connect specification. Please contact the maintainers to allow small mismatches in the configuration, such as a missing trailing slash.

I conclude that the '/' is part of the .well-known part, not the issuer.

That conclusion is unfortunately wrong and also stems from OAuth 2.0 Discovery, which is unrelated to OpenID Connect.

I'm closing this because we won't change the behavior in this system for the points made above. As stated in the other issue, not enforcing the trailing (or not) slash causes even more confusion with developers and punishes small mistakes (like forgetting the trailing slash in some config) severely with hour-long debugging sessions due to a small typo.

@aeneasr aeneasr closed this as completed Jun 26, 2019
@aeneasr
Copy link
Member

aeneasr commented Jul 3, 2019

fwiw coreos/go-oidc#203

@aeneasr
Copy link
Member

aeneasr commented Jul 3, 2019

The issuer URL used to retrieve the OpenID Configuration Discovery must be equal to the one returned by the discovery document. We could therefore add an option to append or strip trailing slashes in the configuration item. This is required because we would introduce a breaking change to existing deployments and must therefore keep existing behavior as is.

@vinckr
Copy link
Member

vinckr commented Aug 31, 2020

Does this #2014 close this issue? @tacurran @k9ert

@aeneasr
Copy link
Member

aeneasr commented Aug 31, 2020

No!

@diefans
Copy link

diefans commented Mar 17, 2021

anything new about this?

@github-actions
Copy link

I am marking this issue as stale as it has not received any engagement from the community or maintainers in over half a year. That does not imply that the issue has no merit! If you feel strongly about this issue

  • open a PR referencing and resolving the issue;
  • leave a comment on it and discuss ideas how you could contribute towards resolving it;
  • open a new issue with updated details and a plan on resolving the issue.

We are cleaning up issues every now and then, primarily to keep the 4000+ issues in our backlog in check and to prevent maintainer burnout. Burnout in open source maintainership is a widespread and serious issue. It can lead to severe personal and health issues as well as enabling catastrophic attack vectors.

Thank you for your understanding and to anyone who participated in the issue! 🙏✌️

If you feel strongly about this issues and have ideas on resolving it, please comment. Otherwise it will be closed in 30 days!

@github-actions github-actions bot added the stale Feedback from one or more authors is required to proceed. label Sep 21, 2021
@aeneasr
Copy link
Member

aeneasr commented Sep 21, 2021

Marked as stale in error.

@aeneasr aeneasr added this to the unplanned milestone Sep 21, 2021
@aeneasr aeneasr added feat New feature or request. and removed stale Feedback from one or more authors is required to proceed. labels Sep 21, 2021
@aeneasr aeneasr modified the milestones: unplanned, v1.11 Sep 21, 2021
@aeneasr aeneasr modified the milestones: v1.11, v2.0 Oct 13, 2021
aeneasr added a commit that referenced this issue Jun 14, 2022
BREAKING CHANGE: The `iss` (issuer) value no longer appends a trailing slash but instead uses the raw value set in the config.

Setting

```yaml
urls:
  self:
    issuer: https://auth.example.com
```

has changed

```patch
-  "iss": "https://auth.example.com/"
+  "iss": "https://auth.example.com"
```

To set a trailing slash make sure to set it in the config value:

```yaml
urls:
  self:
    issuer: https://auth.example.com/
```

Closes #1482
aeneasr added a commit that referenced this issue Jun 14, 2022
BREAKING CHANGE: The `iss` (issuer) value no longer appends a trailing slash but instead uses the raw value set in the config.

Setting

```yaml
urls:
  self:
    issuer: https://auth.example.com
```

has changed

```patch
-  "iss": "https://auth.example.com/"
+  "iss": "https://auth.example.com"
```

To set a trailing slash make sure to set it in the config value:

```yaml
urls:
  self:
    issuer: https://auth.example.com/
```

Closes #1482
@aeneasr aeneasr closed this as completed Jun 14, 2022
aeneasr added a commit that referenced this issue Jun 27, 2022
BREAKING CHANGE: The `iss` (issuer) value no longer appends a trailing slash but instead uses the raw value set in the config.

Setting

```yaml
urls:
  self:
    issuer: https://auth.example.com
```

has changed

```patch
-  "iss": "https://auth.example.com/"
+  "iss": "https://auth.example.com"
```

To set a trailing slash make sure to set it in the config value:

```yaml
urls:
  self:
    issuer: https://auth.example.com/
```

Closes #1482
grantzvolsky pushed a commit that referenced this issue Aug 1, 2022
BREAKING CHANGE: The `iss` (issuer) value no longer appends a trailing slash but instead uses the raw value set in the config.

Setting

```yaml
urls:
  self:
    issuer: https://auth.example.com
```

has changed

```patch
-  "iss": "https://auth.example.com/"
+  "iss": "https://auth.example.com"
```

To set a trailing slash make sure to set it in the config value:

```yaml
urls:
  self:
    issuer: https://auth.example.com/
```

Closes #1482
aeneasr added a commit that referenced this issue Aug 1, 2022
BREAKING CHANGE: The `iss` (issuer) value no longer appends a trailing slash but instead uses the raw value set in the config.

Setting

```yaml
urls:
  self:
    issuer: https://auth.example.com
```

has changed

```patch
-  "iss": "https://auth.example.com/"
+  "iss": "https://auth.example.com"
```

To set a trailing slash make sure to set it in the config value:

```yaml
urls:
  self:
    issuer: https://auth.example.com/
```

Closes #1482
@gabricar
Copy link

gabricar commented Aug 17, 2022

Any chance that 1.11.x gets this fix?

aeneasr added a commit that referenced this issue Aug 18, 2022
BREAKING CHANGE: The `iss` (issuer) value no longer appends a trailing slash but instead uses the raw value set in the config.

Setting

```yaml
urls:
  self:
    issuer: https://auth.example.com
```

has changed

```patch
-  "iss": "https://auth.example.com/"
+  "iss": "https://auth.example.com"
```

To set a trailing slash make sure to set it in the config value:

```yaml
urls:
  self:
    issuer: https://auth.example.com/
```

Closes #1482
aeneasr added a commit that referenced this issue Sep 5, 2022
BREAKING CHANGE: The `iss` (issuer) value no longer appends a trailing slash but instead uses the raw value set in the config.

Setting

```yaml
urls:
  self:
    issuer: https://auth.example.com
```

has changed

```patch
-  "iss": "https://auth.example.com/"
+  "iss": "https://auth.example.com"
```

To set a trailing slash make sure to set it in the config value:

```yaml
urls:
  self:
    issuer: https://auth.example.com/
```

Closes #1482
aeneasr added a commit that referenced this issue Sep 7, 2022
BREAKING CHANGE: The `iss` (issuer) value no longer appends a trailing slash but instead uses the raw value set in the config.

Setting

```yaml
urls:
  self:
    issuer: https://auth.example.com
```

has changed

```patch
-  "iss": "https://auth.example.com/"
+  "iss": "https://auth.example.com"
```

To set a trailing slash make sure to set it in the config value:

```yaml
urls:
  self:
    issuer: https://auth.example.com/
```

Closes #1482
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request.
Projects
None yet
Development

No branches or pull requests

5 participants