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

Remove ReverseProxy authentication from the API (#22219) #22252

Merged
merged 2 commits into from
Dec 30, 2022

Conversation

lunny
Copy link
Member

@lunny lunny commented Dec 27, 2022

backport #22219

Since we changed the /api/v1/ routes to disallow session authentication we also removed their reliance on CSRF. However, we left the ReverseProxy authentication here - but this means that POSTs to the API are no longer protected by CSRF.

Now, ReverseProxy authentication is a kind of session authentication, and is therefore inconsistent with the removal of session from the API.

This PR proposes that we simply remove the ReverseProxy authentication from the API and therefore users of the API must explicitly use tokens or basic authentication.

Replace #22077
Close #22221
Close #22077

Signed-off-by: Andrew Thornton art27@cantab.net

Since we changed the /api/v1/ routes to disallow session authentication
we also removed their reliance on CSRF. However, we left the
ReverseProxy authentication here - but this means that POSTs to the API
are no longer protected by CSRF.

Now, ReverseProxy authentication is a kind of session authentication,
and is therefore inconsistent with the removal of session from the API.

This PR proposes that we simply remove the ReverseProxy authentication
from the API and therefore users of the API must explicitly use tokens
or basic authentication.

Replace go-gitea#22077
Close go-gitea#22221 
Close go-gitea#22077 

Signed-off-by: Andrew Thornton <art27@cantab.net>
@lunny lunny added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Dec 27, 2022
@lunny lunny added this to the 1.17.5 milestone Dec 27, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Dec 27, 2022
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really backport that to 1.17?
I think this is too breaking to merge into 1.17.
Even if the session removal might have been already done in 1.17 (which I'm unsure of), I don't think it is a good idea to backport a breaking change into a minor version increase.
With 1.18, we have the benefit that 1.18.0 hasn't been released yet, so it at least should not really affect many people, and people rather expect breaking changes between releases.
But as we want to be semver compatible, I strongly discourage backporting a breaking change for a released major version

@lunny
Copy link
Member Author

lunny commented Dec 28, 2022

#22219

No plan to back port to 1.17.
But that was considered as a security problem.

@KN4CK3R
Copy link
Member

KN4CK3R commented Dec 28, 2022

I approved it as the security fix but it may be ok to only backport it to 1.18 because 1.17 should be obsolete in the next days.

Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to @KN4CK3R I approve as a security fix

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 29, 2022
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because of securityfix

Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, apparently the common opinion is to backport it.
Blocking it further is useless then…

@lafriks lafriks merged commit 8cd6be1 into go-gitea:release/v1.17 Dec 30, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
@lunny lunny deleted the lunny/replace_22077_2 branch August 24, 2023 11:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants