Skip to content
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

packages/container: Do not allow anonymous login attempts #20573

Closed
wants to merge 3 commits into from
Closed

packages/container: Do not allow anonymous login attempts #20573

wants to merge 3 commits into from

Conversation

algernon
Copy link
Contributor

@algernon algernon commented Jul 31, 2022

When logging into a container registry, the client will use the /token API endpoint to verify credentials. The same end-point is used to obtain a token for anonymous pulls, too.

To be able to reject anonymous logins but still allow anonymous pulls to go through, container.Authenticate will check for the presence of a scope query parameter. This is not present for login requests, but it is for pulls and pushes.

As such, if the scope parameter is present, and ctx.Doer is nil, we fall back to generating a token using the ghost user. This continues allowing anonymous pulls. However, if the scope parameter is not present, and ctx.Doer is nil, and there are other query parameters, Gitea will return an unauthorized request error. If there are no query parameters at all, we can assume that the token requested is for a future anonymous request, and we'll let it pass through.

In practice, this means that docker login with invalid credentials will no longer succeed, but everything else should still work as it did before.

Note that this function (container.Authenticate) is only used for generating a token. Verifying that the credentials are valid for any action is decided elsewhere, this is just the token generation.

Fixes #20563.

@algernon
Copy link
Contributor Author

I have done some rudimentary testing, and with these changes:

  • docker login my-gitea -u my-user -p invalid-password fails properly (this succeeds on main)
  • docker pull my-gitea/my-user/public-image works both anonymously, and when logged in (same as on main)
  • docker push my-gitea/my-user/any-image fails without proper credentials (same as on main)
  • docker pull my-gitea/private-org/private-image fails anonymously, succeeds when logged in with proper credentials (same as on main)
  • docker push my-gitea/private-org/private-image works the same as on main

So the only thing affected that I could find is docker login, which is nice, as that's the only thing whose behaviour the PR intended to change.

Mind you, I'm not a docker wizard, and I am not familiar with their entire API, so there might be some non-obvious cases where this PR is incorrect, so please keep that in mind.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 31, 2022
@noerw noerw added this to the 1.18.0 milestone Jul 31, 2022
When logging into a container registry, the client will use the `/token` API
endpoint to verify credentials. The same end-point is used to obtain a token for
anonymous pulls, too.

To be able to reject anonymous logins but still allow anonymous pulls to go
through, `container.Authenticate` will check for the presence of a `scope` query
parameter. This is not present for login requests, but it is for pulls and
pushes.

As such, if the `scope` parameter is present, and `ctx.Doer` is nil, we fall
back to generating a token using the ghost user. This continues allowing
anonymous pulls. However, if the `scope` parameter is not present, and
`ctx.Doer` is nil, and there are other query parameters, Gitea will return an
unauthorized request error. If there are no query parameters at all, we can
assume that the token requested is for a future anonymous request, and we'll let
it pass through.

In practice, this means that `docker login` with invalid credentials will no
longer succeed, but everything else should still work as it did before.

Note that this function (`container.Authenticate`) is only used for generating a
token. Verifying that the credentials are valid for any action is decided
elsewhere, this is just the token generation.

Fixes #20563.

Signed-off-by: Gergely Nagy <me@gergo.csillger.hu>
@KN4CK3R
Copy link
Member

KN4CK3R commented Aug 1, 2022

Is the assumption true for other clients?

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
@algernon
Copy link
Contributor Author

algernon commented Aug 1, 2022

Is the assumption true for other clients?

They hold for podman too. It should hold for others, because login should not set a scope, because there are no other requests to follow (any pulls, pushes or whatevers will request their own token, appropriately scoped).

@KN4CK3R
Copy link
Member

KN4CK3R commented Aug 1, 2022

The spec only says

The scope field may be empty to request a refresh token without providing any resource permissions to the returned bearer token.

@algernon
Copy link
Contributor Author

algernon commented Aug 3, 2022

So, if I understand correctly, then docker login will request a refresh token, and use that token to obtain scoped tokens for push & stuff. That means that logins will not have a scope, and the logic in this PR is correct. The comments would need updating, though.

I'll dig a bit deeper and figure out what exactly docker/podman do with the token obtained from a login, we'll be wiser after that.

@algernon
Copy link
Contributor Author

Just an FYI: I have not had the time to dig deeper, and likely will not have the time to do that for the foreseeable future. Feel free to close this PR, or take it over, or whatever. Just don't wait for me.

@lunny lunny modified the milestones: 1.18.0, 1.19.0 Oct 19, 2022
@algernon algernon closed this by deleting the head repository Oct 27, 2022
@lunny lunny removed this from the 1.19.0 milestone Dec 13, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. topic/packages type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker login reports sucessful login with incorrect password / token
6 participants