Respect class inheritance

[kylekatarnls]

Hello,

As key classes are not marked final, I assume they can be extended, however key constructors does not follow inheritance.

class SubPrivate extends \phpseclib3\Crypt\EC\PrivateKey {}
class SubPublic extends \phpseclib3\Crypt\EC\PublicKey {}

echo get_class(SubPrivate::createKey('ed25519')) . "\n";
echo get_class(SubPrivate::load('-----BEGIN PRIVATE KEY-----
MC4CAQAwBQYDK2VwBCIEIB5D0i4BdwJwAqawW8w733K+P4Si6ubup0bq508lGwEj
-----END PRIVATE KEY-----')) . "\n";
echo get_class(SubPublic ::load(SubPrivate::createKey('ed25519')->getPublicKey()->toString('PKCS8')) . "\n";

Actual result:

phpseclib3\Crypt\EC\PrivateKey
phpseclib3\Crypt\EC\PrivateKey
phpseclib3\Crypt\EC\PublicKey 

As per typical inheritance mechanism, I would expect to get:

SubPrivate
SubPrivate
SubPublic

Here is a unit test to show this with code: #1878

Is it something that could be planned?

[terrafrost]

Normally you'd do that by making the code do return new static() or some such but that won't work so well if you're doing EC::createKey() or EC::load, which is really the intended use case. Like in that case doing return new static() would mean that you get back an instance of EC instead of EC\PublicKey or EC\PrivateKey. I mean, I get that EC\PublicKey and EC\PrivateKey would have load and createKey as those classes extend the EC class but calling those methods from EC\PublicKey / EC\PrivateKey really isn't the intended use case.

Maybe in v4 of phpseclib it'd be good to have Crypt\Common\EC.php, Crypt\Common\RSA.php, etc. Crypt\EC.php could then be an abstract non-instantiatable class that implements load and createKey as static methods and EC\PrivateKey.php and EC\PublicKey.php could then extend Crypt\Common\EC.php.

Of course, that would prevent what you're trying to do, all together, whereas you're trying to enable it.

For your purposes... maybe you could extend Crypt\EC.php and, for the load and createKey methods, wrap them the appropriate classes from phpseclib and then return the wrapped results?

[kylekatarnls]

Then maybe those classes should all be marked as final. Main reason for subclass in my case was a work-around of a bug that you fixed in newer version and the rest of the stuff could be rewritten using composition instead of inheritance. I think for safety the code should enforce this constraints.

Also the facade is a little "dangerous" because of:

PublicKey::load('-----BEGIN PRIVATE KEY-----
MC4CAQAwBQYDK2VwBCIEIB5D0i4BdwJwAqawW8w733K+P4Si6ubup0bq508lGwEj
-----END PRIVATE KEY-----')

returns a PrivateKey.

I'd say that it should throw an exception when obtained result does not match the class:

public static function load($key, ?string $password = null): AsymmetricKey
{
   $key = ...
   if (!$key instanceof self) {
       throw new InvalidArgumentException('Passed key cannot be used as ' . static::class . ' use ' . $key::class . '::load() instead.');
   }

  return $key;
}

Or somehow construction should not work at all so only the EC facade could create them, but it would need reflection I guess.

[terrafrost]

Those are good ideas. Beats my idea, which would be a BC break so big that it'd break unit tests. The others could be considered a BC break, too, but they're not so big BC breaks that they'd break unit tests and technically any change could be a BC break and you just kinda have to draw a line in the sand somewhere as https://xkcd.com/1172/ kinda hints at.

I'll try to make the PrivateKey and PublicKey subclasses final and I'll just disable load and createKey in PrivateKey and PublicKey. Like if you call them you'll get an exception.

In 4.0 I'll try to make it so that those methods don't even exist but that'll be a BC breaking change (most notably you wouldn't be able to do PublicKeyLoader::load('...') instance of \phpseclib3\Crypt\EC anymore) so I won't do it for 3.0

[terrafrost]

See 2487192

Thanks!

[kylekatarnls]

Nice, note that we can still have class MyOwnNonFinalClass extends EC which would have the same issue but wouldn't throw the exception.

[terrafrost]

I could actually see some valid use cases for that. So there are the nistp curves (eg. secp256k1, P-256, etc) and then there are the djb curves (Ed25519, Ed448). The djb curves are elliptic curves but things none-the-less work a little differently on them. I opted to merge them into one EC class and to kinda mask the differences but let's say I didn't. In that scenario it might have made sense to extend the base EC class and have this new class have it's own PublicKey and PrivateKey classes as well.

As a general rule I'd prefer to err on the side of letting stuff be extended than not. Carbon extends DateTime, for example. If I wanted to replay SFTP logs I could extend SFTP and replace the send_sftp_packet / get_sftp_packet methods. It might not be obvious that that use case would exist to most people but it does. And just because I can't imagine use cases wherein one might want to extend EC or RSA or whatever doesn't mean that they don't exist.

I'm willing to do it for the initial scenario you presented because, not only can I not imagine a valid use case for why you'd want to extend it in that way, you showed a use case wherein it actively confused you.

Now, sure, I suppose one could come up with a contrived use case demonstrating why EC should be final but, at a certain point, end users have to assume responsibility for their own decisions.

If you use reflection to change the visibility of a public method to be private and then try to call the private method and get an error then that's on you and you just gotta own what you did.

I mean, truth be told, a part of me kinda thinks that all methods ought to be public. Presumably people learn how to use APIs through the documentation - not through digging through the code of a library. If they have to learn how to use a library by digging through that libraries code then that library needs better documentation. And, in some cases, having private methods does kinda tie my hands. Like if someone's having some issues and I want them to add some debug code to their code the fact that some methods are private may mean that the changes I want them to make need to be even more expansive because of that. Like maybe they're having an issue with SFTP and, for diagnostic purposes, I want them to call a private method in SSH2 from one of the methods in SFTP. At that point not only do they have to make changes to the SFTP method but I also have to have them change the visibility of the SSH2 method.

[swentel]

Hmm, I feel like this is kind of BC break on a patch release - I suddenly started to have test failures in my ActivityPub module due to this - see https://www.drupal.org/project/activitypub/issues/3347350

Might be good to add this one to the release notes of at https://github.com/phpseclib/phpseclib/releases/tag/3.0.19 (with maybe an alternative