auth: add www-authenticate based on user agent #1350

labkode · 2020-12-01T07:53:11Z

No description provided.

update-docs · 2020-12-01T07:53:14Z

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.

refs

just a small typo correction and a comment, looks good

internal/http/interceptors/auth/auth.go

internal/http/interceptors/auth/auth_test.go

refs · 2020-12-01T11:07:23Z

👍 this works as intended

> PROPFIND /remote.php/dav/files HTTP/1.1
> Host: localhost:9200
> Accept: */*
> User-Agent: mirall
> depth: 0
>
< HTTP/1.1 401 Unauthorized
< Content-Length: 0
< Date: Tue, 01 Dec 2020 11:04:02 GMT
< Www-Authenticate: Basic realm="localhost:9200"
<
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
* Connection #0 to host localhost left intact
* Closing connection 0
-:1: parser error : Document is empty

^

~/code/reva auth-user-agent*
❯ curl -v -k -H "User-Agent: mirall" -H "depth: 0" -X PROPFIND https://localhost:9200/remote.php/dav/files | xmllint --format -

The only changes in ocis are when configuring the webdav service in the frontend:

...
"auth": map[string]interface{}{
	"credentials_by_user_agent": map[string]string{
	"mirall": "basic",
	},
},
...

refs · 2020-12-01T11:08:47Z

I don't think we need to add this to ocis at all, since we're only forwarding the unauthorized request to the backend.

It would be a matter of config for ocis, and be on the same page on adding www-authenticate headers in some services

labkode · 2020-12-02T08:20:53Z

Changes are complete now, please review.

labkode · 2020-12-02T08:21:30Z

@refs can you point me how we can configure it in OCIS and bump your Reva version once this is merged?

refs · 2020-12-02T08:41:25Z

@refs can you point me how we can configure it in OCIS and bump your Reva version once this is merged?

working on making it configurable right now, should be ready by noon, pinging you then 👍

refs

LGTM 👍

ishank011

One small change. Otherwise looks good.

ishank011 · 2020-12-02T09:06:02Z

internal/http/interceptors/auth/auth.go

 			tkn := tokenStrategy.GetToken(r)
 			if tkn == "" {
 				log.Warn().Msg("core access token not set")
+
+				credKeys := applyWWWAuthenticate(r.UserAgent(), conf.CredentialsByUserAgent, conf.CredentialChain)


Maybe rename this as userAgentCredKeys?

refs

Apparently with 4528d1 reva no longer adds Www-Authenticate for unauthenticated requests with unconfigured user agents. I'm testing through ocis proxy with:

curl -v -k -H "depth: 0" -X PROPFIND https://localhost:9200/remote.php/dav/files | xmllint --format -

and reverting such commit yields this response:

> PROPFIND /remote.php/dav/files HTTP/1.1
> Host: localhost:9200
> User-Agent: curl/7.64.1
> Accept: */*
> depth: 0
>
< HTTP/1.1 401 Unauthorized
< Content-Length: 0
< Date: Wed, 02 Dec 2020 09:43:51 GMT
< Www-Authenticate: Basic realm="localhost:9200"
< Www-Authenticate: Bearer realm="localhost:9200"

however without reverting it and providing with mirall user agent and unauthenticated request it correctly adds Www-Authenticate: basic to the response.

labkode · 2020-12-02T12:18:30Z

@refs I'm probably missed something.

refs · 2020-12-02T12:19:13Z

@refs I'm probably missed something.

glad we watched it on time

labkode · 2020-12-02T12:39:00Z

@refs I think it has to do with OCIS.

[gonzalhu@iota ocis]$ curl -i http://localhost:9140/
HTTP/1.1 401 Unauthorized
Www-Authenticate: Basic realm="localhost:9140"
Www-Authenticate: Bearer realm="localhost:9140"
Date: Wed, 02 Dec 2020 12:37:42 GMT
Content-Length: 0

This request goes directly to Reva and without configuring anything, I get all available challenges.

The only change I did to OCIS to test this is to point to this PR the Reva version:

[gonzalhu@iota ocis]$ git diff
diff --git a/ocis/go.mod b/ocis/go.mod
index 01ca7ad..db094ad 100644
--- a/ocis/go.mod
+++ b/ocis/go.mod
@@ -38,6 +38,7 @@ require (
 )

 replace (
+       github.com/cs3org/reva => github.com/labkode/reva v0.0.0-20201202075545-1b0465435bec
        github.com/gomodule/redigo => github.com/gomodule/redigo v1.8.2
        github.com/owncloud/ocis/accounts => ../accounts
        github.com/owncloud/ocis/glauth => ../glauth

refs · 2020-12-02T12:55:39Z

@labkode got the same behavior, but as soon as I configure reva to work with this PR's new config as:

"auth": map[string]interface{}{
	"credentials_by_user_agent": map[string]interface{}{
		"mirall": "basic",
	},
},

I got the following:

~/code/reva auth-user-agent
❯ curl -i http://localhost:9140/
HTTP/1.1 401 Unauthorized
Date: Wed, 02 Dec 2020 12:55:16 GMT
Content-Length: 0

Without setting credentials_by_user_agent I got the same behavior as you. Reverting 4528d1a works for me, that's why I think the changes in that commit cause this. Can you confirm?

labkode · 2020-12-02T13:09:47Z

@refs gitter :)

labkode · 2020-12-02T13:44:23Z

@refs @ishank011 I've added the missing test. If user agent is set but is not found in the map, we return the available credentials.

[gonzalhu@iota ocis]$ curl -i http://localhost:9140/ -H "User-Agent: mirall"
HTTP/1.1 401 Unauthorized
Www-Authenticate: Basic realm="localhost:9140"
Date: Wed, 02 Dec 2020 13:42:57 GMT
Content-Length: 0

[gonzalhu@iota ocis]$ curl -i http://localhost:9140/ -H "User-Agent: "
HTTP/1.1 401 Unauthorized
Www-Authenticate: Basic realm="localhost:9140"
Www-Authenticate: Bearer realm="localhost:9140"
Date: Wed, 02 Dec 2020 13:43:02 GMT
Content-Length: 0

[gonzalhu@iota ocis]$ curl -i http://localhost:9140/ -H "User-Agent: peter"
HTTP/1.1 401 Unauthorized
Www-Authenticate: Basic realm="localhost:9140"
Www-Authenticate: Bearer realm="localhost:9140"
Date: Wed, 02 Dec 2020 13:43:08 GMT
Content-Length: 0

[gonzalhu@iota ocis]$ curl -i http://localhost:9140/
HTTP/1.1 401 Unauthorized
Www-Authenticate: Basic realm="localhost:9140"
Www-Authenticate: Bearer realm="localhost:9140"
Date: Wed, 02 Dec 2020 13:43:11 GMT
Content-Length: 0

refs

with the latest changes work just fine, thanks :)

ishank011

Ah missed that case!

labkode · 2020-12-02T14:16:04Z

@refs some tests don't like me

refs · 2020-12-02T14:23:19Z

weird, you actually have no failing tests but tests that either passed or did not run:

@phil-davis is it possible that some tests didn't run? I'm checking the run logs and don't see apiWebdavUploadTUS/uploadToShare.feature:66 running:

https://cloud.drone.io/cs3org/reva/3287/13/6

ishank011 · 2020-12-02T15:25:32Z

@labkode just need to rebase. The core tests commit was updated earlier today

auth: add www-authenticate based on user agent

f846b3d

refs previously approved these changes Dec 1, 2020

View reviewed changes

internal/http/interceptors/auth/auth.go Outdated Show resolved Hide resolved

internal/http/interceptors/auth/auth.go Outdated Show resolved Hide resolved

internal/http/interceptors/auth/auth_test.go Show resolved Hide resolved

labkode added 2 commits December 2, 2020 08:33

add auth checks

4528d1a

add changelog

edeccd9

labkode dismissed refs’s stale review via edeccd9 December 2, 2020 07:33

labkode changed the title ~~[WIP] auth: add www-authenticate based on user agent~~ auth: add www-authenticate based on user agent Dec 2, 2020

Merge remote-tracking branch 'up/master' into auth-user-agent

1b04654

labkode requested review from refs, ishank011 and butonic December 2, 2020 08:20

refs approved these changes Dec 2, 2020

View reviewed changes

ishank011 previously approved these changes Dec 2, 2020

View reviewed changes

refs requested changes Dec 2, 2020

View reviewed changes

add missing test

befa4a5

labkode dismissed ishank011’s stale review via befa4a5 December 2, 2020 13:42

refs reviewed Dec 2, 2020

View reviewed changes

ishank011 approved these changes Dec 2, 2020

View reviewed changes

Merge remote-tracking branch 'up/master' into auth-user-agent

040e6d3

labkode merged commit 1fb968b into cs3org:master Dec 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auth: add www-authenticate based on user agent #1350

auth: add www-authenticate based on user agent #1350

labkode commented Dec 1, 2020

update-docs bot commented Dec 1, 2020

refs left a comment

refs commented Dec 1, 2020 •

edited

Loading

refs commented Dec 1, 2020 •

edited

Loading

labkode commented Dec 2, 2020

labkode commented Dec 2, 2020

refs commented Dec 2, 2020

refs left a comment

ishank011 left a comment

ishank011 Dec 2, 2020

refs left a comment •

edited

Loading

labkode commented Dec 2, 2020

refs commented Dec 2, 2020

labkode commented Dec 2, 2020

refs commented Dec 2, 2020 •

edited

Loading

labkode commented Dec 2, 2020

labkode commented Dec 2, 2020

refs left a comment

ishank011 left a comment

labkode commented Dec 2, 2020

refs commented Dec 2, 2020 •

edited

Loading

ishank011 commented Dec 2, 2020

auth: add www-authenticate based on user agent #1350

auth: add www-authenticate based on user agent #1350

Conversation

labkode commented Dec 1, 2020

update-docs bot commented Dec 1, 2020

refs left a comment

Choose a reason for hiding this comment

refs commented Dec 1, 2020 • edited Loading

refs commented Dec 1, 2020 • edited Loading

labkode commented Dec 2, 2020

labkode commented Dec 2, 2020

refs commented Dec 2, 2020

refs left a comment

Choose a reason for hiding this comment

ishank011 left a comment

Choose a reason for hiding this comment

ishank011 Dec 2, 2020

Choose a reason for hiding this comment

refs left a comment • edited Loading

Choose a reason for hiding this comment

labkode commented Dec 2, 2020

refs commented Dec 2, 2020

labkode commented Dec 2, 2020

refs commented Dec 2, 2020 • edited Loading

labkode commented Dec 2, 2020

labkode commented Dec 2, 2020

refs left a comment

Choose a reason for hiding this comment

ishank011 left a comment

Choose a reason for hiding this comment

labkode commented Dec 2, 2020

refs commented Dec 2, 2020 • edited Loading

ishank011 commented Dec 2, 2020

refs commented Dec 1, 2020 •

edited

Loading

refs commented Dec 1, 2020 •

edited

Loading

refs left a comment •

edited

Loading

refs commented Dec 2, 2020 •

edited

Loading

refs commented Dec 2, 2020 •

edited

Loading