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

ui: [BUGFIX] Properly encode non-URL safe characters in OIDC responses #10901

Merged
merged 4 commits into from
Aug 24, 2021

Conversation

johncowen
Copy link
Contributor

This PR fixes 2 problems with our OIDC flow in the UI, the first is straightforwards, the second is relatively more in depth:

1: A typo (1.10.1 only)

During #10503 we injected our settings service into the our oidc-provider service, there are some comments in the PR as to the whys and wherefores for this change (https://github.com/hashicorp/consul/pull/10503/files#diff-aa2ffda6d0a966ba631c079fa3a5f60a2a1bdc7eed5b3a98ee7b5b682f1cb4c3R28)

During development, this manifests itself as a javascript error:

Screenshot 2021-08-24 at 13 53 12

Whereas during production, it simply breaks with no clear reason as to what the problem is.

Fixing the typo so it was no longer looking for an unknown service (repository/settings > settings)
fixed this part of the issue which was added as part of the above PR here

2: URL encoding (1.9.x, 1.10.x)

TL;DR: /oidc/authorize/provider/with/slashes/code/with/slashes/status/with/slashes should be /oidc/authorize/provider%2Fwith%2Fslashes/code%2Fwith%2Fslashes/status%2Fwith%2Fslashes

When we receive our authorization response back from the OIDC 3rd party, we POST the code and status data from that response back to consul via acallback as part of the OIDC flow. From what I remember back when this feature was originally added, the method is a POST request to avoid folks putting secret-like things into API requests/URLs/query params that are more likely to be visible to the human eye, and POSTing is expected behaviour.

Additionally, in the UI we identify all external resources using unique resource identifiers. Our OIDC flow uses these resources and their identifiers to perform the OIDC flow using a declarative state machine. If any information in these identifiers uses non-URL-safe characters then these characters require URL encoding and we added a helper a while back to specifically help us to do this once we started using this for things that required URL encoding.

In the majority of the examples you see for OIDC auth flow use examples like:

https://client.example.com/cb?code=SplxlOBeZQQYbYS6WxSbIA&state=xyz

// and

grant_type=authorization_code&code=SplxlOBeZQQYbYS6WxSbIA&redirect_uri=https%3A%2F%2Fclient%2Eexample%2Ecom%2Fcb

...etc

In all the examples in the OIDC spec it 'seems' like code should be alpha-numeric, but if you dig a little deeper code is specified as:

 code       = 1*VSCHAR

which therefore means it can contain non-URL-safe characters 💥

The final fix here make sure that we URL encode code and status before using them with one of our unique resource identifiers, just like we do with the majority of other places where we use these identifiers.

Both fixes here are required for the 1.10.x branch (the typo was added in 1.10.1), our 1.9.x branch doesn't require the first fix for the typo and requires a slightly different way of writing the second fix. Therefore I'll be backporting it here only for changelog reasons, but then manually fixing the backport when it fails.

@johncowen johncowen added type/bug Feature does not function as expected theme/ui Anything related to the UI labels Aug 24, 2021
@johncowen johncowen requested a review from kaxcode August 24, 2021 13:22
Copy link
Contributor

@kaxcode kaxcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - lmk how you want to do the changelogs.

@vercel vercel bot temporarily deployed to Preview – consul-ui-staging August 24, 2021 14:18 Inactive
@vercel vercel bot temporarily deployed to Preview – consul August 24, 2021 14:18 Inactive
@johncowen johncowen merged commit 05a28c3 into main Aug 24, 2021
@johncowen johncowen deleted the ui/bugfix/sso branch August 24, 2021 15:58
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/432487.

@hc-github-team-consul-core
Copy link
Collaborator

🍒✅ Cherry pick of commit 05a28c3 onto release/1.10.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Aug 24, 2021
#10901)

This commit fixes 2 problems with our OIDC flow in the UI, the first is straightforwards, the second is relatively more in depth:

1: A typo (1.10.1 only)

During #10503 we injected our settings service into the our oidc-provider service, there are some comments in the PR as to the whys and wherefores for this change (https://github.com/hashicorp/consul/pull/10503/files#diff-aa2ffda6d0a966ba631c079fa3a5f60a2a1bdc7eed5b3a98ee7b5b682f1cb4c3R28)

Fixing the typo so it was no longer looking for an unknown service (repository/settings > settings)
fixed this.

2: URL encoding (1.9.x, 1.10.x)

TL;DR: /oidc/authorize/provider/with/slashes/code/with/slashes/status/with/slashes should be /oidc/authorize/provider%2Fwith%2Fslashes/code%2Fwith%2Fslashes/status%2Fwith%2Fslashes

When we receive our authorization response back from the OIDC 3rd party, we POST the code and status data from that response back to consul via acallback as part of the OIDC flow. From what I remember back when this feature was originally added, the method is a POST request to avoid folks putting secret-like things into API requests/URLs/query params that are more likely to be visible to the human eye, and POSTing is expected behaviour.

Additionally, in the UI we identify all external resources using unique resource identifiers. Our OIDC flow uses these resources and their identifiers to perform the OIDC flow using a declarative state machine. If any information in these identifiers uses non-URL-safe characters then these characters require URL encoding and we added a helper a while back to specifically help us to do this once we started using this for things that required URL encoding.

The final fix here make sure that we URL encode code and status before using them with one of our unique resource identifiers, just like we do with the majority of other places where we use these identifiers.
@hc-github-team-consul-core
Copy link
Collaborator

🍒❌ Cherry pick of commit 05a28c3 onto release/1.9.x failed! Build Log

johncowen added a commit that referenced this pull request Sep 30, 2021
Our DataSource came in very iteratively, when we first started using it we specifically tried not to use it for things that would require portions of the @src="" attribute to be URL encoded (so things like service names couldn't be used, but dc etc would be fine). We then gradually added an easy way to url encode the @src="" attributes with a uri helper and began to use the DataSource component more and more. This meant that some DataSource usage continued to be used without our uri helper.

Recently we hit #10901 which was a direct result of us not encoding @src values/URIs (I didn't realise this was one of the places that required URL encoding) and not going back over things to finish things off once we had implemented our uri helper, resulting in ~half of the codebase using it and ~half of it not.

Now that almost all of the UI uses our DataSource component, this PR makes it even harder to not use the uri helper, by wrapping the string that it requires in a private URI class/object, that is then expected/asserted within the DataSource component/service. This means that as a result of this PR you cannot pass a plain string to the DataSource component without seeing an error in your JS console, which in turn means you have to use the uri helper, and it's very very hard to not URL encode any dynamic/user provided values, which otherwise could lead to bugs/errors similar to the one mentioned above.

The error that you see when you don't use the uri helper is currently a 'soft' dev time only error, but like our other functionality that produces a soft error when you mistakenly pass an undefined value to a uri, at some point soon we will make these hard failing "do not do this" errors.

Both of these 'soft error' DX features have been used this to great effect to implement our Admin Partition feature and these kind of things will minimize the amount of these types of bugs moving forwards in a preventative rather than curative manner. Hopefully these are the some of the kinds of things that get added to our codebase that prevent a multitude of problems and therefore are often never noticed/appreciated.

Additionally here we moved the remaining non-uri using DataSources to use uri (that were now super easy to find), and also fixed up a place where I noticed (due to the soft errors) where we were sometimes passing undefined values to a uri call.

The work here also led me to find another couple of non-important 'bugs' that I've PRed already separately, one of which is yet to be merged (#11105), hence the currently failing tests here. I'll rebase that once that PR is in and the tests here should then pass 🤞

Lastly, I didn't go the whole hog here to make DataSink also be this strict with its uri usage, there is a tiny bit more work on DataSink as a result of recently work, so I may (or may not) make DataSink equally as strict as part of that work in a separate PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI type/bug Feature does not function as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants