Define new interfaces for version 2 #4

jaapio · 2020-11-20T14:24:06Z

I tried to keep the interface as small as possible. There are 2 main use-cases for this library. As discussed we keep downloading keys out of the scope of this library. When I was thinking about the interface design I think there is no need to have support to read keys from files. The only location where we need files is in the CliWrapper because the gng executable needs this.
The pecl implementation can just use strings to import keys. So there is no need to support files from this library perspective.

Rather than implementing all kinds of response objects, I think we can better throw exceptions in case actions are failing.

Verify signed messages.

It is required to first import a key to a keyring to be able to verify any messages.

I defined 2 methods to import keys because I wanted to have different outputs. The PublicKey class is inspired by the definition of a public key in phive. We could think about the need for a more specialized method PublicKey::getInfo but I think for now there is no real use case for this message rather than be able to print some information?

The SecretKey import is basically the same idea as I had for the PublicKey This will be the only way to create a SecretKey object which is needed to sign a message.

Sign messages

I think we can add this feature later. There is no use-case implemented that requires this in phive.

Some sources to think about:
https://github.com/paragonie/hidden-string/blob/master/src/HiddenString.php which might help to prevent leaks of secret information via serialization etc.

Looking forward to your response :-)

theseer

Thanks a lot for this first draft. It helped quite a lot already even though it may seem like it's all borked given the amount of feedback. But it's of course a lot easier to discuss when something is already there ;)

Given the overall goal of the 2.0 API is the provide a better abstraction to key importing, signature verification and signing, the GPG interface should focus more on these topics.

We have quite a few use cases to cover, I'd say:

Get information about a key based on the data string, to allow the user to decide whether or not it should be imported. While the user interaction is out of scope, we need to provide a means to query for the key information
Import a public or private key data string (Not sure btw if we can import a private key that is passphrase protected without user interaction here)
Get information about a key from the keyring via id / uid / fingerprint
Verify a signature matches a given data file / string
Sign a data file / string and get the Signature
Delete a key from the keyring

We're not yet covering all these use cases.

theseer · 2020-11-20T21:18:41Z

src/GPG.php

+     * @throws InvalidHomeDirectory
+     * @throws InvalidKey
+     */
+    public function importPublicKey(string $keyData): PublicKey;


Technically, we do not have the data needed for constructing PublicKey when "only" importing. We'd have to explicitly query the key data afterwards.

I'm also not convinced we need to return a PublicKey here, as I don't see that key to be commonly used by the caller directly after importing it to the keyring.

It would make sense though to return something like a KeyIdentifier (containing the fingerprint and providing an additional accessormethod to get the ID - which is a fraction of the fingerprint), so the caller can use the ID to query for the public key if needed.

That would save us from an redundant (internal) query.

Returning only the KeyIdentifier is also based on the data we get from gnugp:

What we get from gnupg cli calls on import would be something like this:

[0] => [GNUPG:] KEY_CONSIDERED BEEB9AED51C28445FAB142228DDB46C4EA2EBCDC 0 [1] => [GNUPG:] IMPORTED 8DDB46C4EA2EBCDC Arne Blankerts <[email protected]> [2] => [GNUPG:] IMPORT_OK 1 BEEB9AED51C28445FAB142228DDB46C4EA2EBCDC [3] => [GNUPG:] IMPORT_RES 1 0 1 0 0 0 0 0 0 0 0 0 0 0 0

According to a comment on the array returned by the pecl gnupg import function (probably based on the IMPORT_RES line above and the fingerprint from IMPORT_OK) is:

[imported] => (int), [unchanged] => (int), [newuserids] => (int), [newsubkeys] => (int), [secretimported] => (int), [secretunchanged] => (int), [newsignatures] => (int), [skippedkeys] => (int), [fingerprint] => (string)

When invalid content is passed, all values, even skippedkeys, is 0. The fingerprint value does not exist then.

If anything goes wrong, we'll throw exceptions. I'm not sure if the pecl extension provides any usable error information that we could process (There is gnupg::geterror()) but i'd envision at least somewhat useful error messages but probably only one rather generic ImportFailedException kind of thing with error code and message as best we get...

Additionally: Do you think it would make sense to wrap the string into a value object of some sort? It could verify the content is properly wrapped in the usual -----.... lines and has a useful minimum length to at least have the potential to be a valid key...?

Maybe a PublicKeyData thing?

Yes, you are absolutely right... I should have looked better at the output of gpg here.

I think we should return the keyid's that are imported. So the return type would be

Suggested change

public function importPublicKey(string $keyData): PublicKey;

/** @return KeyId[] **/

public function importPublicKey(string $keyData): array;

An import could contain multiple keyId when the imported file contains multiple keys. There is no difference between import of a private or publickey from a user point of view. But I think when we want to support passphrase protected keys we might want to make a difference. Because the handling would be a bit different. the pecl library doesn't support this so people would need to use the cli wrapper.

Yes, you are absolutely right... I should have looked better at the output of gpg here.

I think we should return the keyid's that are imported. So the return type would be

Sorry to disappoint again: If we rely on the pecl extension, it appears that we only get the count of UIDs, not the list of them. Why every someone consider that useful to hide ;)

I'm more and more wondering whether having ext/gnupg support is actually worth it ;) But we should at least design the objects so they can work in both...

An import could contain multiple keyId when the imported file contains multiple keys. There is no difference between import of a private or publickey from a user point of view. But I think when we want to support passphrase protected keys we might want to make a difference. Because the handling would be a bit different. the pecl library doesn't support this so people would need to use the cli wrapper.

Fine with me.

Regarding:

Suggested change

public function importPublicKey(string $keyData): PublicKey;

/** @return KeyId[] **/

public function importPublicKey(string $keyData): array;

Not happy yet with the signature either ;)

I'm not a fan of array as a return type because it allows for all kinds of ugly edge cases

We have more data (fingerprints for instance) that we'd possibly like to return

With the pecl extension not providing a list of UIDs, this is probably not working

What happend to the replacement of string with PublicKeyData? ;-)

theseer · 2020-11-20T21:35:36Z

src/GPG.php

+     * @throws InvalidHomeDirectory
+     * @throws InvalidKey
+     */
+    public function importSecretKey(string $keyData): SecretKey;


Probably same thing as for importPublicKey applies. Didn't check the output of gnupg here though.

theseer · 2020-11-20T22:00:53Z

src/GPG.php

+     * @throws VerificiationFailed When message was not signed with the signature
+     * @throws UnknownFingerPrint When signature doesn't belong to a known key, key must be imported first
+     */
+    public function verify(string $message, Signature $signature): KeyInfo;


This doesn't feel like the best API we can do yet:

It's a common and (sadly?) expected scenario that the public key is unknown and thus the verification will fail. So this should not be an exception.

Also failing the signature check is a valid scenario and thus does not merit an exception.

To avoid control flow via exceptions, only getting a boolean false from the pecl variant / an actual error exit code from cli should trigger exceptions.

KeyInfo is the wrong return type for this method, imho. We get verification specific results from gnupg like the timestamp of the signature creation, keyid and fingerprint used and whether or not the signature check performed succeeded. We should reflect this in our API.

theseer · 2020-11-20T22:02:59Z

src/GPG.php

+     * @since 2.1 this method can be added later because phive is no just using the verify logic
+     * @return Signature
+     */
+    public function sign(SecretKey $privateKey, string $message): Signature;


Not sure yet what gnupg provides us with there. Given that Signature currently is only a wrapper around the string this doesn't seem like the best fit.

theseer · 2020-11-20T22:05:09Z

src/Signature.php

+ *
+ * @see https://github.com/paragonie/hidden-string/blob/master/src/HiddenString.php inspired by.
+ */
+class Signature


To me, a Signature is more than just the raw string.

We should probably rename this class to something that represents this implementation better and create an actual Signature class that can be the return type for sign and contains as one property whatever this class gets renamed to to represent the signature data.

theseer · 2020-11-20T22:06:15Z

src/Signature.php

+/**
+ * This class could be prevent sigatures form being serialized.
+ *
+ * @see https://github.com/paragonie/hidden-string/blob/master/src/HiddenString.php inspired by.


I don't see this for Signature. There is nothing secret about it.

It would make sense though to do this for the SecretKeyData class.

theseer · 2020-11-20T22:08:51Z

src/SecretKey.php

+/**
+ * @todo define what is needed here.
+ */
+class SecretKey


This should probably be renamed to SecretKeyData. And a new SecretKey could be created that contains this class as one property.

theseer · 2020-11-20T22:09:34Z

src/PublicKey.php

+    public function getInfo(): string {
+    }
+
+    public function getKey(): string {


This should probably return PublicKeyData (see below for SecretKey)

theseer · 2020-11-20T22:10:56Z

src/PublicKey.php

+
+    }
+
+    public function getFingerprint(): string {


Introduce Value Object for the Fingerprint?

theseer · 2020-11-20T22:11:47Z

src/PublicKey.php

+
+class PublicKey
+{
+    private function __construct(string $id, string $fingerprint, array $uids, string $key, \DateTimeImmutable $created)


We should refrain from using scalar types where possible..

jaapio · 2020-12-04T10:10:45Z

I'm back to this issue today, I'm rethinking my approach to this because I just looked at the existing code and tried to model that more properly with some information I got from the gpg tools on my laptop.

Maybe it is better to create a better model of what happens in the real world of gpg. And see how we can adapt the existing PHP extension to fit in. I do personally think that we might want to make the gpg pecl extension optional because the CLI tool can provide us more functionality and better feedback. What would be the use-case to use gnugpg from pecl if you have the command-line tool installed?

theseer · 2020-12-04T10:59:50Z

Maybe it is better to create a better model of what happens in the real world of gpg. And see how we can adapt the existing PHP extension to fit in. I do personally think that we might want to make the gpg pecl extension optional because the CLI tool can provide us more functionality and better feedback. What would be the use-case to use gnugpg from pecl if you have the command-line tool installed?

We can probably simply stop supporting the extension as I agree with the notion that its limitations are holding us back while not providing any benefits.

So, let's just model an API that just works (tm) and makes it easy to use. We do not exactly have to focus on the gpg process details per se if we can abstract those away - if I make sense ;)

jaapio · 2020-12-04T15:04:58Z

I spend most hours today searching for a good output document for the verification action of GPG. I think I found one:
http://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob_plain;f=doc/DETAILS

The verify method has been improved now. It will return a Signature object which represents the output of a verified signature.
When the signature has a valid status the SignatureInfo will be attached. Which contains more information about when the signature was created.

SignatureData is an abstraction to make this library work without the need to access files.

I did just look at the GnuGPG output and only included the stuff that was needed from my point of view. In theory, this part could be supported using the PHP extension. Maybe some information will require an extra call.

Define new interfaces for version2

0e0c31e

theseer requested changes Nov 20, 2020

View reviewed changes

Improve signature verification

7984bf9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define new interfaces for version 2 #4

Define new interfaces for version 2 #4

jaapio commented Nov 20, 2020

theseer left a comment

theseer Nov 20, 2020

theseer Nov 20, 2020

jaapio Nov 22, 2020

theseer Nov 23, 2020

theseer Nov 23, 2020

theseer Nov 20, 2020

theseer Nov 20, 2020

theseer Nov 20, 2020

theseer Nov 20, 2020

theseer Nov 20, 2020

theseer Nov 20, 2020

theseer Nov 20, 2020

theseer Nov 20, 2020

theseer Nov 20, 2020

jaapio commented Dec 4, 2020

theseer commented Dec 4, 2020

jaapio commented Dec 4, 2020

	public function importPublicKey(string $keyData): PublicKey;
	/ @return KeyId[] /
	public function importPublicKey(string $keyData): array;

Define new interfaces for version 2 #4

Are you sure you want to change the base?

Define new interfaces for version 2 #4

Conversation

jaapio commented Nov 20, 2020

Verify signed messages.

Sign messages

theseer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaapio commented Dec 4, 2020

theseer commented Dec 4, 2020

jaapio commented Dec 4, 2020