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

polymc,prismlauncher: replace PolyMC with PrismLauncher #196624

Merged

Conversation

Minion3665
Copy link
Member

@Minion3665 Minion3665 commented Oct 18, 2022

Description of changes
What still needs to be done
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.05 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@LunNova
Copy link
Member

LunNova commented Oct 18, 2022

cc @cleverca22 @starcraft66

@Minion3665 Minion3665 force-pushed the replace-polymc-with-prismlauncher branch from 038428f to 78bed18 Compare October 18, 2022 17:13
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 18, 2022
@Minion3665 Minion3665 added the 1.severity: security Issues which raise a security issue, or PRs that fix one label Oct 18, 2022
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation labels Oct 18, 2022
@Scrumplex
Copy link
Member

The PolyMC maintainers need to be asked if they want to be prismlauncher maintainers (and should probably decide if they want PolyMC to be removed as done here or if they would prefer it to have a security vulnerability added instead)

I would love to be a co-maintainer. I don't use NixOS on my desktop yet, but I am still interested in packaging the app for Nix.

@Minion3665 Minion3665 force-pushed the replace-polymc-with-prismlauncher branch from 30be5c2 to 273ffcc Compare October 18, 2022 17:35
@Minion3665
Copy link
Member Author

The PolyMC maintainers need to be asked if they want to be prismlauncher maintainers (and should probably decide if they want PolyMC to be removed as done here or if they would prefer it to have a security vulnerability added instead)

I would love to be a co-maintainer. I don't use NixOS on my desktop yet, but I am still interested in packaging the app for Nix.

Sure! For that todo to be checked off I more meant the people who previously maintained the PolyMC package, however I have added you to the maintainer list

@Minion3665 Minion3665 force-pushed the replace-polymc-with-prismlauncher branch from 273ffcc to 24e9221 Compare October 18, 2022 17:37
@ofborg ofborg bot added 8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 18, 2022
pkgs/games/prismlauncher/default.nix Outdated Show resolved Hide resolved
pkgs/games/prismlauncher/default.nix Outdated Show resolved Hide resolved
pkgs/games/prismlauncher/default.nix Outdated Show resolved Hide resolved
@Minion3665 Minion3665 force-pushed the replace-polymc-with-prismlauncher branch 2 times, most recently from 1877377 to b7a5735 Compare October 18, 2022 17:54
@Minion3665
Copy link
Member Author

As we're backporting to 22.05, I'm going to move the release-note changes from 22.11 to 22.05 (and modify the existing notes about PolyMC there)

@Minion3665 Minion3665 force-pushed the replace-polymc-with-prismlauncher branch from b7a5735 to 8a8f7d8 Compare October 18, 2022 18:04
@ofborg ofborg bot requested a review from Scrumplex October 18, 2022 18:04
@Minion3665 Minion3665 force-pushed the replace-polymc-with-prismlauncher branch from 8a8f7d8 to b2c3436 Compare October 18, 2022 18:06
@Minion3665 Minion3665 marked this pull request as ready for review October 18, 2022 18:07
@LunNova
Copy link
Member

LunNova commented Oct 18, 2022

I might've messed up adding the backport with how close it is to 22.11 - @SuperSandro2000 removed my backport tag on the PR this depends on.

@Minion3665
Copy link
Member Author

Minion3665 commented Oct 19, 2022

One very minor thing that most likely is not an issue with nix packaging though: The news feed still shows the PolyMC news.

Yep, I can see that too
I'll bring it up in the discord server they're already aware and there's discussion in the #development discord channel about it

Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

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

can't test right now, but package LGTM

@LunNova
Copy link
Member

LunNova commented Oct 19, 2022

the PR just needs to be manually done
~ sandro on matrix

So I think we merge this PR with changes for the 22.11 release notes including the removal of polymc, and for 22.05 a manual backport PR which changes the 22.05 notes and is worded differently because the polymc package isn't getting removed there.

@Minion3665
Copy link
Member Author

the PR just needs to be manually done
~ sandro on matrix

So I think we merge this PR with changes for the 22.11 release notes including the removal of polymc, and for 22.05 a manual backport PR which changes the 22.05 notes and is worded differently because the polymc package isn't getting removed there.

Sounds good to me, I'll add another commit for updating the 22.11 release notes for easy cherry-picking

When we backport, we might consider putting it in #196721, as that only exists to be a dependency for this backport

@Minion3665 Minion3665 force-pushed the replace-polymc-with-prismlauncher branch from a6a4978 to cb167a1 Compare October 19, 2022 18:02
- As polymc has been hostily taken over, prismlauncher (the fork) should
  be used instead
- The previous commit packages prismlauncher, this commit makes it so
  that installing polymc will give an error message
- Previously PolyMC was the suggested replacement for MultiMC
- As PolyMC is marked as insecure and prismlauncher is a replacement,
  this commit suggests using it instead
@Minion3665 Minion3665 force-pushed the replace-polymc-with-prismlauncher branch from cb167a1 to 49c81f0 Compare October 19, 2022 18:07
@SuperSandro2000
Copy link
Member

There is no need for removing polymc and replacing it in my opinion, marking it as insecure is sufficient. We don't want to patronize our users, we just want to make them aware of the situation. It is up to them whether they trust the remaining developers and want to continue polymc or not.

MultiMC, PolyMC and PrismLauncher all utilize a meta data server so that the clients know which jar files to download. This is basically RCE by design. The sole last developer that kicked all other developers from everything, so far also attracted attention by having a domestic terrorism manifest in his Steam bio. Also the majority of active developers moved to PrismLauncher as a consqeuence of this hostile takeover which makes this more or less a rename of PolyMC.

@Minion3665
Copy link
Member Author

the PR just needs to be manually done
~ sandro on matrix

So I think we merge this PR with changes for the 22.11 release notes including the removal of polymc, and for 22.05 a manual backport PR which changes the 22.05 notes and is worded differently because the polymc package isn't getting removed there.

Sounds good to me, I'll add another commit for updating the 22.11 release notes for easy cherry-picking

When we backport, we might consider putting it in #196721, as that only exists to be a dependency for this backport

I've updated the release notes for 2211 and 2205 to do that. On the todo list we're now only waiting for confirmation that we want to remove PolyMC entirely, preferably by @cleverca22 and @starcraft66

@piegamesde
Copy link
Member

I am still against deleting the polymc package for now, and strongly against doing so on stable.

  • Having marked the polymc package as insecure serves the purpose of informing users of the situation, and letting make their own decision
  • People are free to opt-in into using polymc if they choose to trust its developer(s)
  • We can still remove the polymc package once everybody has moved on
  • The polymc package does not need any to have any maintainers for now.
  • I'm not sure whether we have a formal stability policy for the stable channels, but I'd consider deleting package attributes an absolute no-go.

@LunNova
Copy link
Member

LunNova commented Oct 19, 2022

Removal isn't getting backported.

@SuperSandro2000
Copy link
Member

The polymc package does not need any to have any maintainers for now.

The problem is that it fetches all java libraries for Minecraft with the information from a remote server. Even if we freeze the package in the current state it can't be considered safe to use in the default configuration and is not from the remaining developers. It would be irresponsible to keep it.

@piegamesde
Copy link
Member

@SuperSandro2000 I totally get your point, but I don't see a contradiction to what I said. And this is not a question of security or safety, but one about trust.

@SuperSandro2000
Copy link
Member

I don't think that the remaining polymc developer is trustworthy at all based on the actions in the last day(s).

@LunNova
Copy link
Member

LunNova commented Oct 19, 2022

Security in the context of a package with by-design RCE requires trust.

You can't split them! Security always needs trust, if you don't review every change yourself.

Copy link
Contributor

@oxalica oxalica left a comment

Choose a reason for hiding this comment

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

Offline game works. LGTM as changes on master.

@SuperSandro2000 SuperSandro2000 merged commit ab6c14b into NixOS:master Oct 19, 2022
@Minion3665 Minion3665 deleted the replace-polymc-with-prismlauncher branch October 19, 2022 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.severity: security Issues which raise a security issue, or PRs that fix one 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: clean-up 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 9.needs: port to stable A PR needs a backport to the stable release. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace PolyMC with successor