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

NLX - Move error reporting from SSO dashboard to NLX #99

Open
gene1wood opened this issue Apr 2, 2018 · 12 comments
Open

NLX - Move error reporting from SSO dashboard to NLX #99

gene1wood opened this issue Apr 2, 2018 · 12 comments

Comments

@gene1wood
Copy link
Contributor

gene1wood commented Apr 2, 2018

Background

The associated Auth0 rule issue to track the change on that side is mozilla-iam/auth0-deploy#186

Currently, when an error message needs to be conveyed to the user due to an Auth0 rule, that message is sent in a jwt to the sso dashboard to be rendered.

Problem

The navigation flow of sending a user from NLX to the sso dashboard and having to use the back button to get back to the RP isn't ideal. This is currently tracked in SSO dashboard mozilla-iam/sso-dashboard#205 and mozilla/parsys#253 as "improve error page wayfinding"

It's confusing to the user to go to an RP, try to login and then be sent to a site that they weren't on before (sso dashboard) and potentially haven't gone to before to see an error message about another site (their RP or the NLX)

Solution

Define a query parameter (or POST data expectation), for example display_error. When NLX gets a request with this parameter, it fetches the content and displays the error message in the NLX.

How to make this safe

You could expect the payload of this error message to be a JWT as is created now (example of how it's currently done, so that NLX could verify the signature of the payload before displaying it.

Alternatively, you could define templatized error messages in NLX and inject in the variables needed to display the message.

@hidde
Copy link
Contributor

hidde commented Jul 3, 2018

After experimenting a bit, it seems using parameters and templated messages seems the way forward. Thanks for suggesting this, @gene1wood.

I'm implementing it in NLX by using the action parameter for the following errors (this is the list of errors SSO Dashboard currently handles):

Possible errors

githubrequiremfa

Error message for when GitHub is used without MFA.

fxarequiremfa

Error message for when Firefox Accounts is used without MFA.

notingroup

Error message for when user does not have correct group permissions.

accesshasexpired

Error message for when access has expired.

primarynotverified

Error message for when primary identity was not verified.

incorrectaccount

Error message for when wrong account is being used to log in.

error_general

General error message.

Error

Oye, something went wrong.

The info NLX is currently missing

Name of the RP

For errors: not in group, access has expired, incorrect account.

Suggestion: let's pass it using rp_name={{ name }}, for example rp_name=Mozillians, rp_name=Expensify.

Name of the connection

For errors: primary not verified, incorrect account

Suggestion: let's pass it using connection_name={{ name }} for example connection_name=GitHub.

Name of user's most secure connection

For errors: incorrect account

(Can we pass this?) If so, let's pass it using most_secure_connection_name={{ name }}, for example most_secure_connection_name=Firefox%20Accounts.

@gene1wood

  1. I found these are all in the JWT that is being sent to SSO Dashboard. Could you double check if we're ok to pass the above info to NLX when a user hits an error? In ‘plain sight’?

  2. Do you know if we have such a thing as DEV rules in which we could try out posting errors to NLX instead of SSO Dashboard?

UX questions

@m-branson:

  1. Let me know if you'd like to review existing error texts as we could update them if you like.

  2. With the errors soon being in NLX, we can have any buttons that are in NLX. My suggestion would be to always have the ‘Need to log in with a different account?’ appear under an error, so that users go to the initial page and attempt another login, does that make sense?

Example:

Error - Oye something went wrong - button need to login with a different account?

and clicking ‘Need to log in with a different account’ would go to the initial page, i.e. this page:

login screen with email address field, log in with firefox, google or github

@mbransn
Copy link

mbransn commented Jul 3, 2018

@hidde

  1. Let me know if you'd like to review existing error texts as we could update them if you like.

Would be great to get these in a collected environment (sheet?) where they can be reviewed mutually. Additionally, would it be possible to link to the error screen in a staging environment so it can be visualized if necessary? (if there is a way to do this already without running through the entire flow, please tell me) My thinking here: make these touchpoints more visible to the entire team so we can mutually understand their user impact & feedback on them intentionally.

  1. With the errors soon being in NLX, we can have any buttons that are in NLX. My suggestion would be to always have the ‘Need to log in with a different account?’ appear under an error, so that users go to the initial page and attempt another login, does that make sense?

100% yes, for that use case/screen specifically. That said, there may be a more suitable CTA(s) based on the error. This is an additional reason for the sheet mentioned above, how might we better assess & curate the content needed for each error?

@hidde
Copy link
Contributor

hidde commented Jul 3, 2018

Would be great to get these in a collected environment (sheet?) where they can be reviewed mutually. Additionally, would it be possible to link to the error screen in a staging environment so it can be visualized if necessary?

Great idea! I've created a spreadsheet (only accessible to team members), it has all the error messages and a link to an example screen that uses the new error functionality that is available in NLX Dev.

100% yes, for that use case/screen specifically. That said, there may be a more suitable CTA(s) based on the error. This is an additional reason for the sheet mentioned above, how might we better assess & curate the content needed for each error?

Cool. I've defaulted to the same button for all now, and also the same title. They are easy to customise (but we don't have to… consistency is also good).

@hidde
Copy link
Contributor

hidde commented Jul 3, 2018

After discussion with @gdestuynder we agreed I will parse the JWT sent by the rule, rather than adding the variables as query parameters.

@hidde
Copy link
Contributor

hidde commented Jul 5, 2018

I've spent some time on decoding JWT client-side (easy) and verifying it client-side (harder). I have some further questions.

(Summarising this for myself to make sure I understand it correctly, please correct if wrong)

If we're using a JWT, we need to decode it (base64 to plain text) and verify its signature. Anyone can decode it, so just like with passing the values in as plain text, they would be public. So hiding from public is not why we want the JWT (I made incorrect assumption there). The reason we want JWT is that we can add a signature to the JWT, so that we can make ensure the information in it comes from where we think it comes from, in other words we verify authenticity.

Should we verify?

I'd like to discuss possible attacks further. The content we take from the parameters is plain text and inserted as such (with .textContent), so an attacker abusing this would have to do their abusing using text only.

There are probably risks I'm not thinking of, but I thought it'd be good to ask this again.

If we verify, what's the fallback?

I've looked at various libraries that can do verifying for us, but most of them also do signing and various other cryptographic operations. Hence they are quite big, Auth0's one, which seems like the most solid option, adds 300kb to the JS bundle, that's just over 300% increase. There is also a web standard, the Web Crypto API, which I'd prefer to use directly, it has less browser support (about 80% of all users; this may be more for our user base as we have more than average Firefox users).

If we go with verification, my proposal is:

  • users with supporting browsers: we verify the JWT, if we see it is malicious, we display general error instead of one based on the JWT (and send event about it to Google Analytics)
  • users with non-supporting browsers: no verification, we just show the data in the JWT

The alterative would be that we show nothing if we are unable to verify, but to me that feels a bit like throwing out the baby with the bathwater.

@gene1wood
Copy link
Contributor Author

Name of the RP

I'm assuming you mean pass these as additional arguments inside the JWT ya? (Not query parameters)

The client name (RP name) is in the jwt

https://github.com/mozilla-iam/auth0-deploy/blob/fff3e95d52e6078339f4e898c5fb6b479181d0ce/rules/Global-Function-Declarations.js#L17-L24

Name of the connection

The connection name is also in the jwt however you may need to translate it into something human readable

https://github.com/mozilla-iam/auth0-deploy/blob/fff3e95d52e6078339f4e898c5fb6b479181d0ce/rules/Global-Function-Declarations.js#L17-L24

Name of user's most secure connection

I believe this is provided

https://github.com/mozilla-iam/auth0-deploy/blob/399fc189cb2c5f055f6375e2cd9c884dbe3d0d5b/rules/force-users-login-most-secure-method.js#L92-L98

https://github.com/mozilla-iam/auth0-deploy/blob/fff3e95d52e6078339f4e898c5fb6b479181d0ce/rules/Global-Function-Declarations.js#L17-L24

I found these are all in the JWT that is being sent to SSO Dashboard. Could you double check if we're ok to pass the above info to NLX when a user hits an error? In ‘plain sight’?

Yes, we can pass these unencrypted but signed as long as we verify the signature before acting on them.

Do you know if we have such a thing as DEV rules in which we could try out posting errors to NLX instead of SSO Dashboard?

We could make a PR for the Auth0 rule change and deploy it to auth0 dev but not prod.


Oh, looks like you talked to kang and got most of this worked out

Should we verify?

Unless we can always guarantee that no data passed in the jwt will be returned on the screen we must verify. If that's not the case then there's no reason to use a jwt. I can easily see if we were to not verify, that in the future data would be added that is injected into the page, we'd forget that the jwt wasn't being verified, and then we'd have a security vulnerability.

I've looked at various libraries that can do verifying for us, but most of them also do signing and various other cryptographic operations. Hence they are quite big, Auth0's one, which seems like the most solid option, adds 300kb to the JS bundle, that's just over 300% increase.

Why not use a cdn based library that clients likely already have cached? Like this

https://cdnjs.com/libraries/jsrsasign

users with non-supporting browsers: no verification, we just show the data in the JWT

If we're passing any data in the jwt that is then injected into the NLX page, this isn't safe, even for non-supporting browsers. If we're not then there's no reason to be using a jwt

The alterative would be that we show nothing if we are unable to verify, but to me that feels a bit like throwing out the baby with the bathwater.

I don't think we should use a message integrity method that isn't supported on all our user's browsers (especially since we don't need to, jwt is supported on all browsers). If we don't want to use jwt we can spin our own message integrity solution but to me jwt seems like that simplest solution (which is why we used it for sso dashboard)

@gdestuynder
Copy link
Contributor

gdestuynder commented Jul 5, 2018

after discussing this morning, if we do not use a signature, then we have to drop redirect_uri as it could be used for phishing. instead, we'd store it in localstage at login time so that it can be retrieved from a trusted origin
The other pieces of data are not as impactful. Of course, I'd rather we keep signing if possible

@gene1wood
Copy link
Contributor Author

I'm not clear on the downsides of verifying the jwt using a traditional library fetched from a cdn

@hidde
Copy link
Contributor

hidde commented Jul 6, 2018

Downsides of verifying (which, to be clear, I agree would be more elegant):

  • Since we only insert info from the JWT into the DOM with element.textContent, there is no XSS risk, it adds plain text only. (Please correct if wrong, I’ve researched this, but you are the experts)
  • The risk that someone uses this for phishing is unrealistic, you’d have to come up with an X and Y for ‘You logged in using X, but your most secure method is Y’, ie ‘You logged in using GitHub but your most secure method is LDAP. Please call +1234667 and tell us your password..’

Downsides of using a library:

  • NLX increases in size (more than triples), caching mitigates some of that but there will still be lots of users having to wait lots longer and download lots more (for example staff who are on an overseas workweek with small internet bundle)
  • Browser support: if you ask a server to do something, you can know if it will be able to. Not so much on the front-end, there is always going to be a subset of browsers that cannot do it.

Downsides of loading via a CDN:

  • Our bundle and how all scripts and dependencies are related is in Browserify. That doesn’t mix well with external CDNs as there is no trivial way of knowing that resource has loaded when you need it. (I have researched this before when trying to add Auth0.js via its CDN)
  • More fragile: chance of CDN being blocked in content blocker and no errors at all, chance of CDN request failing because user drove into a tunnel.

My proposal, looking at time, needs and possible risks to user experience and stability, would be to either keep this simple and show client names and connection names as we receive them, unverified, or given the downsides I've identified above, we could still leave error messaging in SSO Dashboard, where we can do what we want on the server, and use libraries we want as they won’t make the user wait / spend money on their data bundle.

@gdestuynder
Copy link
Contributor

The risk that someone uses this for phishing is unrealistic,

Just to make sure this part is clear: this is incorrect if a redirect_uri is used. You can redirect a user to any website and it looks like it's been vetted by Mozilla at this point. Great phishing opportunity and very realistic. Our VA process would find this in a minute for example, which is my benchmark for "how likely would this happen".

@hidde hidde modified the milestones: Sprint Alpha, Sprint Beta Jul 10, 2018
@hidde
Copy link
Contributor

hidde commented Jul 10, 2018

Yes, absolutely. We would only insert things with element.textContent, not in element.href. To still have a redirect_uri available, it would be stored in localStorage on every visit.

Let's discuss in Tech Alignment if:

  • we want to keep error messaging in SSO Dashboard
  • we want to go ahead moving it to NLX

@hidde
Copy link
Contributor

hidde commented Jul 17, 2018

As discussed in Tech Alignment, this is too hard to do in the front-end because of large libraries and inevitable browser support issues (it will always not work for some users). Doing it in a back-end would not have these two problems, as libraries in the back-end are ‘free’ from a perf perspective and we only need support on the OS the back-end runs on, not all of our user's browsers.

We will defer working on this until we have a back-end for NLX, so that it can do our JWT verification for us and we can have verified error messages as we currently have them.

@hidde hidde removed their assignment Jul 17, 2018
@hidde hidde removed this from the Sprint Beta milestone Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants