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

Implements Common interfaces on DH key #1630

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kylekatarnls
Copy link
Contributor

No description provided.

@kylekatarnls kylekatarnls changed the title Implements Common\PrivateKey Implements Common interfaces on DH key Mar 22, 2021
@kylekatarnls kylekatarnls marked this pull request as ready for review March 22, 2021 08:51
@terrafrost
Copy link
Member

Your last commit is exactly why I didn't make DH keys implement PublicKey or PrivateKey.

What's the use case for wanting DH keys to implement those interfaces?

The use case for RSA, EC and DSA implementing those interfaces is in SSH2.php. It accepts an object that is an instance of PrivateKey. Anything that implements that interface has the sign method, which is what you need to be able to do to authenticate into an SSH server.

If there's a good use then in this case, in addition to merging this PR, it might be prudent to redo how the interfaces work in the next major release of phpseclib (phpseclib 4.0, I'm tentatively thinking, altho I guess I could make 3.1 BC breaking too idk lol)

Thanks!

@kylekatarnls
Copy link
Contributor Author

kylekatarnls commented Mar 22, 2021

I guess this could have 2 layers:

interface Common\PrivateBasicKey
{
  // The current content except sign()
}

interface Common\PublicBasicKey
{
  // The current content except verify()
}

interface Common\PrivateKey extends Common\PrivateBasicKey
{
  public function sign($message);
}

interface Common\PublicKey extends Common\PublicBasicKey
{
  public function verify($message, $signature);
}

So current interface does not change but we're able to show the shared methods across all the keys and we can type PrivateBasicKey params where any key including DH ones would be allowed.

I'm not yet sure DH is actually compatible end-to-end in my case, but I'm trying to make a certificate endpoint library and would ideally support any format phpseclib does.

@kylekatarnls
Copy link
Contributor Author

kylekatarnls commented Mar 23, 2021

Here is a simpler example of need for a common interface that includes DH:

public function addKey(Common\PublicKey|DH\PublicKey $key): void
{
  $this->store[$key->getFingerprint()] = $key;
}

Without PHP 8, we can't type it properly. Then the type here is a mix of interface+class. Still both have fully compatible getFingerprint() and the common part may be enough for some key ring app.

@terrafrost
Copy link
Member

I'm not yet sure DH is actually compatible end-to-end in my case, but I'm trying to make an certificate endpoint library and would ideally support any format phpseclib does.

Well fwiw X.509 certificates wouldn't work with DH keys because you can't sign with a DH key. With X.509 certs you have a public key and a signature. The public key is, in theory, intended to verify signatures and if you can't create a signature you can't verify one.

Let me mull on this

@kylekatarnls
Copy link
Contributor Author

For now I simplified on my side ignoring completely DH and it could be fine. Feel free to close this PR if irrelevant. Thank you 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants