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

Optionally disable certificate validation for OAuth2 auth windows #3894

Merged
merged 13 commits into from Sep 1, 2021

Conversation

schrodingersket
Copy link
Contributor

@schrodingersket schrodingersket commented Aug 9, 2021

Closes #3893, Closes #2778, Closes INS-912

I fully expect to have some feedback regarding better ways to grab the validateSSL setting or maybe a different place where this event handler should be, but I figured a functional fix would be a good place to start!

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Thank you for the issue report and this PR!

Because this is specifically for auth, for security reasons I wonder if we should introduce a standalone setting (like validateOAuthSSL) rather than use the existing one. This way users have more control over disabling cert validation for regular requests but ensuring cert validation for authentication.

@wdawson / @DMarby / @johnwchadwick any opinions on using the existing flag or introducing a new one specifically for OAuth? The current flag validateSSL links to the blue box. We do have a section dedicated to security in preferences, so a custom flag like validateOAuthSSL could sit in there (red box).

image

packages/insomnia-app/app/main.development.ts Outdated Show resolved Hide resolved
packages/insomnia-app/app/main.development.ts Outdated Show resolved Hide resolved
@develohpanda develohpanda changed the title Bugfix/3893 Optionally disable certificate validation for OAuth2 auth windows Aug 9, 2021
@schrodingersket
Copy link
Contributor Author

Thanks for getting the conversation started on this! Once the Insomnia team decides which direction to take this PR, I'm happy to help where I can (:

@DMarby
Copy link
Contributor

DMarby commented Aug 10, 2021

any opinions on using the existing flag or introducing a new one specifically for OAuth? The current flag validateSSL links to the blue box. We do have a section dedicated to security in preferences, so a custom flag like validateOAuthSSL could sit in there (red box).

@develohpanda I think it might make sense for this to be a separate option.
Given that some oauth flows also make requests using our network stack however, if the option to not validate certificates for oauth is enabled, but the one for requests is not, that could lead to the window being able to open, but the request made to complete the flow failing, so if we want to make a separate option for this we might also need to make that option be taken into account for those cases?

const responsePatch = await sendWithSettings(requestId, {

const responsePatch = await network.sendWithSettings(requestId, {

const responsePatch = await sendWithSettings(requestId, {

const response = await sendWithSettings(requestId, {

@develohpanda
Copy link
Contributor

any opinions on using the existing flag or introducing a new one specifically for OAuth? The current flag validateSSL links to the blue box. We do have a section dedicated to security in preferences, so a custom flag like validateOAuthSSL could sit in there (red box).

@develohpanda I think it might make sense for this to be a separate option.
Given that some oauth flows also make requests using our network stack however, if the option to not validate certificates for oauth is enabled, but the one for requests is not, that could lead to the window being able to open, but the request made to complete the flow failing, so if we want to make a separate option for this we might also need to make that option be taken into account for those cases?

That's a great point! It looks like all those OAuth flows use network.sendWithSettings, and sendWithSettings is only used by OAuth flows

image

sendWithSettings calls into _actuallySend, which contains the following

// SSL Validation
if (settings.validateSSL) {
addTimelineText('Enable SSL validation');
} else {
setOpt(Curl.option.SSL_VERIFYHOST, 0);
setOpt(Curl.option.SSL_VERIFYPEER, 0);
addTimelineText('Disable SSL validation');
}

So it should be fairly straightfoward to tell the networking stack to use settings.validateOAuthSSL if _actuallySend is called by sendWithSettings.

Lots of ways to solve this, @schrodingersket I'll let you give it a crack, but this is the general code path! 😄

To summarize the feedback:

  • introduce a new setting named validateOAuthSSL alongside validateSSL
  • add that setting to the "Security" section under General preferences
  • move the event handler to individual OAuth 2.0 window instead of full app
  • use settings/get settings before the window opens to have a synchronous handler
  • update the networking layer to respect the validateSSl or validateOAuthSSL flag depending on the call path

@schrodingersket
Copy link
Contributor Author

Lots of ways to solve this, @schrodingersket I'll let you give it a crack, but this is the general code path!

To summarize the feedback:

  • introduce a new setting named validateOAuthSSL alongside validateSSL
  • add that setting to the "Security" section under General preferences
  • move the event handler to individual OAuth 2.0 window instead of full app
  • use settings/get settings before the window opens to have a synchronous handler
  • update the networking layer to respect the validateSSl or validateOAuthSSL flag depending on the call path

That all sounds perfectly doable. I'll take a swing at it this week 😎

Thanks for all the input everyone! Super cool to get to learn from you guys and to have the opportunity give a little back to a project I use every day 🤓

@wdawson
Copy link
Contributor

wdawson commented Aug 10, 2021

Just as a naming comment, I wonder if it would be better to not include the authorization protocol (i.e. "OAuth") in the name for the flag. In case folks are using different protocols. Maybe just validateAuthSSL instead? It would apply to OIDC as well I assume.

@develohpanda
Copy link
Contributor

Maybe just validateAuthSSL instead?

Yep! I'm good with that 👍🏽

@schrodingersket
Copy link
Contributor Author

schrodingersket commented Aug 11, 2021

@develohpanda - looks like we have a slight snag in restricting the scope of the new validateAuthSSL option to the child window created during the OAuth2 authorization flow.

The TL;DR is that the certificate-error event is thrown both in the global app object, and the child window - and that a event.preventDefault() does not seem prevent a certificate-error event from being emitted in app. Specifically, calling child.loadURL throws an actual Error instance (which is the cause of the blank screen appearing to hang - when the error is thrown, child.close() never gets called):

try {
await child.loadURL(url);
} catch (e) {
// Reject with error to show result in OAuth2 tab
reject(e);
// Need to close child window here since an exception in loadURL precludes normal call in
// _parseUrl
child.close();
}

I pushed a50d153 which provides all the other functionality described above and in which you can reproduce what I'm running into if you'd like to poke at it. Any suggestions as to how to handle this would be greatly appreciated (: Adding the certificate-error event handler to the global app object certainly works, but I definitely want to try to get that behavior scope-limited to the child window before resorting to that as an "actual" fix.

Anyway, here are some screenshots of the new option in Settings, as well as what happens when an error is encountered in the authorization window when the Fetch Tokens button is clicked in the OAuth 2 tab:

2021-08-10_22-19-11_new_security_settings

2021-08-10_23-02-25_fetch_tokens_err

@schrodingersket
Copy link
Contributor Author

schrodingersket commented Aug 12, 2021

Bingo! I found a workaround for the issue described above using the setCertificateVerifyProc API.

Should be all good to go with this now. I'll work on adding some unit tests and updating existing ones as appropriate tomorrow, but it should be all set for review at this point.

@develohpanda
Copy link
Contributor

Oh wow, that's impressive @schrodingersket 🙌🏽 👏🏽 I hadn't had a chance to debug through this yet but I'm stoked you found a way around it!

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Just an initial code review 🥳

packages/insomnia-app/app/network/network.ts Outdated Show resolved Hide resolved
packages/insomnia-app/app/network/network.ts Outdated Show resolved Hide resolved
@dimitropoulos
Copy link
Contributor

@develohpanda the video works for me now, maybe try again?

@develohpanda
Copy link
Contributor

develohpanda commented Aug 31, 2021

@dimitropoulos Looks like it was a Firefox thing 😭 works in Chrome. I've never hit this before (still marked as corrupted on Firefox), how strange!

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the video! 🙌🏽

The error message does seem a bit verbose but I don't think we should parse it to render something different; rather show the entire reason why something failed (we do the same thing when failing to fetch a GrapqhQL schema, show the raw message)

@vercel vercel bot temporarily deployed to Preview September 1, 2021 18:56 Inactive
Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

@schrodingersket I would really love to have some pointers into how you get the test environment going that you used in the video you took (and in general in this PR). I'm always on the lookout for more easily testing the OAuth2 features of the app.

The code looks good to me and the functionality in the video looks good as well.

@dimitropoulos dimitropoulos enabled auto-merge (squash) September 1, 2021 20:48
@dimitropoulos dimitropoulos merged commit ccac337 into Kong:develop Sep 1, 2021
@schrodingersket
Copy link
Contributor Author

schrodingersket commented Sep 2, 2021

@schrodingersket I would really love to have some pointers into how you get the test environment going that you used in the video you took (and in general in this PR). I'm always on the lookout for more easily testing the OAuth2 features of the app.

The code looks good to me and the functionality in the video looks good as well.

@dimitropoulos sure thing! We use Keycloak at work for IAM and OAuth2, and I've been testing against that for this PR. OAuth2, JWT, and OIDC support are pretty well-supported in my experience with it.

If you'd like, I'd be happy throw together a super quick test repo for you based on their Docker image with SSL set up for you as a reference repository for testing if you feel it'd be worthwhile to have on hand and don't feel like rummaging through all the docs yourself (:

@dimitropoulos
Copy link
Contributor

I would really really love that and the whole team will appreciate it! Thanks!!

@schrodingersket
Copy link
Contributor Author

schrodingersket commented Sep 3, 2021

@dimitropoulos @develohpanda - here you go:

https://github.com/schrodingersket/keycloak-oauth2-ssl-testbed

Everything should be configured and ready right out of the box for you there. I added a fairly comprehensive README with screenshots and such for getting everything running, but the TL;DR is (assuming you've installed Docker) that you just need to run run.sh in a terminal and import the included Insomnia export in that repo into your local instance and you should be all good to go. It's also configured as a Git Sync repo, so you can just import it from Git instead of as a File Import if you prefer.

I added the script I used to generate the certificates (via openssl) to the repository, so you can modify those certs however you need; that script generates a root certificate authority and signs the server certificates with it, since I expect it might be useful for y'all to have a convenient place to test out custom CA bundles as well (:

@develohpanda
Copy link
Contributor

Thanks a tonne @schrodingersket, this is bound to be a useful resource 🙌

@dimitropoulos
Copy link
Contributor

hey @schrodingersket just wanted to come back and say thank you (again) for the above. we used it just now to work with another issue on the insomnia stream and it worked very well! we really appreciate it!

@schrodingersket
Copy link
Contributor Author

Whoohoo! Glad you found it to be a useful resource. (:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants