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

gitea: 1.21.3 -> 1.21.4 #281543

Merged
merged 1 commit into from
Jan 20, 2024
Merged

gitea: 1.21.3 -> 1.21.4 #281543

merged 1 commit into from
Jan 20, 2024

Conversation

SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Jan 17, 2024

Description of changes

https://github.com/go-gitea/gitea/releases/tag/v1.21.4

Deployed this to my instance without problems. This also fixed the repository search crashing. I used an overlay to fix this temporarily in my config.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@SuperSandro2000 SuperSandro2000 added the 1.severity: security Issues which raise a security issue, or PRs that fix one label Jan 17, 2024
@fpletz
Copy link
Member

fpletz commented Jan 17, 2024

Do we need a backport to 23.11 due to the security issues?

@SuperSandro2000
Copy link
Member Author

The corresponding PR that fixed it is go-gitea/gitea#28765 and the bug got introduced in go-gitea/gitea@18de83b#diff-a13c89f8dad46c17e7d8e0200b1716954e4bb6436bf447ce159be7297992368bR1023 which is part of 1.20. So we would need a minor version bump for stable as there is no update to 1.20. Also there are other security fixes which likely didn't get backported, either.

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

LGTM.

Do we need a backport to 23.11 due to the security issues?

So, that's a good question and I did some research: I'm not even sure if the "Require token for GET subscription endpoint" change is actually a vulnerability: if no token is provided, an error 500 is currently given, no information seems to be leaked.

The circl library from cloudflare seems to be a transitive dependency (gitea -> github.com/go-git/go-git -> github.com/ProtonMail/go-crypto) and the CVE to be fixed is only relevant for post-quantum cryptography. However, there's an open PR only for that in go-crypto (PR 142). So to me it seems as if gitea isn't affected.

I'm not really sure if gitea flags everything touching the security-relevant parts of the software as security-related in their release notes.

That said, IIRC each minor upgrade of gitea isn't so minor after all and either breaks e.g. the API or does some non-trivial DB migrations which is why the minor isn't touched mostly because people have reservations about that.

If my analysis turns out to be wrong or a vulnerability is fixed in the future w/o a backport to the previous minor upstream, then the only way forward is marking gitea on stable as insecure unless we can agree on making an exception for gitea the same way it's the case for e.g. matrix-synapse where we also unconditionally backport updates.

cc @techknowlogick who's more involved in upstream development as well.

@techknowlogick
Copy link
Member

@Ma27 thanks for this review, I'll go through and respond

I'm not really sure if gitea flags everything touching the security-relevant parts of the software as security-related in their release notes.

The cloudflare bump in this release was counted as a security flag due to the dep having a CVE. This is because regardless of whether it applies to Gitea or not, it pops on security scanners as they don't have the entire context, and so it's our way of telling people who come to us that we've dealt with it.

That said, IIRC each minor upgrade of gitea isn't so minor after all and either breaks e.g. the API or does some non-trivial DB migrations which is why the minor isn't touched mostly because people have reservations about that.

Gitea doesn't use semver, so 1.20 -> 1.21 is a "major release" and can potentially include breaking changes. One of the reasons for this is SIV in go, where bumping from 1 -> 2 means rewriting the imports in every single file to bump the version. Many projects do, but for us, as we handle ~400+ PRs merged a month, we can run into merge conflicts with open PRs, and it creates a poor experience for contributors if they have to constantly resolve git conflicts.

If my analysis turns out to be wrong or a vulnerability is fixed in the future w/o a backport to the previous minor upstream, then the only way forward is marking gitea on stable as insecure unless we can agree on making an exception for gitea the same way it's the case for e.g. matrix-synapse where we also unconditionally backport updates.

then the only way forward is marking gitea on stable as insecure

Previously, in Nixpkgs, we took the patches from the PR and applied them to the older version for backporting. It's harder to do if the go.mod gets modified, but it's still possible.

For 1.21.3, we (Gitea) cut a matching 1.20 release due to the security issue present in that, but usually, once we release a new major version, the previous is counted as EOL. We are working towards changing this with an LTS version (likely will be the 1.22 release if all goes well) so that OSes don't need to backport patches themselves.

@Ma27
Copy link
Member

Ma27 commented Jan 20, 2024

The cloudflare bump in this release was counted as a security flag due to the dep having a CVE. This is because regardless of whether it applies to Gitea or not, it pops on security scanners as they don't have the entire context, and so it's our way of telling people who come to us that we've dealt with it.

Yeah, that's alright and I'm not criticizing the change itself.
What I was interested in was if we need to take action here, i.e. backport the change to 1.20 we have on 23.11.

Gitea doesn't use semver, so 1.20 -> 1.21 is a "major release" and can potentially include breaking changes.

I'm aware of that. What's relevant for me as distro maintainer here is that minor version bump for gitea is basically a major release and thus not eligible for a backport - though I understand semver mostly as tool for libraries since API changes are usually not what I'm concerned about on applications, so don't get me wrong, I'm not questioning that decision anyways.

Previously, in Nixpkgs, we took the patches from the PR and applied them to the older version for backporting. It's harder to do if the go.mod gets modified, but it's still possible.

Yep, but given that gitea doesn't seem affected I don't consider that necessary so far.

For 1.21.3, we (Gitea) cut a matching 1.20 release due to the security issue present in that, but usually, once we release a new major version, the previous is counted as EOL. We are working towards changing this with an LTS version (likely will be the 1.22 release if all goes well) so that OSes don't need to backport patches themselves.

Yeah, I think I heard of that a while ago, but great to hear that plans are getting more concrete!

Anyways, I'll merge now, there's nothing wrong with the change itself.

@Ma27 Ma27 merged commit 178182b into NixOS:master Jan 20, 2024
31 checks passed
@SuperSandro2000 SuperSandro2000 deleted the gitea branch January 20, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants