fix(Locker): don't store transport-options.ssl within the lock-file #12019
Conversation
src/Composer/Package/Locker.php
Outdated
| @@ -431,6 +431,15 @@ private function lockPackages(array $packages): array | |||
|
|
|||
| $spec = $this->dumper->dump($package); | |||
| unset($spec['version_normalized']); | |||
| // remove transport-options.ssl from lock file | |||
There was a problem hiding this comment.
The comment states whats obvious from the code but does not explain 'why'
There was a problem hiding this comment.
Initially there was a detailed comment but It took several lines to have the answer.
But I agree that the current comment is meaningless, so I see some options:
- remove this comment and let other developers try to guess the reason
- put the full description there, so comment will have the answer
- add link to this PR so the answer could be found
| @@ -431,6 +431,14 @@ private function lockPackages(array $packages): array | |||
|
|
|||
| $spec = $this->dumper->dump($package); | |||
| unset($spec['version_normalized']); | |||
| // remove `transport-options.ssl` from lock file to prevent storing | |||
| // local-filesystem repo config paths in the lock file as that makes it less portable | |||
| if (isset($spec['transport-options']['ssl'])) { | |||
There was a problem hiding this comment.
Note: you don't need to check with isset if transport-options is set first, then ssl, you can do both at once, isset($random->deep['garbage']['not']->existing); works fine :) https://3v4l.org/UAaJW
|
Thanks yes that probably makes sense. |
|
It's entirely unclear to me how this is supposed to work. Once a package is in the lock file, you can't tell which repository it originally came from during a composer install from lock, so you can't tell what TLS options to apply. All the settings necessary to download and install a package must be in the lock file, which of course includes TLS options. If you need to update TLS options in the lock file, this should be possible by running "composer update --lock" without modifying any versions. Think this needs to be reverted. I don't understand the premise in this bug either. What's wrong with having the lock file point at those files? It's a very unusual set up to define repositories in a config file rather than the composer.json can you explain the use case there a bit more and what the problem even is? |
|
@rtm-ctrlz A few more questions:
Please note if we do not get any feedback here we will have to revert this from the next Composer release. |
The goal of this change is not to store path of private certificates within Our setup looks like this:
Without this change we have several issues:
My point is that there no need to store tls-options with |
|
Does downloading dist archives from your custom repository requires to provide the certificate or only reading the metadata ? |
|
@stof both, in a nut shell there is NGinx with certificate verification serving static files generated by composer/satis |
|
@rtm-ctrlz I suggest you to confirm whether this is actually true when performing an installation from lock file (without a cache of the downloaded archive). With this patch applied, there is no way for composer to associate the locked package to a TLS certificate it would add to send (as it is not associated to your custom repository in |
|
@stof having specified
I'm not sure on how can I confirm this behaviour. |
|
@rtm-ctrlz your setup of having the repository specified outside the project is quite specific though. Wanting to have a reusable lock file based on different repository definitions looks weird. And this PR still does not result in Composer passing the TLS certificate from the global config but passing no TLS certificate.
Try downloading the archive with curl without providing a certificate, maybe. |
|
@stof Indeed there is a lack of our configuration and private registry worked without requiring certificate, thanks for pointing. With our private registry got fixed (now it requires client certificate, as intended) I'm facing that certificate is used by composer only for getting metadata, but not the dist-files, so clean install (no
Moving |
I'd prefer to see it at "global composer configuration, for developer/CI" - most our work is done with private packages and I can't see the reason to "specify repository in composer.json for every package and in case of change update them all" instead of "setup workspace (composer) with repository configuration in one place and use for every package we develop".
Indeed, we are trying to achieve (without success for now) configuration when composer is using path to certificate from composer's configuration, but not from lock file, because in our case (as I described above) using certificate from lock will lead to failures of installation (ex. two developers stores their certificates within home directory, but have different usernames, so the path to certificate will also be different). |
This still expect that all developers have an equivalent repository defined in their global config instead of making the project self-contained. I think your use case should be achieved by adding a new authentication type to Composer (authenticating with a client certificate), so that |
Agree, but having less developers than packages makes updating developers configs easer than updating composer.json for every package. I'll have a look towards implementing such authentication type for |
|
Ok in the meantime I will revert this as the patch is clearly broken. |
Description
Don't store
transport-options.sslwithin the lock-file otherwise the installation or updates will (in most cases) fail for repositories that use client TLS certificates, because during installationtransport-optionsfromcomposer.lockwill be used instead of repository settings (certificates) from composer configuration file.Example
Minimal configuration for case "private registry with client-certificates" (
~/.composer/config.json){ "repositories": { "private.registry": { "type": "composer", "url": "https://composer.registry.example.com", "options": { "ssl": { "local_cert": "/home/user/.composer/certs.d/private.cert", "local_key": "/home/user/.composer/certs.d/private.key" } } } } }Dummy project (
~/project/composer.json){ "name": "exmaple/dummy", "type": "project", "require": { "private-repo/package": "^1.0" } }Dummy project lock (
~/project/compose.lock) (with current composer){ "content-hash": "...", "packages": [ { "name": "private-repo/package", "version": "1.0.0", "transport-options": { "ssl": { "local_cert": "/home/user/.composer/certs.d/private.cert", "local_key": "/home/user/.composer/certs.d/private.key" } } } ] }In this case installation/update will fail if the environment performing such task will have different configuration (
~/.composer/config.json), because path to certificate will be different and that will lead to cURL error message ("wrong certificate").Some uses cases when this will could happen:
composer.logwithin VCS and 2+ developers with different composer configurationsFixed lock-file
With this change lock-file (
~/project/compose.lock) will look like this:{ "content-hash": "...", "packages": [ { "name": "private-repo/package", "version": "1.0.0" } ] }So composer will use TLS transport-options from configuration and installation will succeed.
Is it breaking change?
I suppose this change as non-breaking, because it may will break in only one case - composer.lock file is somehow in sync with tls certificates in all places calling
install/updateand at the same time composer configuration still contains repository configuration, but is out of sync with certificates path; for me this approach is looking quite strange for me because it is "forming install environment around lock-file".Is it te 8000 sted? - Yes