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

Remove (additional) CSRF check from public endpoints if a Bearer authentication header is present (OAuth 2) #5694

Closed
Dagefoerde opened this Issue Jul 12, 2017 · 13 comments

Comments

Projects
None yet
7 participants
@Dagefoerde

Dagefoerde commented Jul 12, 2017

OCS APIs cannot be used in a straightforward way if OAuth-authenticated clients perform requests, due to CSRF checks. If a Bearer authentication header is present, it should be sufficient to assume that no CSRF attack takes place. As discussed below in this thread, it would be great if additional CSRF checks were disabled if requests are authenticated by a bearer token.

(Edited on 2017-08-07, 2:53 pm) -- Original Text:

If you add Nextcloud as an OAuth 2 service provider to a Moodle (3.3) installation, Moodle's OAuth API queries a userinfo_endpoint in order to obtain information about the authorising Nextcloud user. This fails since such an endpoint does not exist. Although actually from the OpenID spec, such an endpoint is useful to find out who was just logged in. It is also useful to check whether an access token is still valid without actually performing an operation on files. :)

Specs for the userinfo endpoint: https://openid.net/specs/openid-connect-core-1_0.html#UserInfo (general) and https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse (response).

Elements of the response can be: https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims, of which sub is a MUST (identifier of a Nextcloud user; what do you suggest? ID or username?). Moodle currently relies on a username and an email being present in the userinfo response. I already found out that you do not necessarily know an email adress, so I reported this there: https://tracker.moodle.org/browse/MDL-59511. I would suggest that you add an email address if it is known, instead of mocking one.

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Jul 12, 2017

I'm not really a fan of adding new endpoints, especially such as implementing parts of yet another specification which in the end will lead to other people demanding other parts of the specification as well.

Would the /ocs/v2.php/cloud/user suffice here? At the moment all requests to the API require an OCS-APIRequest: true header to be set to pass the CSRF check (and in an ideal world, consumers would send that one) but we can also adjust the code to not require a CSRF check here in case an OAuth bearer token is used.

<?xml version="1.0"?>
<ocs>
 <meta>
  <status>ok</status>
  <statuscode>200</statuscode>
  <message>OK</message>
 </meta>
 <data>
  <enabled>true</enabled>
  <id>lukas</id>
  <quota>
   <free>3909530726400</free>
   <used>6657835832</used>
   <total>3916188562232</total>
   <relative>0.17</relative>
   <quota>-3</quota>
  </quota>
  <email>lukas@nextcloud.com</email>
  <phone></phone>
  <address></address>
  <website></website>
  <twitter></twitter>
  <groups>
   <element>Engineering</element>
   <element>General</element>
  </groups>
  <display-name>Lukas Reschke</display-name>
 </data>
</ocs>
@Dagefoerde

This comment has been minimized.

Dagefoerde commented Jul 12, 2017

/ocs/v2.php/cloud/user would be more than sufficient information-wise! I didn't know it and I agree that it is better to re-use it instead of adding another endpoint.
But of course it would be too simple if it worked right away: Moodle only supports JSON (https://github.com/moodle/moodle/blob/350700bf8b509f5269b0fefd34fec0d3d5393c99/lib/classes/oauth2/client.php#L246). Is it possible to change the output format to JSON by adding a GET parameter? Everything within <data></data> would need to be on top level of the returned JSON object, then mapping the fields into Moodle is very easy.

we can also adjust the code to not require a CSRF check here in case an OAuth bearer token is used.

This would be great, thanks!

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Jul 12, 2017

Is it possible to change the output format to JSON by adding a GET parameter? Everything within would need to be on top level of the returned JSON object, then mapping the fields into Moodle is very easy.

"It depends". Our OCS API automatically supports converting the responses to JSON by appending ?format=json. The data would however still be within the ocs->data blob and changing that in the OCS API isn't possible as we're just returning a response object which gets translated by the framework.

Would it be possible to add nested support for this into Moodle? I could then take a look at the CSRF check.

{
   "ocs":{
      "meta":{
         "status":"ok",
         "statuscode":200,
         "message":"OK"
      },
      "data":{
         "enabled":"true",
         "id":"lukas",
         "quota":{
            "free":3909529214976,
            "used":6657835832,
            "total":3916187050808,
            "relative":0.17,
            "quota":-3
         },
         "email":"lukas@nextcloud.com",
         "phone":"",
         "address":"",
         "website":"",
         "twitter":"",
         "groups":[
            "Engineering",
            "General"
         ],
         "display-name":"Lukas Reschke"
      }
   }
}
@Dagefoerde

This comment has been minimized.

Dagefoerde commented Jul 12, 2017

For reference, where is the response for /ocs/v2.php/cloud/user implemented?

Would it be possible to add nested support for this into Moodle?

I'll have a look into that and will report back. Thanks! :)

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Jul 12, 2017

Route registered at

['root' => '/cloud', 'name' => 'Users#getCurrentUser', 'url' => '/user', 'verb' => 'GET'],
, which calls
/**
* @NoAdminRequired
* @NoSubAdminRequired
*
* gets user info from the currently logged in user
*
* @return DataResponse
* @throws OCSException
*/
public function getCurrentUser() {
$user = $this->userSession->getUser();
if ($user) {
$data = $this->getUserData($user->getUID());
// rename "displayname" to "display-name" only for this call to keep
// the API stable.
$data['display-name'] = $data['displayname'];
unset($data['displayname']);
return new DataResponse($data);
}
throw new OCSException('', \OCP\API::RESPOND_UNAUTHORISED);
}

@Dagefoerde

This comment has been minimized.

Dagefoerde commented Jul 12, 2017

Awesome, that'll do. No further endpoint required (for me). This should be sufficient, unless you'd ever want to be an OpenID provider. Accessing nested JSON values works with - as a path delimiter (not my idea ;) ).

For future reference, this is my Moodle configuration.
nc-endpoints
nc-mappings

Apart from the CSRF check, this issue is resolved for me. Thanks!

@LukasReschke LukasReschke self-assigned this Jul 12, 2017

@Dagefoerde Dagefoerde changed the title from OAuth 2: Provide a userinfo endpoint to Remove (additional) CSRF check from public endpoints if a Bearer authentication header is present (OAuth 2) Aug 4, 2017

@LukasReschke LukasReschke added this to the Nextcloud 13 milestone Aug 11, 2017

@fishfree

This comment has been minimized.

fishfree commented Sep 24, 2017

@LukasReschke Could you please tell me how to modify codes in Nextcloud 12 to solve this problem (for integrating with Moodle) ? I can't wait for the release of 13 :-)

@pierreozoux

This comment has been minimized.

Member

pierreozoux commented Nov 19, 2017

@LukasReschke I was investigating Nextcloud as oauth2 provider, and I came to the same conclusion.
If this is fixed, then we can use the ocs endpoint to get the user details.
(And looks like other major open source project like Rocket.Chat, Discourse, and GitLab allow to configure the path, so we are close to an epic win :) )
(Yes, I'm really excited by this feature :) )

Do you know if it'll still be planned for the Nextcloud 13?
If not, I read a lot the code around oauth2 and and ocs, to try to understand what was missing, so I might be able to help if you point me to the right direction!

Thanks!

@pierreozoux

This comment has been minimized.

Member

pierreozoux commented Dec 9, 2017

@rullzer rullzer removed this from the Nextcloud 14 milestone Jan 10, 2018

@moodlebeuth

This comment has been minimized.

moodlebeuth commented Jan 11, 2018

@rullzer: Why have you removed this issue from the milestone list for the nextcloud 13 release?

Michael

@rullzer

This comment has been minimized.

Member

rullzer commented Jan 11, 2018

I removed it from 14. We first moved it to 13. So far nothing has happened in this direction and as far as I know nobody is actively working (or plans to work on this) for 14. We can always add it to the milestone again when somebody works on it.

@rullzer rullzer added this to the Nextcloud 14 milestone Jan 11, 2018

pierreozoux added a commit that referenced this issue Jan 11, 2018

Remove the need for CSRF check on ocs::getCurrentUser
Fixes #5694

I tested on my server, and worked like a charm :)

I think in term of security it is fine to open this route. What do you think?
@pierreozoux

This comment has been minimized.

Member

pierreozoux commented Jan 11, 2018

Just created a PR that works. I'm just not sure about the security implication. #7798

@moodlebeuth

This comment has been minimized.

moodlebeuth commented Jan 12, 2018

Hello,

thanks for your answer.

Ok, if you are in feature freeze of the Nextcloud 13 release, please could you aply a patch for this issue - even if you think it is not so relevant - in the next following release, maybe in 13.0.1?

I think for users, which uses both Moodle and Nextcloud, it would be relevant enough to fix this issue.

Thanks a lot & Greetings,
Michael

rullzer added a commit that referenced this issue Jan 15, 2018

Don't perform CSRF check on OCS routes with Bearer auth
Fixes #5694

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer added a commit that referenced this issue Jan 25, 2018

Don't perform CSRF check on OCS routes with Bearer auth
Fixes #5694

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer added a commit that referenced this issue Jan 29, 2018

Don't perform CSRF check on OCS routes with Bearer auth
Fixes #5694

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer added a commit that referenced this issue Jan 29, 2018

Don't perform CSRF check on OCS routes with Bearer auth
Fixes #5694

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer added a commit that referenced this issue Feb 8, 2018

Don't perform CSRF check on OCS routes with Bearer auth
Fixes #5694

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment