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

Library Loader Improvements #7177

Closed
wants to merge 1 commit into from
Closed

Library Loader Improvements #7177

wants to merge 1 commit into from

Conversation

darbyjack
Copy link
Member

This PR will be initially created a draft so a discussion can be made.

Background:
Towards the end of the 1.16.5 era, Spigot introduced a new preview feature for runtime library downloading. The way this works is an author can specify libraries in their plugin.yml that will then be pulled from Maven Central on the startup of the plugin.

Problem:
The current situation is that all libraries are pulled directly from Maven Central, which can be classified as a violation of their TOS (https://repo1.maven.org/terms.html) under "excessive requests and bandwidth usage" (other projects have already been yelled at by Maven Central for this).

The other problem here is that not everyone has the ability to deploy their artifacts to Maven Central. A lot of people will run their own nexus instance and host their artifacts there.

Solution:
The original implementation for this PR was going to be to add a new section to the individual plugin.yml to allow developers to add extra repositories there, but after having a discussion with @zml2008 , the concept of it being potentially dangerous to allow developers to specify their own maven repositories came into play. Given that, we decided it should be up to the server owner themselves to specify any extra repositories to download from, therefore being implemented as a setting in the Paper configuration file.

Example:

settings:
  repos:
  - https://papermc.io/repo/repository/maven-public/

@MiniDigger
Copy link
Member

while this fixes the blatant TOS violation (that upstream doesn't seem to care about), I am not sure this actually improves the situations for devs, since users will need to manually add stuff there, and we all know users don't read plugin descriptions :/

@zml2008
Copy link
Contributor

zml2008 commented Dec 22, 2021

I imagined the use case for custom repos as something servers/networks with their own dev staff could use to easily distribute internal dependencies really, or to havd their own internal mirror of Central

@electronicboy
Copy link
Member

I mean, this gets naunced because theres always a way around it, "install the plugin and my core and then restart the server twice to load stuff"

I think that it should basically be a config option to allow if we load repos from plugins or something, feels kinda weird about how we hope to expose this in a user friendly manner for devs to be able to take care of, it's not like people aren't already doing this type of thing already

@darbyjack
Copy link
Member Author

Implemented suggested kashike changes.

@darbyjack
Copy link
Member Author

I mean, this gets naunced because theres always a way around it, "install the plugin and my core and then restart the server twice to load stuff"

I think that it should basically be a config option to allow if we load repos from plugins or something, feels kinda weird about how we hope to expose this in a user friendly manner for devs to be able to take care of, it's not like people aren't already doing this type of thing already

So are you saying here that we should go with the original implementation of allowing developers to place in their plugin.yml a new section for more repositories but only pull from a Maven Central mirror if a config setting is enabled in the Paper config? If that's the case, what happens if an artifact that is needed is from a 3rd party repo but they have the setting off? They are just out of luck unless they enable it?

@aerulion
Copy link
Contributor

Maybe another possibility would be to be able to individually specify which plugins are trusted to use third party repos. In my opinion, it would be a lot easier for the developer to set the required repo links in the plugin.yml instead of somehow telling the end user which repos need to be added. Having a single option to globally accept third party repos still raises a problem if a user installs a new plugin and doesn't know it specifies new repositories.
If a plugin specifies its own repos and isn't in the trusted list, it could simply be disabled with a message saying something like "Add plugin named 'X' to the trusted list for it to work properly", which would be far more intuitive for the "casual" user - at least in my opinion.
Another benefit would be that one does not need to modify the config if a new plugin version requires a different / new repo.

@stale
Copy link

stale bot commented Feb 28, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@Andre601
Copy link

Andre601 commented Apr 3, 2022

So are you saying here that we should go with the original implementation of allowing developers to place in their plugin.yml a new section for more repositories but only pull from a Maven Central mirror if a config setting is enabled in the Paper config? If that's the case, what happens if an artifact that is needed is from a 3rd party repo but they have the setting off? They are just out of luck unless they enable it?

A bit late to this discussion, but perhaps a solution could be like this?

Whenever a plugin defines an external, non-MavenCentral repository in their plugin.yml would Paper do one of two things:

  • With setting to allow external repos enabled
    Prints a message that plugin X is downloading library Y through 3rd-party repo Z.
    Example message:
    [00:00:00 INFO] [ExamplePlugin] Downloading library 'example' from 3rd-party repository 'https://example.com/repo'.
    
  • With setting to allow external repositories disabled
    Prints a warning regarding plugin X trying to download library Y from 3rd-party repo Z and that stuff may not work as expected. Maybe even disable the plugin because of this?
    Example message:
    [00:00:00 WARN] [ExamplePlugin] Denied attempt at downloading library 'example' from 3rd-party repository 'https://example.com/repo'.
    [00:00:00 WARN] [ExamplePlugin] Plugin may not work as expected.
    

One other thing I was thinking about could be to allow defining certain libraries as "required" by the plugin, so that Paper can determine if the lack of this lib would be a breaking change or not and then cancel the enable/load process or not based on that.

The question now would be on how to implement this without breaking compatibility with Spigot itself as most plugins for sure don't want to just support Paper nor want to offer a separate version for it.
Perhaps the new stuff could be under a dedicated papermc section in the plugin.yml or in a distinct papermc.yml file?

If we would go with first would I propose the following structure as a possible idea:

papermc:
  libraries:
    # Would assume this being a lib on MavenCentral
    - library: 'com.example:maven-lib:1.0.0'
    # 
    # No required field -> Is required by default
    - repository: 'https://example.com/repo/'
      library: 'com.example:example:1.0.0'
    #
    # required: false -> Plugin may work without it
    - repository: 'https://example.com/repo/'
      library: 'com.example:another-example:1.0.0'
      required: false

In the above would Paper then load the libraries. If allowing external repos is denied would this result in paper printing a warning about example not being downloadable and disabling the plugin. Assuming that lib was on central, would it instead print a warning about another-example not being downloadable and that functionality of the plugin may be limited while it continues with the loading procedure of the plugin...

I hope you understand the general idea of my proposal here, as I'm not the best at explaining things.
It certainly would make stuff a lot more complicated but at the same time could open up a lot of possibilities to have here.

@kxmpxtxnt
Copy link

kxmpxtxnt commented Apr 5, 2022

Paper if you don't do that, I hope your sweater sleeve gets wet when you wash your hands and your cereal spoon falls into the cereal and that ijr step on these small 1x1 lego bricks every morning and the thin 2x4 lego bricks never come off other plates again.

Its just a JOKE, dont freak out immediately

@lokka30
Copy link

lokka30 commented Apr 16, 2022

Perhaps a paper.yml setting (enabled by default) could toggle the allowance of custom Maven repositories?

I mean, wouldn't you only install plugins you trust anyway? /shrug

@zml2008
Copy link
Contributor

zml2008 commented Apr 17, 2022

So I think I've managed to remember the points that came up in our last discussion about this, but to start off in response:

I mean, wouldn't you only install plugins you trust anyway? /shrug
Evaluating plugins for trust is non-trivial -- even with a fully staffed team of moderators, dev.bukkit.org wasn't perfect.

Going back to the original discussions, I think what people seemed satisfied with was:

  • plugins can provide repositories, which may be ignored if a setting in paper.yml is set
  • server admins can also provide repositories. the default repository in the configuration file would be Paper's repository (which mirrors Central)
  • server-specified repositories are checked before plugin-specified repositories

Another point is on allowing plugin developers to specify file hashes for artifacts listed in the library loader. This might be useful to ensure integrity of remote repositories, but server admins should be able to disable hash checking, for the case where admins might want to replace a plugin's library with their own patched version.

In an ideal world, I'd have this whole library loader system not even do transitive dependency resolution, instead generating the flattened dependency list in the plugin.yml at build time, but that ship has already sailed imo so there's no point in changing that.

This should provide a good balance between flexibility for plugin developers, and control for server admins.

@lokka30
Copy link

lokka30 commented Apr 18, 2022

@zml2008 - I also like those ideas. I am all for the hash detection system that you mentioned as well. In addition, server repositories being loaded before plugin repositories can supplement this by making it difficult for malicious plugins to try inject malicious libraries into other plugins to 'offset the blame'.

I mean, wouldn't you only install plugins you trust anyway? /shrug

View offtopic reply

Just to go slightly offtopic - my comment here was regarding one's own 'human antivirus' to only install things that they can trust. Absolutely, there are teams dedicated to finding malicious resources- though, it's still up to the user whether they want to download a plugin posted by a user who just joined the website (and has somehow also created a project called EssentialsX - what a coincidence 😆).

@Katzen48
Copy link

Katzen48 commented May 6, 2022

Some repositories, especially those that are used internally, may require authentication.
I think, adding fields for username and password for Basic Authentication to the PaperConfig (not the plugin.yml, because the developer would expose their credentials) would help in that case.

@stale
Copy link

stale bot commented Jul 6, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@Andre601
Copy link

Andre601 commented Jul 6, 2022

A draft PR being marked as stale.... nice

@stale stale bot removed the resolution: stale label Jul 6, 2022
@Owen1212055
Copy link
Member

In general, I think we are trying to phase away from spigot's system and instead use our own custom system for adding dependencies.
In this case, see:
#8108

Should this be kept open for more discussion if we do want to add to this?

@Owen1212055
Copy link
Member

Closing, as in general Paper plugins are hopefully going to replace this system by allowing plugins to add their own repositories for library resolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.