Hardened plugin loading #12787
Hardened plugin loading #12787
Conversation
Definitely something to lean into a bit more as we work through this change. |
Yes, this is the kind of problem that we should fix with pre-release support (or definitive rejection), and changing the Makefiles for the plugins to support whichever alternative we choose. |
|
Haven't tried the macOS build (don't have a working machine on hand), but it seems that the failing tests on platforms other than Linux is due to the version being reported by the binary does not match its name, so the subsequent build fails after the installation succeeded. For some reason the 0.2.19 binary reports the right version for the linux/amd64 build, but not for the windows/amd64 or the darwin/arm64 ones. Case in point: # Linux
[elbaj@elbaj-hashicorp packer] (hardened_plugin_loading)*$ ./packer-plugin-comment_v0.2.24_x5.0_linux_amd64 describe
{"version":"0.2.24","sdk_version":"0.0.11","api_version":"x5.0","builders":[],"post_processors":[],"provisioners":["-packer-default-plugin-name-"],"datasources":[]}
# Windows
packer@DESKTOP-BD6B7SS MINGW64 ~
$ ./packer-plugin-comment_v0.2.24_x5.0_windows_amd64.exe describe
{"version":"1.0.0","sdk_version":"0.0.11","api_version":"x5.0","builders":[],"post_processors":[],"provisioners":["-packer-default-plugin-name-"],"datasources":[]}Note: we should probably fail at |
lbajolet-hashicorp
force-pushed
the
hardened_plugin_loading
branch
from
55db95b to
6350791
Compare
I'm looking into this now. |
Out of curiosity, what are you looking into? The binary was published with v1.0.0 so that's why the test fails, I believe we can fix them by re-publishing the plugins with the right version, i.e. locally build for Windows/MacOS (no need to sign even since we don't execute them), and use |
packer/plugin-getter/plugins.go
Outdated
| log.Printf("[TRACE] version %q of file %q does not match constraint %q", pluginVersionStr, path, pr.VersionConstraints.String()) | ||
| continue | ||
| } | ||
| if strings.Replace(pluginVersionStr, "v", "", -1) != describeInfo.Version { |
There was a problem hiding this comment.
Not that it is important here but if we find ourselves doing more of these version checks hashicorp/go-version supports checking if "v1.0.0" is equal to "1.0.0". It essentially drops the v prefix when comparing using the Equal function.
This is a single usage so probably overkill but calling out in case we need to do this more.
There was a problem hiding this comment.
Yeah about this one: fun-fact, this changes in the follow-up PR, since the replace makes the versions invalid when working with v1.0.0-dev, as they end up being 1.0.0-de :D
Since we're removing the alternative plugin installation directories in favour of only supporting installing them in the PACKER_PLUGIN_PATH directory, we only return one directory when getting the known plugin directories.
Since we'll be only accepting plugins installed locally along with a shasum, and those that adopt the convention we introduced with required_plugins, we remove support for plugins that only have a packer-plugin-.* type name.
Since we'll only look in the plugin directory, and not from multiple sources now, for installing/listing plugins, we can simplify the structures which used to accept multiple directories so they only accept one.
Since the plugin directory should now be unique instead of a list, we can use it directly from the receiver's structure instead of as a parameter to the function.
The comments weren't capitalised correctly, and the directory example included the `packer-plugin-' prefix, which should not be the case in reality.
The plugin installation list should be sorted according to a name first, then version second basis. Right now, we only rely on the glob to add plugin installs to this list, making the order unreliable since the lexicographical order is not the order in which we want to see the same plugin ordered (e.g. v1.0.9 > 1.0.10). To fix this, we implement a logic for sorting a list of installations that does what's described above with more accuracy.
Right now we had two paths for discovering installed plugins, i.e. through plugin-getter's `ListInstallations' function, or through the `Discover' call, which relied on a glob to list the installations. This was required since we allowed plugins to be installed in multiple locations, and with different constraints. Now that we force a certain convention, we can consolidate the logic into ListInstallations, and rely on that logic in `Discover' to load a plugin into the PluginConfig for the current Packer run.
When a plugin is loaded from Packer, we now check that the version it reports matches what the name implies. In case there's a mismatch, we log, and reject the binary.
Since the plugins remove subcommand now only loads valid plugins, it cannot run on mock data anymore, and the logic for creating a valid plugin hierarchy is not present in this repository, but does in another, so we move those tests from here to that repository in order to continue testing them.
When Packer orders the plugins by their version, we'd assumed it was ordered from highest to lowest. However, this was the case because our implementation of `Less' was actually returning `>=', which is not what it was supposed to do. This commit therefore fixes the implementation of `Less' to do what it is documented to do, and ensures the behaviour is correct through additional testing. Changing this requires some changes to the loading process as well because of the aforementioned assumption with regards to ordering.
When a pre-release version of a plugin is locally installed, it may or may not be loaded depending on the constraints expressed in the template being executed. If the template contains constraints for loading the plugin, it would be ignored, while if that wasn't present, it would be loaded. This is inconsistent, and deserves to be addressed, which is what this commit does. With this change, plugin pre-releases are now loaded, provided the version reported matches the constraints, independently from its pre-release suffix `-dev'. Also, if a release with the same version is also installed alongside the pre-release version of a plugin, it will have precedence over the pre-release.
There was a problem hiding this comment.
This looks good. The failed tests are due to the tests plugins github.com/sylviamoss/comment having mixed versions that fail the newly added plugin version validation check (version filename must equal the version in the describe command). I think we should move to a trusted plugin for these test (e.g packer-plugins-hashicups).
I'm approving so that you can merge once the tests have been resolved.
In order to test the Packer subcommands, some tests rely on a plugin: github.com/sylviamoss/comment. This plugin is outdated compared to our SDK, and some of the binaries releases don't match what is expected by Packer, i.e. v1.0.0 vs. v0.2.8. To fix that, we migrate to use the hashicups plugin, which is our demo plugin for Packer, and is actually maintained, and up-to-date, which will make those tests more reliable moving forward.
lbajolet-hashicorp
force-pushed
the
hardened_plugin_loading
branch
from
bb563f7 to
e1b2669
Compare
|
Alright, botched rebases are botched; given we're merging both this and #12828, I'll fix it in that branch instead, and merge both PRs as one |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
As part of the plugin loading changes, this PR enforces the convention set by
required_pluginsin terms of the name of the plugin binary, and the file hierarchy in which it's supposed to be loaded from.This makes it impossible to load plugins built from dev branches however as they don't have a final version (follow-up PRs to come for that), and the Makefiles from the plugins will need to evolve to support that.
Marking as Draft for the moment so we don't make life too hard for us devs, but this is ready for review as it stands.