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

feat: Disable short-name repository with config value #1227

Merged

Conversation

jrbeverly
Copy link
Contributor

@jrbeverly jrbeverly commented May 10, 2022

Summary

Support disabling the installation & updating of the short-name repository using the configuration value disable_short_name_repository. This does not remove any existing cache/copy of the short-name repository.

This change allows the short-name plugins repository to be disabled in distributed zip files (or existing installations) by setting the the configuration field disable_short_name_repository as yes. This has the effect of disabling asdf plugin add <name>, as no short-name repository will be configured to allow installation without a URL.

Fixes: #1194

Motivation

One of the issues that I've encountered with asdf is in the compliance/security realm. Since the short-name repository is a map of git repositories for lookup, it hides the true source of how a plugin is installed & works. This can result in confusion scenarios where someone wasn't aware they were running bash scripts from an unknown github repository, of which they were working off latest.

Documentation can always recommend running asdf plugin add <name> <git-url> (where git-url is from a known 👍 set), but it is a bit of a concern point about "what ifs" and all that.

For this change I wanted to allow the plugin repository to be disabled, requiring asdf plugin add <name> <git-url> in a way that was a minimally disruptive to the existing workflows that might rely on asdf plugin add <name>.

Other Information

For this change I was looking for a way to disable the default short-name plugins repository, so that asdf plugin add <name> <git-url> was the only way to install a plugin. I think completely removing the short-name plugins repository would be a major breaking change that would negatively impact a lot of workflows. So this change focuses on allowing it to be disabled.

An opt-out system (e.g. disable_asdf_plugins_repository) would accomplish this too, but a bug or misconfiguration could result in the asdf plugin add <name> functionality becoming enabled. By using the asdf_repository_url, as long as the distributed asdf.tar.gz has the defaults file patched to remove this configuration, then the short-name repository should never be configured.

Distributing the asdf.tar.gz with the git URL patched (or the /repository directory pre-configured) is a way of disabling the short-name repository at the moment.

A concern with this change is that the introduction of a configuration field like asdf_repository_url could/may/will give the false(?) impression that providing a custom short-name plugins repository is a supported feature (with associated overhead of defining API/etc/etc), which isn't the intention of this change (e.g. asdf plugin add <name> <git-url>). I'm assuming based on that previous conversation that it isn't your intention to support that, so I have added to the docs accordingly. If that isn't the case, then this bit can be ignored (& related warning in docs removed)

Previous discussions: #952

@jrbeverly jrbeverly marked this pull request as ready for review May 11, 2022 00:20
@jrbeverly jrbeverly requested a review from a team as a code owner May 11, 2022 00:20
lib/utils.bash Outdated Show resolved Hide resolved
lib/utils.bash Outdated Show resolved Hide resolved
Copy link
Contributor

@jthegedus jthegedus left a comment

Choose a reason for hiding this comment

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

Hi @jrbeverly you are right, as discussed in #1194 #952, we do not wish to expose the plugin repo as a public API by committing to a specification. This solution would commit us to that API.

With regards to your "Other Information" section. I would be in favour of a boolean flag to disable the shortname repo for those who are security conscious. Would you consider repurposing this PR for that solution? I imagine behaviour would log that the shortname repo is disabled and that you can add plugins via asdf plugin add <name> <git_url>.

@jrbeverly jrbeverly changed the title feat: Default short-name repository as config value feat: Support disabling short-name repository as config value Jun 23, 2022
@jrbeverly jrbeverly requested a review from jthegedus June 23, 2022 00:47
@jthegedus jthegedus changed the title feat: Support disabling short-name repository as config value feat: Disable short-name repository with config value Jun 23, 2022
@jthegedus
Copy link
Contributor

I will perform final review of this soon.

@jthegedus
Copy link
Contributor

jthegedus commented Jun 27, 2022

@jrbeverly Thanks for reworking this PR 🙏

I made some additional changes:

  • renamed the option to the more verbose disable_plugin_short_name_repository
  • wrote a test for asdf plugin list all code path with this config
  • simplified some of the test code to match existing test pattern
  • updated the docs for "Syncing the asdf Short-name Repository"
  • updated the example config with the default value "no" instead of the optional "yes" to match rest of examples

Can you give this a look and comment with your approval before I merge?

@jrbeverly
Copy link
Contributor Author

LGTM, the doc improvements talking about sync events make a lot of sense 👍

@jthegedus jthegedus merged commit 18caea3 into asdf-vm:master Jun 27, 2022
@jthegedus
Copy link
Contributor

jthegedus commented Jun 27, 2022

Awesome, thanks for your contribution @jrbeverly

[Edit] This always happens right after I click merge, but we probably should have noted how this is different from the setting plugin_repository_last_check_duration = never. It is different in that plugin_repository_last_check_duration = never will not stop the initial git clone of the asdf plugin repo, nor does it stop people running asdf plugin add <name>. I will update in future PR.

@Stratus3D
Copy link
Member

This looks great. Thanks @jthegedus and @jrbeverly !

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

Successfully merging this pull request may close these issues.

Allow configuring asdf to use a custom plugin repository
3 participants