Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define new interfaces for version 2 #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions src/CliWrapper.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

declare(strict_types=1);

namespace PharIo\GnuPG;

use PharIo\Executor\Executor;
use PharIo\FileSystem\Directory;

final class CliWrapper implements GPG
{
public function __construct(Executor $executor, Directory $home, ?Directory $temp = null)
{
}

public function importPublicKey(string $keyData): PublicKey
{
// TODO: Implement importPublicKey() method.
}

public function importSecretKey(string $keyData): SecretKey
{
// TODO: Implement importSecretKey() method.
}

public function verify(string $message, Signature $signature): KeyInfo
{
// TODO: Implement verify() method.
}

public function sign(SecretKey $privateKey, string $message): Signature
{
// TODO: Implement sign() method.
}
}
44 changes: 44 additions & 0 deletions src/GPG.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php

declare(strict_types=1);

namespace PharIo\GnuPG;

interface GPG
{
/**
* @param string $keyData raw key data
*
* @throws InvalidHomeDirectory
* @throws InvalidKey
*/
public function importPublicKey(string $keyData): PublicKey;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <Arne@Blankerts.de>
    [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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? ;-)


/**
*
* @since 2.1 this method can be added later because phive is no just using the verify logic
* @param string $keyData raw key data
*
* @throws InvalidHomeDirectory
* @throws InvalidKey
*/
public function importSecretKey(string $keyData): SecretKey;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Verifies the message is signed with the sigature
*
* Ensures that the signature is valid.
*
* @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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Uses the secret to create a signature.
*
* @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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
10 changes: 10 additions & 0 deletions src/InvalidHomeDirectory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

declare(strict_types=1);

namespace PharIo\GnuPG;

class InvalidHomeDirectory extends Exception
{

}
10 changes: 10 additions & 0 deletions src/InvalidKey.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

declare(strict_types=1);

namespace PharIo\GnuPG;

class InvalidKey extends Exception
{

}
11 changes: 11 additions & 0 deletions src/KeyInfo.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

declare(strict_types=1);

namespace PharIo\GnuPG;


class KeyInfo
{

}
35 changes: 35 additions & 0 deletions src/PeclWrapper.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

declare(strict_types=1);

namespace PharIo\GnuPG;

use PharIo\FileSystem\Directory;

final class PeclWrapper implements GPG
{
public function __construct(Directory $home, ?Directory $temp = null)
{
$this->gpg = new \Gnupg();
}

public function importPublicKey(string $keyData): PublicKey
{
// TODO: Implement importPublicKey() method.
}

public function importSecretKey(string $keyData): SecretKey
{
// TODO: Implement importSecretKey() method.
}

public function verify(string $message, Signature $signature): KeyInfo
{
// TODO: Implement verify() method.
}

public function sign(SecretKey $privateKey, string $message): Signature
{
// TODO: Implement sign() method.
}
}
26 changes: 26 additions & 0 deletions src/PublicKey.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

declare(strict_types=1);

namespace PharIo\GnuPG;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should refrain from using scalar types where possible..

{
}

public function getId(): string {
}

public function getInfo(): string {
}

public function getKey(): string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably return PublicKeyData (see below for SecretKey)


}

public function getFingerprint(): string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduce Value Object for the Fingerprint?


}
}
15 changes: 15 additions & 0 deletions src/SecretKey.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php

declare(strict_types=1);

namespace PharIo\GnuPG;

/**
* @todo define what is needed here.
*/
class SecretKey
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{


public function getKeyData() {}
}
21 changes: 21 additions & 0 deletions src/Signature.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?php

declare(strict_types=1);

namespace PharIo\GnuPG;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
class Signature
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
/**
* Will return internal sigature data
*
* This can be stored to a file for example.
*/
public function getData(): string {
}
}
10 changes: 10 additions & 0 deletions src/VerificiationFailed.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

declare(strict_types=1);

namespace PharIo\GnuPG;

class VerificiationFailed extends Exception
{

}