In #3528139: Package Manager should use a copy of Composer that is local to the current project, if available, I propose that we allow Package Manager to use a copy of Composer that is local to the project. This would greatly reduce friction when using Automatic Updates or Project Browser, or really, anything that uses Package Manager. It would particularly benefit people who use Drupal on shared hosting, who cannot always control which version of Composer they have (some hosts use outdated versions that Package Manager doesn't support, and are unwilling to update it).
However, the idea of keeping Composer's binaries in vendor runs into the wall that is drupal/core-vendor-hardening, which deletes Composer's binaries entirely and that's that.
The plugin should be configurable to skip cleaning certain packages, if desired. See #18 for proposed syntax.
Get reviews and sign-offs.
The drupal/core-vendor-hardening plugin now allows projects to opt out of cleaning certain packages. The default behavior is unchanged, but a project may add this to its composer.json to prevent a given package (composer/composer, in this example) from being cleaned:
"extra": {
"drupal-core-vendor-hardening": {
"composer/composer": false
}
}
Or, at the command line:
composer config extra.drupal-core-vendor-hardening --merge --json '{"composer/composer": false}'
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !12719
Comments
Comment #2
phenaproximaAfter pondering the vendor hardening plugin, I think what might make the most sense is...to special-case
composer/composer. We should still remove its binaries, so thatvendor/bin/composerdoesn't exist...butvendor/composer/composer/bin/composershould keep existing, and have its execute bit removed.In other words, Composer's linked binaries won't exist anymore, but its original binaries will, without their execute bits.
I think that's the right balance between making Package Manager easier to use and not having to do an extensive refactor of the entire vendor hardening plugin and its configuration. It's also safer to just special-case this one package, for this specific purpose, rather than giving people the tools to make any binary persist (especially since I've never heard of another use-case for doing that).
Comment #3
phenaproximaDiscussed the above with @catch and Slack and the feeling seems to be that I should go ahead and create that MR, then let the security team and release managers consider it.
Comment #5
phenaproximaTagging for security review to confirm that this solution doesn't introduce any security flaws, and release manager review to confirm that this change is non-disruptive and, hopefully, eligible for backport to 11.2.3.
Also tagging to point out that Drupal CMS needs this.
Comment #6
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #7
phenaproximaGet lost, bot.
Comment #8
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #9
phenaproximaThe bot is wrong and stupid.
Comment #10
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #11
phenaproximaWhat a nuisance!
Comment #12
longwaveIs there a case for making this configurable, at least with an on/off flag, for sites that will never use automatic updates and want to retain the existing behaviour?
Comment #13
longwaveAlso the bot doing this has a point:
The core commit check script ensures all files are not executable, so we need an exception or some other way to set this up.
Comment #14
phenaproximaOkay, refactored the test a bit in the hopes that it will work on CI now.
Comment #15
phenaproximaI think this is already configurable (by putting
composer/composer: ['bin']into theextra.drupal-vendor-hardeningsection of composer.json), but I should add a test to prove it.Comment #16
phenaproximaComment #17
xjmI confirmed with @phenaproxima that in HEAD, the binaries are removed entirely, so the change proposed by this issue is adding an internal configuration to make the composer binaries retained and read-only at the given path.
Composer is only a dev dependency for core, not a production one, so this mainly becomes important for sites that have Package Manager enabled (i.e. Composer is a production dependency). If this passes security review, then it's essentially a feature addition of a single internal constant and the binaries.
However, other sites could be affected depending on the sophistication of their deployment workflows. Ideally sites' production builds will be deployed with
--no-dev, but we can't be sure this is the case everywhere, so it's a bit dangerous to suddenly have these binaries popping up everywhere in unpredictable situations.I asked @phenaproxima to consider instead making this an optional configuration that is off by default, but that Drupal CMS (and perhaps later Package Manager itself) enables. That way, it could be backported safely with very little API surface or risk to existing sites, but Drupal CMS could also get access to the configuration immediately. This would also be a security hardening: sites or applications would need to opt into making the binaries available, and they would remain unavailable in the default case for deployments where they are not needed.
This will also need a CR and a release note.
Thanks!
Comment #18
phenaproximaSubmitted for your consideration: a way that keeps the status quo very much intact, but also gives Drupal CMS a path forward.
Right now, there is no way to stop a package from being cleaned. The way the plugin is written utterly precludes that. This MR provides such a way: set a package's cleanup paths to
false:If a project sets this up, then
composer/composerwill not be touched. This approach leaves the default behavior unchanged; without this override, the package will be cleaned as it currently is because the configuration will be the same as it is now.In other words, existing sites will continue working exactly as they do now with no configuration change required. Drupal CMS will be able to keep Composer's binary around, and will be responsible for taking whatever additional steps are necessary to make it read-only. (Probably a
post-update-cmdscript that runs a line or two of custom PHP, but that is completely outside the scope of core's concerns.)This is the most minimal change I could come up with that should (I hope) be eligible for backport, without changing what will happen for new or existing sites.
Comment #19
phenaproximaComment #20
phenaproximaComment #21
phenaproximaChange record drafted: https://www.drupal.org/node/3536166
Comment #22
longwaveFrom a security point of view I think this is fine, given it's new behaviour that is explicitly opt-in, so existing sites don't have to change anything. Site owners can do slightly more dangerous things with this feature but that's up to them to configure it and it's not up to us to prevent that.
I don't see any release management issues either given there is little disruption here. The only possible minor disruption is the addition of the File constraint which there is a chance is already used in the wild somewhere, though given this just uses the Symfony constraint it is likely they have made the same implementation anyway. We should add a change record for this just in case.
We should also document this new capability in
composer/Plugin/VendorHardening/README.txt, marking NW for that and the CR.Also removing the bot tag as I think the bot was correct earlier when we had that executable file in the patch, that is gone so the bot should be happy.
Comment #23
phenaproximaDocumentation and change record added.
Comment #24
longwaveNo further comments, this is as small a change as can be to get this feature in. Marking RTBC for another committer to double check everything.
Agree that this can be backported to 11.2.x so Drupal CMS can make use of it.
Comment #25
phenaproximaCommitters, please backport this to 11.2.x. 🙏
Doing so will allow this improvement to come to all existing Drupal CMS projects (see #3535731: Require Drupal 11.2.4 and add Composer as a runtime dependency for where that will be done), and it will greatly help new Drupal CMS users get set up with Package Manager (without needing to wait until 11.3.0). The change set is as small as possible to facilitate a clean backport, and I have been explicit about this request since I started talking about it with the release managers. :)
Comment #26
xjmWhen this is committed, remember to tag the issue for the release notes of the correct 11.2.x patch release (which is 11.2.3 as of now).
Comment #30
catchI think this looks good and it will help Drupal CMS site a lot.
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
Comment #32
phenaproximaI propose that we recommit this as-is.
It is an opt-in behavior that does not change the current out-of-the-box behavior in any way, nor introduce any new (potentially insecure) default configuration. Drupal CMS could immediately take advantage of it to help our users get set up with Composer more quickly, even without the other changes that were made, and subsequently reverted, in Package Manager. This one is the key.
For that reason, I am tentatively restoring RTBC on the hopes that the release managers and security team will agree with my assessment, although I understand that this was reverted out of a well-reasoned abundance of caution. Depending on how we decide to proceed, it may or may not make sense to revisit this issue, but I am optimistic that this patch will not get in the way of a more holistic, secure way for Package Manager to use a local copy of Composer.
Comment #33
catchLeft a comment on the MR.
Comment #35
phenaproximaComment #36
pameeela commentedAll seems sensible to me, thanks to all who contributed to making sense of it!
Comment #39
catchLooks good now without the package manager changes.
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!