[full-ci] Rewrite of the authentication middleware #4374

C0rby · 2022-08-10T12:40:57Z

@xoxys found out that it was possible to send unauthenticated requests to the settings service.
As it turned out it was generally possible to send unauthenticated requests to any service. The reason for this that we had multiple authentication middlewares which were called sequentially. When the current authenticator wasn't able to authenticate the request it would just pass it onto the next http handler in case any following middleware could authenticate it. In the end when no authenticator was able to authenticate the request the request still continued to the target service and was handled.

Here is a simplified diagram of the old behavior

  graph TD;
      OTHER_MIDDLEWARE-->OIDC;
      OIDC-->BASIC;
      BASIC-->SIGNED_URL;
      SIGNED_URL-->PUBLIC_SHARE;
      PUBLIC_SHARE-->TARGET;

Now instead of having multiple authentication middlewares sequentially, we have one authentication middleware which can invoke multiple authenticators

flowchart TD;
    OTHER_MIDDLEWARE-->AUTHENTICATION;
    subgraph AUTHENTICATION
    OIDC
    BASIC
    SIGNED_URL
    PUBLIC_SHARE
    end
   AUTHENTICATION-->A{Authenticated?}
   A{Authenticated?}--YES-->Target
   A{Authenticated?}--NO-->401_Response

While this is already an improvement, this also introduced new problems. The requests for the static resources of the web client aren't authenticated, which means they would be blocked by the authentication middleware. To work around this I had to introduce a static list of paths and path prefixes for "unprotected" paths i.e. paths which don't require authentication.

Maintaining that list will probably be a large pain so to make that cleaner and future proof we should consider something like adding an "unprotected" flag to the proxy routes. Then we could resolve the routes before the authentication middlewares are invoked so we have that extra information while handling the authentication of the request.

I left this out of this PR so that it won't get too big and I didn't want to introduce too many changes to a critical part like authentication.

update-docs · 2022-08-10T12:41:01Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

ownclouders · 2022-08-10T12:48:33Z

💥 Acceptance test Core-API-Tests-ocis-storage-1 failed. Further test are cancelled...

kobergj

Some questions/remarks

services/proxy/pkg/middleware/authentication.go

services/proxy/pkg/middleware/oidc_auth.go

services/proxy/pkg/middleware/basic_auth.go

services/proxy/pkg/middleware/oidc_auth.go

services/proxy/pkg/middleware/basic_auth.go

wkloucek

I did a quick testing with following deployment examples:

oCIS with WOPI / Collabora / OnlyOffice
- opening a document does not work because a 401 on this url: https://ocis.owncloud.test/external/spaces/1284d238-aa92-42ce-bdc4-0b0000009157$7f1c5702-7d39-474c-a882-0235cf282daa/OfficeDocument%20Test%20files/file-sample_1MB.docx?app=Collabora&fileId=1284d238-aa92-42ce-bdc4-0b0000009157%247f1c5702-7d39-474c-a882-

Parallel deployment

logging in as Marie does not work, because she is configured to use oC10 which results in a 401 on this url: https://cloud.owncloud.test/apps/openidconnect/redirect (see also proxy config

ocis/deployments/examples/oc10_ocis_parallel/config/ocis/proxy.yaml

Lines 60 to 65 in cf8533c

    
           - name: oc10 
        
             routes: 
        
               - endpoint: "/" 
        
                 backend: http://oc10:8080 
        
               - endpoint: "/data" 
        
                 backend: http://localhost:9140

)

C0rby · 2022-08-11T11:24:04Z

I did a quick testing with following deployment examples:

* oCIS with WOPI / Collabora / OnlyOffice
  
  * opening a document does not work because a 401 on this url: https://ocis.owncloud.test/external/spaces/1284d238-aa92-42ce-bdc4-0b0000009157$7f1c5702-7d39-474c-a882-0235cf282daa/OfficeDocument%20Test%20files/file-sample_1MB.docx?app=Collabora&fileId=1284d238-aa92-42ce-bdc4-0b0000009157%247f1c5702-7d39-474c-a882-

* Parallel deployment
  
  * logging in as Marie does not work, because she is configured to use oC10 which results in a 401 on this url: https://cloud.owncloud.test/apps/openidconnect/redirect (see also proxy config https://github.com/owncloud/ocis/blob/cf8533cbb90dd8128ec614db8f36103f9453bbc5/deployments/examples/oc10_ocis_parallel/config/ocis/proxy.yaml#L60-L65
    )

Yeah, I think the code still needs some more changes.
I noticed that many things just happened implicitly because the requests still got routed to e.g. reva and there is more authentication logic...

The old approach of the authentication middlewares had the problem that when an authenticator could not authenticate a request it would still send it to the next handler, in case that the next one can authenticate it. But if no authenticator could successfully authenticate the request, it would still be handled, which leads to unauthorized access.

I admit it would be better to implement the tests but I tried and it is a bit tricky since we can't mock everything we would need to mock. I'll wan to get these changes in first and later in the near future we should revisit the auth middleware architecture and refactor it a bit more to be more testable and future proof.

C0rby · 2022-08-16T13:46:26Z

I tested the example parallel deployment and the example wopi deployment and they worked for me. 👍

wkloucek · 2022-08-17T06:19:32Z

I tested the example parallel deployment and the example wopi deployment and they worked for me. +1

I can confirm this in the case of the wopi example 👍
But the parallel deployment is still not working on my side. Marie still gets a 401 on https://cloud.owncloud.test/apps/files/ 🤔

C0rby · 2022-08-17T06:40:26Z

Ok, let me check again.

C0rby · 2022-08-17T07:02:43Z

You're right. I must've forgotten to replace the docker tag or something.
But it seems like to properly support the parallel deployment we would need to refactor the authentication middleware some more like I described it in the initial comment.

kobergj

One critical question. Rest is optional

services/proxy/pkg/middleware/authentication.go

services/proxy/pkg/middleware/public_share_auth.go

kobergj · 2022-08-18T11:44:03Z

services/proxy/pkg/middleware/signed_url_auth.go

 	}

 	return res.Records[0].Value, nil
 }
+
+func (m SignedURLAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) {


Still no comment, also for other Authenticate funcs

butonic · 2022-08-18T13:56:29Z

in reva http services can maintain a list of unprotected endpoints, eg. for ocdav

Since our 'proxy' is actually an API gateway that authenticates requests, I wonder how go micro marks routes as unprotected 🤔

C0rby · 2022-08-22T12:46:32Z

Can someone mark the "vulnerability" in Sonarcloud as false positive? It's just complaining about a string constant which contains "credential" in the name.

I can't do that, probably because the PR is from me.

outdated review

sonarqubecloud · 2022-08-22T14:01:41Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

15.6% Coverage
0.0% Duplication

Merge: 976a91c dfe7032 Author: David Christofas <[email protected]> Date: Mon Aug 22 16:39:03 2022 +0200 Merge pull request #4374 from owncloud/rewrite-auth-middleware [full-ci] Rewrite of the authentication middleware

C0rby requested review from rhafer, butonic, kobergj and wkloucek August 10, 2022 12:40

C0rby self-assigned this Aug 10, 2022

kobergj requested changes Aug 10, 2022

View reviewed changes

wkloucek previously requested changes Aug 11, 2022

View reviewed changes

C0rby changed the title ~~Rewrite of the authentication middleware~~ [full-ci] Rewrite of the authentication middleware Aug 11, 2022

C0rby force-pushed the rewrite-auth-middleware branch from 558fe42 to ace9bca Compare August 11, 2022 10:56

C0rby marked this pull request as draft August 11, 2022 11:14

David Christofas added 12 commits August 12, 2022 10:47

clean up the authenticators middlewares

f35c8b9

refactor unprotected paths check

ddfc01b

update authentication tests

ef02092

add tests for the basic auth middleware

7bc1305

add tests for the public share auth middleware

17a4e65

some more cleaning up

06ffd9c

fix some authentication cases

d271ae2

add missing www-authentication header on failed authentication

32f68f9

add missing unprotected paths

036c466

fix logic of when to add the www-authenticate headers

5d45f0e

C0rby force-pushed the rewrite-auth-middleware branch from ace9bca to 5d45f0e Compare August 12, 2022 08:48

David Christofas added 2 commits August 15, 2022 11:38

add missing unprotected path

3ebfcbf

add missing unprotected path

864438b

C0rby mentioned this pull request Aug 16, 2022

Connection closed while TUS makes the client stuck #4395

Closed

2 tasks

add missing comments

78d85b6

David Christofas added 2 commits August 16, 2022 15:16

add unprotected path prefix for external apps

905ead6

add unprotected path prefix for parallel deployment

3f25ca2

C0rby marked this pull request as ready for review August 16, 2022 13:45

C0rby requested review from wkloucek and kobergj August 16, 2022 13:45

add missing comments and changelog

b5ef10d

C0rby mentioned this pull request Aug 17, 2022

Refactor proxy request authentication #4401

Closed

kobergj requested changes Aug 18, 2022

View reviewed changes

add missing comments

12d42e0

C0rby force-pushed the rewrite-auth-middleware branch from 2bbdfaa to 12d42e0 Compare August 22, 2022 12:24

kobergj approved these changes Aug 22, 2022

View reviewed changes

replace strings.Title with cases.Title

dfe7032

C0rby force-pushed the rewrite-auth-middleware branch from 4b4be08 to dfe7032 Compare August 22, 2022 13:26

C0rby merged commit c4881f5 into master Aug 22, 2022

delete-merged-branch bot deleted the rewrite-auth-middleware branch August 22, 2022 14:39

wkloucek mentioned this pull request Sep 16, 2022

fix the oidc provider cache #4600

Merged

9 tasks

dragonchaser mentioned this pull request Nov 29, 2023

Listing of Trashbin Items Slow #7844

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[full-ci] Rewrite of the authentication middleware #4374

[full-ci] Rewrite of the authentication middleware #4374

C0rby commented Aug 10, 2022

update-docs bot commented Aug 10, 2022

ownclouders commented Aug 10, 2022 •

edited

Loading

kobergj left a comment

wkloucek left a comment

C0rby commented Aug 11, 2022

C0rby commented Aug 16, 2022

wkloucek commented Aug 17, 2022

C0rby commented Aug 17, 2022

C0rby commented Aug 17, 2022

kobergj left a comment

kobergj Aug 18, 2022

butonic commented Aug 18, 2022

C0rby commented Aug 22, 2022

sonarqubecloud bot commented Aug 22, 2022

	- name: oc10
	routes:
	- endpoint: "/"
	backend: http://oc10:8080
	- endpoint: "/data"
	backend: http://localhost:9140

[full-ci] Rewrite of the authentication middleware #4374

[full-ci] Rewrite of the authentication middleware #4374

Conversation

C0rby commented Aug 10, 2022

update-docs bot commented Aug 10, 2022

ownclouders commented Aug 10, 2022 • edited Loading

kobergj left a comment

Choose a reason for hiding this comment

wkloucek left a comment

Choose a reason for hiding this comment

C0rby commented Aug 11, 2022

C0rby commented Aug 16, 2022

wkloucek commented Aug 17, 2022

C0rby commented Aug 17, 2022

C0rby commented Aug 17, 2022

kobergj left a comment

Choose a reason for hiding this comment

kobergj Aug 18, 2022

Choose a reason for hiding this comment

butonic commented Aug 18, 2022

C0rby commented Aug 22, 2022

sonarqubecloud bot commented Aug 22, 2022

ownclouders commented Aug 10, 2022 •

edited

Loading