search

phar-io / gnupg

A thin wrapper around the gnupg binary, mimicking the pecl/gnupg api
https://phar.io
Other
8 stars 2 forks source link

implode(): Invalid arguments passed #2

Closed peter279k closed 4 years ago

peter279k commented 4 years ago

Reproducing issue

wget -O phive.phar "https://phar.io/releases/phive.phar"
wget -O phive.phar.asc "https://phar.io/releases/phive.phar.asc"
gpg --keyserver hkps.pool.sks-keyservers.net --recv-keys 0x9D8A98B29B2D5D79
gpg --verify phive.phar.asc phive.phar
rm phive.phar.asc
chmod +x phive.phar

Expected

Actual

Phive 0.13.2 - Copyright (C) 2015-2020 by Arne Blankerts, Sebastian Heuer and Contributors
Copying phpunit-8.5.2.phar to ./tools/phpunit
Downloading https://squizlabs.github.io/PHP_CodeSniffer/phars/phpcs-3.5.4.phar
Downloading https://squizlabs.github.io/PHP_CodeSniffer/phars/phpcs-3.5.4.phar.asc
Copying phpcs-3.5.4.phar to ./tools/phpcs
Downloading https://github.com/phpstan/phpstan/releases/download/0.12.9/phpstan.phar
Downloading https://github.com/phpstan/phpstan/releases/download/0.12.9/phpstan.phar.asc
Downloading key CF1A108D0E7AE720
Trying to connect to keys.openpgp.org (37.218.245.50)
Downloading https://keys.openpgp.org/pks/lookup?op=get&options=mr&search=0xCF1A108D0E7AE720
Successfully downloaded key.

    Fingerprint: D326 80D5 957D C711 6BE2 9C14 CF1A 108D 0E7A E720

    Ondrej Mirtes <ondrej@mirtes.cz>

    Created: 2019-12-09

[ERROR]    An error occurred while processing your request:

          implode(): Invalid arguments passed

          #0 src/services/signature/gpg/GnupgVerificationResult.php(27)
          phar-io/phive#1 unknown file(0): PharIo\Phive\Cli\Runner->errorHandler()
          phar-io/phive#2 src/services/signature/gpg/GnupgVerificationResult.php(27): implode()
          phar-io/phive#3 src/services/phar/PharDownloader.php(90): PharIo\Phive\GnupgVerificationResult->getStatusMessage()
          phar-io/phive#4 src/services/phar/PharDownloader.php(44): PharIo\Phive\PharDownloader->verifySignature()
          phar-io/phive#5 src/services/phar/PharService.php(22): PharIo\Phive\PharDownloader->download()
          phar-io/phive#6 src/services/phar/InstallService.php(45): PharIo\Phive\PharService->getPharFromRelease()
          phar-io/phive#7 src/commands/install/InstallCommand.php(45): PharIo\Phive\InstallService->execute()
          phar-io/phive#8 src/commands/install/InstallCommand.php(37): PharIo\Phive\InstallCommand->installRequestedPhar()
          phar-io/phive#9 src/shared/cli/Runner.php(193): PharIo\Phive\InstallCommand->execute()
          phar-io/phive#10 src/shared/cli/Runner.php(60): PharIo\Phive\Cli\Runner->execute()
          phar-io/phive#11 (313): PharIo\Phive\Cli\Runner->run()
          phar-io/phive#12 {main}

          Environment: PHP 7.3.17-1+ubuntu18.04.1+deb.sury.org+1
          Phive Version: 0.13.2

          This should not have happened and is most likely a bug.
          Please report it at https://github.com/phar-io/phive/issues, make sure you include
          the full output of this error message. Thank you!

After tracing error message output, I think the problematic code snippets are as follows:

https://github.com/phar-io/phive/blob/e75aa29de010ec25f12afcbe242fc2072284f066/src/services/signature/gpg/GnupgVerificationResult.php#L27

And the $this->verificationData['status'] variable may not be array type.

To resolve this issue, I think it should cast the $this->verificationData['status'] variable to array type before making implode function call.

peter279k commented 4 years ago

And my phive.xml is as follows:

?xml version="1.0" encoding="UTF-8"?>
<phive xmlns="https://phar.io/phive">
  <phar name="phpunit" version="7.3" installed="7.3.5" location="./tools/phpunit" copy="false"/>
  <phar name="phpcs" version="^3.5.2" installed="3.5.4" location="./tools/phpcs" copy="true"/>
  <phar name="phpstan" version="^0.12.2" installed="0.12.9" location="./tools/phpstan" copy="true"/>
</phive>
peter279k commented 4 years ago

To dig this issue, I also use development repository and add some output functions before making implode function call.

.......
    public function getStatusMessage(): string {
        var_dump(gettype($this->verificationData['status']));
        var_dump($this->verificationData['status']);

        return \implode("\n", $this->verificationData['status']);
    }

.......

After I use the repository on master development branch, I found $this->verificationData['status'] variable is as follows:

/path/to/phive/src/services/signature/gpg/GnupgVerificationResult.php:27:
string(7) "integer"
/path/to/phive/src/services/signature/gpg/GnupgVerificationResult.php:28:
int(9)
theseer commented 4 years ago

Thanks for the report. It took me a while to reproduce this. It seems like this only happens with ext/gnupg installed.

Your fix though is not addressing the underlying issue. The problem seems to lay in our wrapper implementation which deviates from the ext/gnugpg results:

Our Wrapper:

array(1) {
  [0]=>
  array(5) {
    ["fingerprint"]=>
    string(16) "4AA394086372C20A"
    ["validity"]=>
    int(0)
    ["timestamp"]=>
    string(10) "1536419702"
    ["status"]=>
    array(3) {
      [0]=>
      string(15) "[GNUPG:] NEWSIG"
      [1]=>
      string(55) "[GNUPG:] ERRSIG 4AA394086372C20A 1 10 00 1536419702 9 -"
      [2]=>
      string(35) "[GNUPG:] NO_PUBKEY 4AA394086372C20A"
    }
    ["summary"]=>
    int(128)
  }
}

ext/gnupg:

array(1) {
  [0]=>
  array(5) {
    ["fingerprint"]=>
    string(16) "4AA394086372C20A"
    ["validity"]=>
    int(0)
    ["timestamp"]=>
    int(1536419702)
    ["status"]=>
    int(9)
    ["summary"]=>
    int(128)
  }
}

Apart from the maybe non obvious type confusion for the timestamp, the status element is completely wrong. This clearly qualifies as a bug. But not in phive but the gnupg wrapper. I'll open/move this issue over to it.

theseer commented 4 years ago

Note to self: https://github.com/rwinlib/gpgme/blob/master/include/gpgrt.h#L152

peter279k commented 4 years ago

Thanks for your time to dig issue in advanced. And it seems that the gnupg result is not different.

I think it can let these results be consistency and this issue will be fixed :).

peter279k commented 4 years ago

@theseer, do you have any possible solution to resolve this issue?

If possible, I think you can describe possible solution approach and I can work on that and create related PR about this issue :).

theseer commented 4 years ago

Sure.

Workaround

Disable ext/gnupg until fix is released.

Fix

It's a two (maybe three) step thing:

  1. The result array returned in the phar-io/gnupg wrapper's ::verify() method needs to be adjusted to be identical to the one from ext/gnupg. My current assumption is that the status property equals the gnupg return code. But that's not been properly checked yet.

  2. We use the status property to show the raw output on error. That's obviously wrong as ext/gnupg only returns an int here and our wrapper puts all the text output into it. We need to adjust the error handling not to use the status property. Potentially by having a map that translates the error code into a string (https://github.com/phar-io/gnupg/issues/2#issuecomment-623189928).

  3. ~(optional) If we can find a way for ext/gnupg to return the raw output to be more verbose, we should implement an identical api in our wrapper to get the output the same way. And add the call to our error reporting.~ Not possible

peter279k commented 4 years ago
  1. (optional) If we can find a way for ext/gnupg to return the raw output to be more verbose, we should implement an identical api in our wrapper to get the output the same way. And add the call to our error reporting.

I think it's not possible after looking at the official gnupg function usage reference.

I also prefer using the method 2 and provide some other ways to resolve this issue:

  1. Create new class named StatusHandler.php and convert status code to current message with GnuPG const status message lists.

  2. Create new method named convertStatus inside harIo\GnuPG\GnuPG class. And do same work as above approach.

I think it would be quick to resolve this issue and it's very similar with method 2 you mention.

@theseer, do you have any suggestion about this :)?

theseer commented 4 years ago

I guess it becomes more and more obvious that trying to provide a drop-in alternative rather than an actual wrapper with some additional logic isn't as beneficial as I believed when the library got created.

If - at least for now - we keep the approach of mimicking the ext/gnupg API, the only change needed here is the ensure that we return the correct status code in our cli wrapper. So basically, only step 1 is part of this project.

The step 2 is within phar-io/phive. I think we can create a sort of value object GnupgStatus, that get's the original status code as int supplied via constructor and has asString() translating it into human readable form. We can of course also provide asInt(). And, for convenience, an isSuccess to internalize that decision.

theseer commented 4 years ago

I removed the optional step 3.

theseer commented 4 years ago

Given for all the cases I could come up with the status and return code are identical and match what the linked gpgme header file lists, I'll assume that's correct and implement it that way.

Since ext/gnupg has ::geterror I'll implement that as the hook for getting the error text.

Thanks for the research!

peter279k commented 4 years ago

@theseer, thanks for your commit.

After using the native PHP GnuPG::geterror() method, it cannot get current status message:

Here are my code snippets:

use PharIo\GnuPG\Factory;
use PharIo\FileSystem\Directory;

$executor = new Factory();
$gnupg = $executor->createGnuPG(new Directory('/home/user/.gnupg'));

$result = $gnupg->verify('9D8A98B29B2D5D79', file_get_contents('./phive.phar.asc'));

var_dump($result);
var_dump($gnupg->geterror());

Actual result

/data/gnupg/test.php:15:
array(1) {
  [0] =>
  array(5) {
    'fingerprint' =>
    string(16) "9D8A98B29B2D5D79"
    'validity' =>
    int(0)
    'timestamp' =>
    int(1572391508)
    'status' =>
    int(9)
    'summary' =>
    int(128)
  }
}
/data/gnupg/test.php:16:
bool(false)
peter279k commented 4 years ago

@theseer, after doing more tests about native GnuPG::geterror method, it seems that this method is only affected by some methods are "failed" or has exception message.

For example, turning off the GnuPG error mode and using error PGP SIGNATURE via GnuPG::verify method.

These code snippets are as follows:

use PharIo\FileSystem\Directory;
use PharIo\GnuPG\Factory;

$executor = new Factory();
$gnupg = $executor->createGnuPG(new Directory('/home/user/.gnupg'));
$gnupg->seterrormode(\Gnupg::ERROR_SILENT);

$result = $gnupg->verify('9D8A98B29B2D5D79', '1234');

var_dump($result);
var_dump($gnupg->geterror());

Actual result

/data/gnupg/test.php:16:
bool(false)
/data/gnupg/test.php:17:
string(13) "verify failed"
theseer commented 4 years ago

I'm not sure what you are trying to tell me here:

That I have to change the error mode in the factory for the ext/gnupg so gnupg::geterror works?

That the wrapper isn't working?

Something else? :)

peter279k commented 4 years ago

@theseer, thanks for your reply.

I mean the GnuPG::geterror wrapper behavior is different from ext/gnupg::geterror method.

I just use the Factory to create my GnuPG wrapper quickly. Forgive me that this approach lets you misunderstand what I mean :cry: ...

I think this latest commit cannot resolve this issue...

theseer commented 4 years ago

No worries.

Let's sort things out.

Implode call issue (this very ticket)

The actual implode call is (or was rather) not part of this repository - it happened in phar-io/phive. But phive relied on the status returned by our wrapper to be an array of strings rather than the int as it should have been because ext/gnupg returns an int in status.

The wrapper code has been changed to now have the return code of the underlying gnupg run as status.

phive

This change of course theoretically broke phive to now neither work with the extension nor the cli wrapper. So phive was changed to not do the implode call anymore but use the ErrorString class (https://github.com/phar-io/phive/blob/master/src/services/signature/gpg/GnupgVerificationResult.php#L29) provided by phar-io/gnupg.

We actually don't even use the wrapper implementation of geterror in phive because I have no means of accessing its instance when we get the result - and don't even need it.

Inconsistencies

There are two (potential) inconsistencies:

Strings returned

Our implementation maps the "official" error strings from the gnupg source code (see https://github.com/phar-io/gnupg/blob/master/makeErrorCodeMap.sh#L3). The ext/gnupg extension has hardcoded error strings (for example: https://github.com/php-gnupg/php-gnupg/blob/master/gnupg.c#L1538).

Unless someone feels the urge to grep through the source code of the extension and map out all their error messages with the corresponding code, there is very little chance both implementations return identical strings.

False or not

The geterror implementation is stateful, returning the error string matching the error code of the last whatever was run. While I consider that a very impractical implementation choice (to say the least..), there's not much I can do if the goal of this project is to mimic the API provided.

Now for your input: If I understood correctly, you basically are telling me I should change https://github.com/phar-io/gnupg/blob/master/src/Factory.php#L27 to $gpg->seterrormode(\Gnupg::ERROR_SILENT); so I'll geterror will "always" work?

peter279k commented 4 years ago

Now for your input: If I understood correctly, you basically are telling me I should change https://github.com/phar-io/gnupg/blob/master/src/Factory.php#L27 to $gpg->seterrormode(\Gnupg::ERROR_SILENT); so I'll geterror will "always" work?

Yes, that's correct.

peter279k commented 4 years ago

Our implementation maps the "official" error strings from the gnupg source code (see https://github.com/phar-io/gnupg/blob/master/makeErrorCodeMap.sh#L3). The ext/gnupg extension has hardcoded error strings (for example: https://github.com/php-gnupg/php-gnupg/blob/master/gnupg.c#L1538).

Unless someone feels the urge to grep through the source code of the extension and map out all their error messages with the corresponding code, there is very little chance both implementations return identical strings.

I think we should let ext/gnupg and PharIo\GnuPG\GnuPG wrappers about geterror method behavior be consistency.

theseer commented 4 years ago

Our implementation maps the "official" error strings from the gnupg source code (see https://github.com/phar-io/gnupg/blob/master/makeErrorCodeMap.sh#L3). The ext/gnupg extension has hardcoded error strings (for example: https://github.com/php-gnupg/php-gnupg/blob/master/gnupg.c#L1538).

Unless someone feels the urge to grep through the source code of the extension and map out all their error messages with the corresponding code, there is very little chance both implementations return identical strings.

I think we should let ext/gnupg and PharIo\GnuPG\GnuPG wrappers about geterror method behavior be consistency.

Fine with me. Do you feel like providing a PR for the map based on the pecl source?

theseer commented 4 years ago

Now for your input: If I understood correctly, you basically are telling me I should change https://github.com/phar-io/gnupg/blob/master/src/Factory.php#L27 to $gpg->seterrormode(\Gnupg::ERROR_SILENT); so I'll geterror will "always" work?

Yes, that's correct.

Okay, can do I guess.

theseer commented 4 years ago

In general: You do agree the problem this ticket was about is solved?

peter279k commented 4 years ago

In general: You do agree the problem this ticket was about is solved?

Yes. This issue is resolved, thanks :).

peter279k commented 4 years ago

Fine with me. Do you feel like providing a PR for the map based on the pecl source?

Yes. It's correct. We should let GnuPG has detailed status text on PECL source.

peter279k commented 4 years ago

@theseer, I also create this issue to make status text consistency.

Thanks for your help again :).

theseer commented 4 years ago

I'm not sure that issue you opened for ext/gnupg will make sense to them. What's the "native gpg binary wrapper" you refer to?

peter279k commented 4 years ago

I'm not sure that issue you opened for ext/gnupg will make sense to them. What's the "native gpg binary wrapper" you refer to?

The "native gpg binary wrapper" is about the gpg command.

theseer commented 4 years ago

I'm not sure that issue you opened for ext/gnupg will make sense to them. What's the "native gpg binary wrapper" you refer to?

The "native gpg binary wrapper" is about the gpg command.

As in, the gnupg binary called? If so, how? Because merely running gpg will not generate any structured PHP style output. If you refer to phar-io/gnupg's call, the resulting array is a result of parsing - and no longer contains the status text blob.

What is it you want them to do?

peter279k commented 4 years ago

I hope the parsed verified output should contain status text blob, not just a status code and it can be consistency gpg command with --verify execution output.

And the PHP style output is just a example, and I think I should modify them to be general output to be compatible with gpg binary command executing result.