Allow authentication against the _vmdb_session cookie for UI only #543

skateman · 2019-01-15T17:42:33Z

We're consuming the API from our UI with token authentication, which is not the best because we have to poll the API to renew the token. This made the complexity of the login code unnecessarily high and also the token renewal floods the log with useless information every X seconds.

The API and the UI workers are both using the same memcached server to store short-living information, such as sessions or tokens. As the two workers are basically the same application, they implicitly have the same session cookie and so through memcached, sharing the access to the same session. However, the API was not using this session, for now.

I'm proposing a new authentication mechanism for the UI only (replaces #542) that will test if the CSRF header is set on the API request. This header will be used from the UI only and will also prevent from opening a HTTP basic auth dialog even if the session times out. This will make #542 completely obsolete.

I've discussed this with @Fryguy already and honestly, due to the simplicity of this feature I am afraid that this is too good to be true. Anyway, we will use this PR with @himdel to work further on the UI part and make changes if something goes wrong. The behavior of the API should not be affected by this change, however, we will be able to drop a few hunderd lines of JS from the UI.

When using external authentication, the apache would force basic auth on every unauthorized API request, but there's a fix for that here: ManageIQ/manageiq-appliance#224

himdel · 2019-01-16T12:25:31Z

This header will be used from the UI only and will prevent from opening a HTTP basic auth dialog if the session times out.

Looks like this is still missing the header code.

Without it, when the cookie expires, the request will be done without a cookie, auth_mechanism will return nil, and we will end up in when :basic, nil: request_http_basic_authentication...

skateman · 2019-01-16T12:28:52Z

@himdel you're right, we're also missing the CSRF token validation ... the question is how we want to have them. I assume that GET requests to the API should be also verified against the CSRF token, but maybe i'm off the rails with this (pun intended). ping @martinpovolny

skateman · 2019-01-16T19:23:21Z

@martinpovolny I've added the CSRF token verification, it was way simpler than I thought. I assume we also want to verify the origin, but please let me know if not. cc @himdel

miq-bot · 2019-01-17T14:13:23Z

This pull request is not mergeable. Please rebase and repush.

jvlcek · 2019-01-17T16:36:01Z

I worked with @skateman to test these changes. The testing done was with SAML via Keycloak, External Auth and SSO (kerberos) via IPA. I believe this is a good test sampling. LGTM JoeV

skateman · 2019-01-17T17:33:26Z

@miq-bot rm_label unmergeable
@miq-bot add_reviewer @abellotti
@miq-bot add_label hammer/no

app/controllers/api/base_controller/authentication.rb

spec/requests/authentication_spec.rb

Fryguy · 2019-01-17T19:37:40Z

The behavior of the API should not be affected by this change, however, we will be able to drop a few hunderd lines of JS from the UI.

wow 😮

Fryguy · 2019-01-17T19:40:19Z

app/controllers/api/base_controller/authentication.rb

+          # only the CSRF token is being sent and we want the API to know that the request was
+          # initiated from the UI. Otherwise the response to a such request would force the
+          # browser to throw an undesired HTTP basic authentication dialog.
+          :cookie_ui


What I'm reading in your comment is that the CSRF token will always exist, so why have the check for the session cookie here at all. That is, it seems this code can just be

... elsif request.x_csrf_token :cookie_ui else ...

So, that being said, where do we use the SESSION_COOKIE at all after this point? Even in your tests, I don't see you setting the cookie at all.

@Fryguy in my tests the session cookie is sent automatically, that's AFAIK default rack behavior. You'll have a session cookie in Rails after your first request to the site, even if you're not logged in. But you're right, I'll simplify this part...

Fryguy · 2019-01-17T19:42:51Z

app/controllers/api/base_controller/authentication.rb

-        when :basic, :basic_ui, nil
+        when :cookie_ui
+          raise AuthenticationError unless valid_ui_session?
+          auth_user(session[:userid])


I'm assuming the cookie check is in here?

It actually happens earlier in the valid_ui_session? when validating the CSRF token as it is also part of the session.

Fryguy · 2019-01-17T19:43:37Z

I have a few questions but overall I'm ok with this approach. I would like another set of eyes though... @jrafanie ?

jrafanie · 2019-01-17T20:01:33Z

I have a few questions but overall I'm ok with this approach. I would like another set of eyes though... @jrafanie ?

Yeah, I looked at it. @skateman and I chatted and I neglected to comment here. I'm good with the changes although I'm not sure what to do about the verifying of the origin.

miq-bot · 2019-01-17T20:03:50Z

Some comments on commit skateman@c8cfce3

spec/requests/authentication_spec.rb

⚠️ - 220 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 233 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 240 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2019-01-17T20:03:55Z

Checked commit skateman@c8cfce3 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

spec/requests/authentication_spec.rb

⚠️ - Line 215, Col 9 - Lint/UnderscorePrefixedVariableName - Do not use prefix _ for a variable that is used.

skateman · 2019-01-17T20:05:37Z

@jrafanie I've added the origin verification, it doesn't hurt as it allows nil origin as well which is the standard Rails behavior. The browser sends the Origin header with POST requests only, so those will get validated (tested - works). We can append the header manually to other type of requests in the UI code, but that's a decision to make in the UI PR.

jrafanie

👍

martinpovolny · 2019-01-18T08:51:27Z

As discussed f2f: when talking with the API from a browser there should be 2 secrets:

one in possession of the browser, but out of controll of the Javascript -- that is the (http-only) cookie.
second in possession of the Javascript code so that it does not get automatically added by the browser (unless requested by javascript code) -- that is some token.

The 1st is vulnerable to XSRF but relatively safe from XSS, the 2nd is vulnerable to XSS but relatively safe from XSRF.

Then together with CSP we should be safe. (We still have to work on making the CSP policy stricter.)

I have to confess that I have not read this PR in details but I believe that your solution is implemented as we discussed.

👍

skateman · 2019-01-18T09:00:40Z

@martinpovolny yes it is, now it hangs on the UI side here: ManageIQ/manageiq-ui-classic#5164

miq-bot added the wip label Jan 15, 2019

skateman mentioned this pull request Jan 15, 2019

Use cookie authentication against the API, drop the token renewal ManageIQ/manageiq-ui-classic#5164

Merged

skateman force-pushed the cookie-auth branch 3 times, most recently from 72377e0 to 4b3da56 Compare January 16, 2019 19:20

skateman mentioned this pull request Jan 17, 2019

Let API requests go through apache if they contain a CSRF token ManageIQ/manageiq-appliance#224

Merged

miq-bot added the unmergeable label Jan 17, 2019

skateman force-pushed the cookie-auth branch from 4b3da56 to b112980 Compare January 17, 2019 17:23

skateman changed the title ~~[WIP] Allow authentication against the _vmdb_session cookie for UI only~~ Allow authentication against the _vmdb_session cookie for UI only Jan 17, 2019

miq-bot removed wip unmergeable labels Jan 17, 2019

miq-bot requested a review from abellotti January 17, 2019 17:34

miq-bot added the hammer/no label Jan 17, 2019

Fryguy reviewed Jan 17, 2019

View reviewed changes

app/controllers/api/base_controller/authentication.rb Show resolved Hide resolved

Fryguy reviewed Jan 17, 2019

View reviewed changes

spec/requests/authentication_spec.rb Show resolved Hide resolved

skateman force-pushed the cookie-auth branch from b112980 to 3fb30f7 Compare January 17, 2019 19:14

Fryguy reviewed Jan 17, 2019

View reviewed changes

Allow authentication against the _vmdb_session cookie for UI only

c8cfce3

skateman force-pushed the cookie-auth branch from 3fb30f7 to c8cfce3 Compare January 17, 2019 19:59

Fryguy approved these changes Jan 17, 2019

View reviewed changes

jrafanie approved these changes Jan 17, 2019

View reviewed changes

Fryguy merged commit b903640 into ManageIQ:master Jan 17, 2019

Fryguy added this to the Sprint 103 Ending Jan 21, 2019 milestone Jan 17, 2019

Fryguy added the enhancement label Jan 17, 2019

skateman deleted the cookie-auth branch January 17, 2019 20:44

jvlcek mentioned this pull request May 5, 2020

[RFE] Support OpenID-Connect/OAuth2 ManageIQ/manageiq#19867

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow authentication against the _vmdb_session cookie for UI only #543

Allow authentication against the _vmdb_session cookie for UI only #543

skateman commented Jan 15, 2019 •

edited

Loading

himdel commented Jan 16, 2019

skateman commented Jan 16, 2019

skateman commented Jan 16, 2019

miq-bot commented Jan 17, 2019

jvlcek commented Jan 17, 2019

skateman commented Jan 17, 2019 •

edited

Loading

Fryguy commented Jan 17, 2019

Fryguy Jan 17, 2019

Fryguy Jan 17, 2019 •

edited

Loading

skateman Jan 17, 2019

Fryguy Jan 17, 2019

skateman Jan 17, 2019

Fryguy commented Jan 17, 2019

jrafanie commented Jan 17, 2019

miq-bot commented Jan 17, 2019

miq-bot commented Jan 17, 2019

skateman commented Jan 17, 2019

jrafanie left a comment

martinpovolny commented Jan 18, 2019 •

edited

Loading

skateman commented Jan 18, 2019

Allow authentication against the _vmdb_session cookie for UI only #543

Allow authentication against the _vmdb_session cookie for UI only #543

Conversation

skateman commented Jan 15, 2019 • edited Loading

himdel commented Jan 16, 2019

skateman commented Jan 16, 2019

skateman commented Jan 16, 2019

miq-bot commented Jan 17, 2019

jvlcek commented Jan 17, 2019

skateman commented Jan 17, 2019 • edited Loading

Fryguy commented Jan 17, 2019

Fryguy Jan 17, 2019

Choose a reason for hiding this comment

Fryguy Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

skateman Jan 17, 2019

Choose a reason for hiding this comment

Fryguy Jan 17, 2019

Choose a reason for hiding this comment

skateman Jan 17, 2019

Choose a reason for hiding this comment

Fryguy commented Jan 17, 2019

jrafanie commented Jan 17, 2019

miq-bot commented Jan 17, 2019

miq-bot commented Jan 17, 2019

skateman commented Jan 17, 2019

jrafanie left a comment

Choose a reason for hiding this comment

martinpovolny commented Jan 18, 2019 • edited Loading

skateman commented Jan 18, 2019

skateman commented Jan 15, 2019 •

edited

Loading

skateman commented Jan 17, 2019 •

edited

Loading

Fryguy Jan 17, 2019 •

edited

Loading

martinpovolny commented Jan 18, 2019 •

edited

Loading