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

Fixes #4895: Adds CSRF protection check to the API if a session user is ... #1331

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
4 participants
@ehelms
Member

ehelms commented Mar 26, 2014

...present.

@dLobatog

This comment has been minimized.

Show comment
Hide comment
@dLobatog

dLobatog Mar 26, 2014

Member

Good catch!

You could just put a protect_from_forgery in the base_controller, just that, instead of checking if session[:user] exists?
If you really just want to protect CSRF for POST/PUT requests, you can use :only => or :except => ..., or even protect_from_forgery unless request.method == 'GET'

We already use protect_from_forgery for all requests here

I would say the best choice would be to put it in the Foreman::Controller::Authentication module in the included part, just like we do with Foreman::Controller::UsersMixin. It would be one line of code that would work both for API and non-API requests

Member

dLobatog commented Mar 26, 2014

Good catch!

You could just put a protect_from_forgery in the base_controller, just that, instead of checking if session[:user] exists?
If you really just want to protect CSRF for POST/PUT requests, you can use :only => or :except => ..., or even protect_from_forgery unless request.method == 'GET'

We already use protect_from_forgery for all requests here

I would say the best choice would be to put it in the Foreman::Controller::Authentication module in the included part, just like we do with Foreman::Controller::UsersMixin. It would be one line of code that would work both for API and non-API requests

@ehelms

This comment has been minimized.

Show comment
Hide comment
@ehelms

ehelms Mar 27, 2014

Member

@elobato if I open this up to CSRF protection for all PUT/POST I would think that a generic API call using either basic auth or oauth would fail as the user would need to have a CSRF token and pass it along. Whereas here I am targeting someone that is logged in with a session and is using the API either from a UI component or directly via curl/postman/etc. Thoughts?

Member

ehelms commented Mar 27, 2014

@elobato if I open this up to CSRF protection for all PUT/POST I would think that a generic API call using either basic auth or oauth would fail as the user would need to have a CSRF token and pass it along. Whereas here I am targeting someone that is logged in with a session and is using the API either from a UI component or directly via curl/postman/etc. Thoughts?

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Mar 31, 2014

Contributor

@ehelms makes sense, this was one of my concerns with #4776. Arguably this applies to SSO too, but I'm not sure how we should fix it. Maybe @adelton has some thoughts or experience?

Contributor

domcleal commented Mar 31, 2014

@ehelms makes sense, this was one of my concerns with #4776. Arguably this applies to SSO too, but I'm not sure how we should fix it. Maybe @adelton has some thoughts or experience?

@adelton

This comment has been minimized.

Show comment
Hide comment
@adelton

adelton Mar 31, 2014

Member

@ehelms makes sense, this was one of my concerns with #4776. Arguably this applies to SSO too, but I'm not sure how we should fix it. Maybe @adelton has some thoughts or experience?

Not sure what the issue is exactly. Can you rephrase it?

Member

adelton commented Mar 31, 2014

@ehelms makes sense, this was one of my concerns with #4776. Arguably this applies to SSO too, but I'm not sure how we should fix it. Maybe @adelton has some thoughts or experience?

Not sure what the issue is exactly. Can you rephrase it?

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Mar 31, 2014

Contributor

Our API permits requests if the user has an active session (via a session cookie). @ehelms is adding CSRF protection to prevent an attack against a user who has a session open in Foreman, which usually uses a token hidden in the form to verify the request actually comes from a Foreman page, rather than an attack.

However as well as session auth, our API accepts SSO (e.g. via REMOTE_USER with mod_auth_kerb), so if a user was using SSO to access Foreman then they might be vulnerable to a similar sort of attack against our API. I wonder if we should provide a similar token for SSO access to the API.

Contributor

domcleal commented Mar 31, 2014

Our API permits requests if the user has an active session (via a session cookie). @ehelms is adding CSRF protection to prevent an attack against a user who has a session open in Foreman, which usually uses a token hidden in the form to verify the request actually comes from a Foreman page, rather than an attack.

However as well as session auth, our API accepts SSO (e.g. via REMOTE_USER with mod_auth_kerb), so if a user was using SSO to access Foreman then they might be vulnerable to a similar sort of attack against our API. I wonder if we should provide a similar token for SSO access to the API.

@adelton

This comment has been minimized.

Show comment
Hide comment
@adelton

adelton Mar 31, 2014

Member

SSO should mean that the user does not need to authenticate multiple times. It does not mean the user shouldn't acknowledge entering the authenticated area or being asked to confirm actions in the authenticated area.

So I see nothing wrong creating that session based on separate SSO-authentication call, and then requiring that session identification to be present.

Member

adelton commented Mar 31, 2014

SSO should mean that the user does not need to authenticate multiple times. It does not mean the user shouldn't acknowledge entering the authenticated area or being asked to confirm actions in the authenticated area.

So I see nothing wrong creating that session based on separate SSO-authentication call, and then requiring that session identification to be present.

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Mar 31, 2014

Contributor

Makes sense, thanks @adelton. I filed http://projects.theforeman.org/issues/4968 so we can collect some further thoughts in that area. Sorry for derailing your PR @ehelms.

@ehelms would you mind adding a test to test/functional/api/base_controller_subclass_test.rb to check that CSRF is active on session requests? Thanks.

Contributor

domcleal commented Mar 31, 2014

Makes sense, thanks @adelton. I filed http://projects.theforeman.org/issues/4968 so we can collect some further thoughts in that area. Sorry for derailing your PR @ehelms.

@ehelms would you mind adding a test to test/functional/api/base_controller_subclass_test.rb to check that CSRF is active on session requests? Thanks.

@ehelms

This comment has been minimized.

Show comment
Hide comment
@ehelms

ehelms Apr 3, 2014

Member

Closing in favor of - #1352

Member

ehelms commented Apr 3, 2014

Closing in favor of - #1352

@ehelms ehelms closed this Apr 3, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment