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

rework long-term authentication #27455

Closed
wants to merge 5 commits into from

Conversation

earl-warren
Copy link
Contributor

  • The current architecture is inherently insecure, because you can construct the 'secret' cookie value with values that are available in the database. Thus provides zero protection when a database is dumped/leaked.
  • This patch implements a new architecture that's inspired from: Paragonie Initiative.
  • Integration testing is added to ensure the new mechanism works.
  • Removes a setting, because it's not used anymore.

Refs: https://codeberg.org/forgejo/forgejo/pulls/1562

- The current architecture is inherently insecure, because you can
construct the 'secret' cookie value with values that are available in
the database. Thus provides zero protection when a database is
dumped/leaked.
- This patch implements a new architecture that's inspired from: [Paragonie Initiative](https://paragonie.com/blog/2015/04/secure-authentication-php-with-long-term-persistence#secure-remember-me-cookies).
- Integration testing is added to ensure the new mechanism works.
- Removes a setting, because it's not used anymore.

Refs: https://codeberg.org/forgejo/forgejo/pulls/1562
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 5, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 5, 2023
Comment on lines 1 to 2
// Copyright 2023 The Gitea Authors. All rights reserved.
// Copyright 2023 The Forgejo Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2023 The Gitea Authors. All rights reserved.
// Copyright 2023 The Forgejo Authors. All rights reserved.
// Copyright 2023 The Gitea Authors. All rights reserved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it a requirement to remove copyright notices to contribute code to Gitea?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would require permission from the author(s) of the code.

Copy link
Member

Choose a reason for hiding this comment

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

This has been explained many times already to you, and you have been warned as well in the last pull request to follow contribution guidelines. If you'd like to adhere to the contribution guidelines we'd be happy to have any contribution, however as you've shown to willfully disregard them action may need to be taken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where, in the contributions guidelines, can I find the requirement to remove an existing copyright notice from a MIT licensed code?

Copy link
Member

Choose a reason for hiding this comment

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

We're not asking you to remove copyright, but instead asking for copyright assignment. If each of our contributors added to the headers of the file they touched then some files could have over 1000 listings, this is clearly untenable. We don't remove copyright, for example there is places where we used code from go and kept their headers because we took that code and adhered to the license, but as you are contributing the code that's where the difference is. If you don't have the rights to do so under DCO and contribution document then that prevents us from accepting code. This is following guidance from the Linux Foundation to ensure a codebase free of any conflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not aware Gitea required a copyright assignment. Could you please let me know where this is explained in the contributions guidelines?

Copy link
Member

Choose a reason for hiding this comment

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

For the past 7 years, Gitea has required the same policy on copyright headers; you've been told this in other PRs, and have modified your PRs to be accepted. Not adhering to it once or twice can be forgiven as an accident, however, continuing to do so, including after being warned, has to be treated as you not acting in good faith, especially in this PR for a security-related PR knowing it can't be merged as-is.
Thankfully we've been working on a PR for this as well, and we were going to send it over privately once ready, so we are able to resolve this issue via a different PR without code that is tainted by copyright issues.
As I've said before, we welcome all contributions as long as they adhere to the project standards, if you/Forgejo are refusing to do so that's unfortunate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was not aware Gitea required a copyright assignment. Could you please let me know where this is explained in the contributions guidelines?

It has been documented 7 years ago: a90b252?short_path=eca12c0#diff-eca12c0a30e25b4b46522ebf89465a03ba72a03f540796c979137931d8f92055

image

And it has been updated to use SPDX format: https://github.com/go-gitea/gitea/blame/16a766cba10dcaf53cc413cee62a952031bef5bc/CONTRIBUTING.md#L430

image

models/auth/auth_token.go Outdated Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor

I just would like to mention what I ever saw:

image

@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 5, 2023
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

blocking per comments

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 5, 2023
@earl-warren
Copy link
Contributor Author

The request that I remove the Forgejo copyright header is:

  • not a technical blocker
  • not a legal requirement
  • not a requirement of the contributing guidelines

I have always consistently respected the Gitea guidelines in all my pull requests.

@techknowlogick in your capacity as TOC member and Gitea Ltd. shareholder (sole owner of the domain and the trademark) you inform me that this is a hard requirement for the first time. You have authority to do so and I do not dispute this. However, please understand this is news to me and do not claim that I acted in bad faith or that I could have been informed of this requirement in the past.

I will keep this PR open because I believe it is beneficial to Gitea users and will improve the security of the service they are running. Should you reconsider your position and agree that the copyright headers are kept as they are (just as many others which are already in the Gitea codebase) I will happily update the PR and resolve any conflicts.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 9, 2023

The request that I remove the Forgejo copyright header is:

* not a technical blocker
* not a legal requirement
* not a requirement of the contributing guidelines

I have always consistently respected the Gitea guidelines in all my pull requests.

Please be realistic, and do not try to catch every chance to create friction.

The contributing guideline has had this since 7 years ago: a90b252?short_path=eca12c0#diff-eca12c0a30e25b4b46522ebf89465a03ba72a03f540796c979137931d8f92055

image

And it has been updated to use SPDX format: https://github.com/go-gitea/gitea/blame/16a766cba10dcaf53cc413cee62a952031bef5bc/CONTRIBUTING.md#L430

image

techknowlogick in your capacity as TOC member and Gitea Ltd. shareholder (sole owner of the domain and the trademark) you inform me that this is a hard requirement for the first time. You have authority to do so and I do not dispute this.

This is an open-source community, please do not try to attack Gitea by your "company" speech.

However, please understand this is news to me and do not claim that I acted in bad faith or that I could have been informed of this requirement in the past.

This is not the first time. I also consider that's your bad action. Have you forgotten your PR " add Upload URL to release API #26663 " ?

Some soft-fork people have created too many lies to Gitea and Gitea's contributors and really harmed Gitea.


And quote from earl-warrn's discussion:

Did I suspect a discussion would eventually happen over these repeated requests to remove Forgejo copyright headers? Sure. Did I know about an unwritten policy that has never been articulated anywhere and never enforced before? Absolutely not. And how could I? The rationale that justifies its existence does not make any sense to me and I was quite surprised when it was clarified that it is in fact a policy to be enforced. But it is the Gitea policy and I accept it as a fact. If I was to argue about it, I would do so in Gitea spaces, not in Forgejo spaces.

Calling me a liar, saying I acted in bad faith is not a behavior that I tolerate. It has no place in Forgejo spaces and goes against the code of conduct. It falls in the category of derogatory comments and personal attacks:

The definition of "lie": "an intentionally false statement" or "a statement made by somebody knowing that it is not true" (from dictionary, anyone who is interested could look it up in dictionaries)

Let's see what happens in an old PR: add Upload URL to release API #26663, #26663 let's read it line by line:

Hi earl-warren please remove this line, we have given this feedback to you previously

not the first time

and also is stated what new files should have in our contributing.md.

it shows the correct reference.

This is following advice given to us from The Linux Foundation.

it has a clear example, and that's how modern / civilized / great projects do.

Many others are able to contribute to the project following the contributing.md, and we don't have // Copyright Allspice, //Copyright Blender, //Copyright ... etc.. there are literally hundreds of orgs that contribute to this project.

this example is even clear: do not include other header, all orgs use the same standard.

The project appreciates you upstreaming some of the code, however, if you aren't able to follow the contribution documentation we can't make a special exception just for you. By continuously knowingly ignoring the contributing documentation we may need to take action.

everything has been explained and you have been warned kindly.


If a person doesn't have difficulty in reading English, I think these comments are clear enough, that's not like some of you said: it is news, not been informed, It was not resolved in any of them, and in none of them was there a statement telling that it is a "hard requirement".

Selectively ignoring the the comments and keeping acting behaviors against it but saying "not be informed", it matches the definition of "lie": "an intentionally false statement".

@go-gitea go-gitea locked as too heated and limited conversation to collaborators Oct 9, 2023
@techknowlogick
Copy link
Member

Temporarily locked the thread to prevent escalation.

…ber-me

Conflicts:
	models/migrations/migrations.go
with permission from the author of the commits, the Forgejo copyright
headers are removed. This is done under protest and for the sake of
the Gitea users who would otherwise be deprived of an important security
fix.
@lunny lunny closed this in #27606 Oct 14, 2023
lunny pushed a commit that referenced this pull request Oct 14, 2023
Closes #27455

> The mechanism responsible for long-term authentication (the 'remember
me' cookie) uses a weak construction technique. It will hash the user's
hashed password and the rands value; it will then call the secure cookie
code, which will encrypt the user's name with the computed hash. If one
were able to dump the database, they could extract those two values to
rebuild that cookie and impersonate a user. That vulnerability exists
from the date the dump was obtained until a user changed their password.
> 
> To fix this security issue, the cookie could be created and verified
using a different technique such as the one explained at
https://paragonie.com/blog/2015/04/secure-authentication-php-with-long-term-persistence#secure-remember-me-cookies.

The PR removes the now obsolete setting `COOKIE_USERNAME`.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged modifies/migrations size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants