Refactor token authentication #103
Conversation
|
|
|
WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue. |
mdellweg
force-pushed
the
refactor_push_api
branch
2 times, most recently
from
c8c0f64
to
768af4a
Compare
This reverts commit 946eacf. [noissue]
mdellweg
force-pushed
the
refactor_push_api
branch
6 times, most recently
from
22f245d
to
4249e45
Compare
pulp_container/app/models.py
Outdated
| if not digest == self._get_digest(salt, url): | ||
| raise PermissionError("Access not authenticated") | ||
|
|
||
| def sign_url(self, url, salt=None): |
There was a problem hiding this comment.
I think the name of this method is a bit misleading, isn't it? The hash and salt is appended to the existing URL.
requirements.txt
Outdated
| @@ -2,3 +2,4 @@ pulpcore>=3.4 | |||
| ecdsa~=0.13.2 | |||
| pyjwkest~=1.4.0 | |||
| pyjwt[crypto]~=1.7.1 | |||
| url-normalize | |||
There was a problem hiding this comment.
Maybe we should pin this dependency too.
pulp_container/app/models.py
Outdated
| """ | ||
| if self.content_guard: | ||
| url = self.content_guard.cast().sign_url(url) | ||
| return HttpResponseRedirect(url) |
There was a problem hiding this comment.
Use redirect() instead of HttpResponseRedirect. It does the same thing and it looks a bit cleaner, see https://docs.djangoproject.com/en/3.0/topics/http/shortcuts/#redirect.
mdellweg
force-pushed
the
refactor_push_api
branch
4 times, most recently
from
60a0653
to
c2b76f0
Compare
|
I added the possibility to disable it again. |
mdellweg
force-pushed
the
refactor_push_api
branch
from
c2b76f0
to
79ffbef
Compare
pulp_container/app/models.py
Outdated
| @@ -375,3 +386,54 @@ def append_chunk(self, chunk, chunk_size=None, save=True): | |||
| self.file.close() # Flush | |||
| for algorithm in Artifact.DIGEST_FIELDS: | |||
| setattr(self, algorithm, hashers[algorithm].hexdigest()) | |||
|
|
|||
|
|
|||
| def _gen_shared_secret(): | |||
There was a problem hiding this comment.
can this be moved under content guard class? it does not appear to be used anywhere else
There was a problem hiding this comment.
It can. However the field using it as default generator must be defined afterwards.
There was a problem hiding this comment.
Turns out djangos migration mechanism really does not like it. Sorry, i'm moving it out again.
| } | ||
| return headers | ||
| try: | ||
| authorization_header = request.headers["Authorization"] |
There was a problem hiding this comment.
this code changes the behaviour where we return 401 even when hitting root endpoint (/v2 )
Some registries return 401 and others 200
https://registry-1.docker.io/v2/
https://quay.io/v2/
I am ok with both of approaches however I think that returning 401 and then asking for token auth, generating the token and then verifying it in the nuance is a surplus work because the only thing that the client cares is 1) presence of registry 2.0 header and 4) whatever code is fine except 404 because what he looks at is the header to identify whether this is a v2 api registry otherwise he needs to fall back to the v1 api.
Thoughts?
There was a problem hiding this comment.
I see both of the above examples returning 401.
Also, with the current implementation, i need to raise the AuthenticationFailed exception to spit out the Www-Authenticate header.
There was a problem hiding this comment.
ha, you are right, quay does return 401. I did through the browser so the "true" response kinda implied to me that it was a 200.
I have tested and it does work. 🎈 |
mdellweg
force-pushed
the
refactor_push_api
branch
from
79ffbef
to
3dd1e86
Compare
mdellweg
force-pushed
the
refactor_push_api
branch
2 times, most recently
from
a0dccb6
to
7e446a0
Compare
mdellweg
force-pushed
the
refactor_push_api
branch
from
7e446a0
to
02564d7
Compare
|
@ipanova want to rereview? |
|
@mdellweg looks good to me, thank you! Feel free to merge :) |
The token server runs on the port 24817 by default. In the docs, we provide the example of the settings file with the port number 24816. This may confuse users in the future.
```
(pulp) [vagrant@pulp3-source-fedora31 backup]$ http :24817/v2/
HTTP/1.1 401 Unauthorized
Allow: GET, HEAD, OPTIONS
Connection: close
Content-Length: 58
Content-Type: application/json
Date: Mon, 13 Jul 2020 08:39:20 GMT
Docker-Distribution-Api-Version: registry/2.0
Server: gunicorn/20.0.4
Vary: Accept
WWW-Authenticate: Bearer realm="http://localhost:24816/token",service="pulp3-source-fedora31.localhost.example.com"
X-Frame-Options: SAMEORIGIN
{
"detail": "Authentication credentials were not provided."
}
```
In addition to that, the note stating that the push API is unauthenticated was removed because such a support wass added in pulp#105 and pulp#103.
[noissue]
The token server runs on the port 24817 by default. In the docs, we provide the example of the settings file with the port number 24816. This may confuse users in the future.
```
(pulp) [vagrant@pulp3-source-fedora31 backup]$ http :24817/v2/
HTTP/1.1 401 Unauthorized
Allow: GET, HEAD, OPTIONS
Connection: close
Content-Length: 58
Content-Type: application/json
Date: Mon, 13 Jul 2020 08:39:20 GMT
Docker-Distribution-Api-Version: registry/2.0
Server: gunicorn/20.0.4
Vary: Accept
WWW-Authenticate: Bearer realm="http://localhost:24816/token",service="pulp3-source-fedora31.localhost.example.com"
X-Frame-Options: SAMEORIGIN
{
"detail": "Authentication credentials were not provided."
}
```
In addition to that, the note stating that the push API is unauthenticated was removed because of pulp#105 and pulp#103.
[noissue]
No description provided.