[full-ci] Refactor proxy to get rid of the hardcoded unprotected lists #4461

C0rby · 2022-08-25T21:35:51Z

Related Issue

Fixes Refactor proxy request authentication #4401

Motivation and Context

How Has This Been Tested?

test environment:
test case 1:
test case 2:
...

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

update-docs · 2022-08-25T21:35:55Z

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.

kulmann · 2022-08-26T07:31:17Z

Gave it a spin, with following results:

uploads work

preview requests throw a http 500, with error message that reveals an internal 401:

<d:error xmlns:d="DAV" xmlns:s="http://sabredav.org/ns">
   <s:exception></s:exception>
   <s:message>  {&#34;id&#34;:&#34;com.owncloud.api.thumbnails&#34;,&#34;code&#34;:500,&#34;detail&#34;:&#34;could not get image from source: could not get the image \&#34;1284d238-aa92-42ce-bdc4-0b0000009157$06d8eaca-7a6d-4798-9b31-c24ab6b26a91/SpaceX/starship super heavy.jpeg\&#34;. Request returned with statuscode 401 &#34;,&#34;status&#34;:&#34;Internal Server Error&#34;}</s:message>
</d:error>

creating a text file failed: PUT to https://localhost:9200/remote.php/dav/spaces/1284d238-aa92-42ce-bdc4-0b0000009157%2406d8eaca-7a6d-4798-9b31-c24ab6b26a91/asdf.txt -> 401 (Unauthorized)
opening a previously uploaded text file failed with basic auth popup and 401 on GET for https://localhost:9200/remote.php/dav/spaces/1284d238-aa92-42ce-bdc4-0b0000009157%2406d8eaca-7a6d-4798-9b31-c24ab6b26a91/SpaceX/clm0008.txt
renaming files/folders works
copying filles/folders failed: COPY to https://localhost:9200/remote.php/dav/spaces/1284d238-aa92-42ce-bdc4-0b0000009157%2406d8eaca-7a6d-4798-9b31-c24ab6b26a91/SpaceX%20asdf -> 500 (Internal Server Error) -> after page reload the file files/folders are there. 🤔

I refactored the proxy so that we execute the routing before the authentication middleware. This is necessary so that we can determine which routes are considered unprotected i.e. which routes don't need authentication.

I added an unprotected flag to the proxy routes which is evaluated by the authentication middleware. This way we won't have to maintain a hardcoded list of unprotected paths and path prefixes and we will hopefully reduce the times we encounter the basic auth prompt by web browsers.

ownclouders · 2022-08-30T10:39:15Z

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

sonarcloud · 2022-08-30T16:46:28Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

65.0% Coverage
0.0% Duplication

kulmann

Retested a lot of different scenarios, all worked well. Only thing I found was on the archiver route for public links (see comment), but also broken on master, so IMO not a blocker for this PR.

kulmann · 2022-09-01T09:55:53Z

services/proxy/pkg/config/defaults/defaultconfig.go

-					Backend:  "http://localhost:9130",
+					Endpoint:    "/signin/",
+					Backend:     "http://localhost:9130",
+					Unprotected: true,
 				},
 				{
 					Endpoint: "/archiver",


tough one: when the archiver is opened from a public link it needs to be unprotected as well. Archiver from public link has ?public-token=xyz in the url query.

I'd be fine solving that in a followup ticket, as it's also currently broken in master.

Hm, I guess this should be fixable with an addtional QueryRoute. I see if I can address this in another PR.

rhafer

Looks good to me apart from a little nit picking about the comment.

rhafer · 2022-09-01T10:26:57Z

services/proxy/pkg/middleware/authentication.go

@@ -91,7 +49,8 @@ func Authentication(auths []Authenticator, opts ...Option) func(next http.Handle

 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			if isOIDCTokenAuth(r) || isUnprotectedPath(r) {
+			ri := router.ContextRoutingInfo(r.Context())
+			if isOIDCTokenAuth(r) || ri.IsRouteUnprotected() {
 				// The authentication for this request is handled by the IdP.


Nit: This comment seems to be not quite correct anymore.

rhafer · 2022-09-01T10:42:37Z

services/proxy/pkg/config/defaults/defaultconfig.go

-					Backend:  "http://localhost:9130",
+					Endpoint:    "/signin/",
+					Backend:     "http://localhost:9130",
+					Unprotected: true,
 				},
 				{
 					Endpoint: "/archiver",


Hm, I guess this should be fixable with an addtional QueryRoute. I see if I can address this in another PR.

rhafer · 2022-09-01T14:03:49Z

Turns out that this causes some unstable behavior for routes that match multiple rules 😱. E.g. we have:

                {                                 
                    Endpoint:    "/app/list",
                    Backend:     "http://localhost:9140",
                    Unprotected: true,
                },
                {
                    Endpoint: "/app/", // /app or /apps? ocdav only handles /apps
                    Backend:  "http://localhost:9140",
                },

And now requests on /app/list/ sometimes go through and sometimes fail. (Not this is not exactly a new bug in the code, it just seems that we didn't have overlapping rules of the same type before. I'll open a new issue for that and try to come up with a quick fix. (We might also temporary revert this PR).

C0rby self-assigned this Aug 25, 2022

C0rby force-pushed the refactor-proxy branch from ca51a23 to bcdca2d Compare August 25, 2022 21:37

David Christofas added 2 commits August 30, 2022 12:30

refactor proxy code

894b232

I refactored the proxy so that we execute the routing before the authentication middleware. This is necessary so that we can determine which routes are considered unprotected i.e. which routes don't need authentication.

C0rby force-pushed the refactor-proxy branch from bcdca2d to 20364fc Compare August 30, 2022 10:31

C0rby force-pushed the refactor-proxy branch 2 times, most recently from 1b2cdc3 to 46f48de Compare August 30, 2022 12:51

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

add missing unprotected flag and fix proxy test

62500fa

fix linting issues

52eb9e3

C0rby force-pushed the refactor-proxy branch from 46f48de to 52eb9e3 Compare August 30, 2022 13:16

C0rby changed the title ~~Refactor proxy to get rid of the hardcoded unprotected lists~~ [full-ci] Refactor proxy to get rid of the hardcoded unprotected lists Aug 30, 2022

fix default policy and add changelog

ca5c410

C0rby requested review from butonic, kulmann, kobergj and rhafer August 30, 2022 15:46

C0rby marked this pull request as ready for review August 30, 2022 15:47

update tests

4e697f3

C0rby force-pushed the refactor-proxy branch from bce1953 to 4e697f3 Compare August 30, 2022 16:40

kulmann approved these changes Sep 1, 2022

View reviewed changes

rhafer approved these changes Sep 1, 2022

View reviewed changes

rhafer merged commit bfb26cc into master Sep 1, 2022

delete-merged-branch bot deleted the refactor-proxy branch September 1, 2022 10:43

This was referenced Sep 1, 2022

proxy unstable routing behavior for routes that match multiple rules #4497

Closed

proxy: Avoid sorting endpoints for every request #4514

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[full-ci] Refactor proxy to get rid of the hardcoded unprotected lists #4461

[full-ci] Refactor proxy to get rid of the hardcoded unprotected lists #4461

C0rby commented Aug 25, 2022 •

edited by kulmann

Loading

update-docs bot commented Aug 25, 2022

kulmann commented Aug 26, 2022

ownclouders commented Aug 30, 2022 •

edited

Loading

sonarcloud bot commented Aug 30, 2022

kulmann left a comment

kulmann Sep 1, 2022

kulmann Sep 1, 2022

rhafer Sep 1, 2022

rhafer left a comment

rhafer Sep 1, 2022

rhafer Sep 1, 2022

rhafer commented Sep 1, 2022

[full-ci] Refactor proxy to get rid of the hardcoded unprotected lists #4461

[full-ci] Refactor proxy to get rid of the hardcoded unprotected lists #4461

Conversation

C0rby commented Aug 25, 2022 • edited by kulmann Loading

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

update-docs bot commented Aug 25, 2022

kulmann commented Aug 26, 2022

ownclouders commented Aug 30, 2022 • edited Loading

sonarcloud bot commented Aug 30, 2022

kulmann left a comment

Choose a reason for hiding this comment

kulmann Sep 1, 2022

Choose a reason for hiding this comment

kulmann Sep 1, 2022

Choose a reason for hiding this comment

rhafer Sep 1, 2022

Choose a reason for hiding this comment

rhafer left a comment

Choose a reason for hiding this comment

rhafer Sep 1, 2022

Choose a reason for hiding this comment

rhafer Sep 1, 2022

Choose a reason for hiding this comment

rhafer commented Sep 1, 2022

C0rby commented Aug 25, 2022 •

edited by kulmann

Loading

ownclouders commented Aug 30, 2022 •

edited

Loading