phpseclib3: PublicKeyLoader should have PrivateKeyLoader and check type

[tomsommer]

It's confusing that PublicKeyLoader can both return private and/or public keys.

The helper-class should be renamed or only work with public keys, creating a private-key equivalent.

It's specially bad with IDEs, since you don't know what kind of AsymmetricKey class you get back.

[terrafrost]

It's confusing that PublicKeyLoader can both return private and/or public keys.

I agree, to an extent. The problem is that it's called public key crypto so the loader for that is consequently named PublicKeyLoader.

I guess I could have gone with AsymmetricKeyLoader or just AsymmetricLoader but idk - I like PublicKeyLoader better.

The helper-class should be renamed or only work with public keys, creating a private-key equivalent.

That'd be a BC breaking change for the 3.0 branch so it's a non-starter there. I suppose I could say "well, not many people are using 3.0 yet so it's not gonna hurt a lot of people!" but if I do that then why would any big project switch over at all? Maybe the big projects are currently working on incorporating 3.0 and this would force them to redo some of their work whilst undermining my own credibility.

As for doing it for the master branch (which will eventually become 4.0)... idk. Realistically speaking, 4.0 is prob years away so it's not something I'm overly worried about atm.

It's specially bad with IDEs, since you don't know what kind of AsymmetricKey class you get back.

I hear ya, altho you're still gonna run into some issues since you don't know the key type that'd be returned (be it EC, RSA, DSA, etc). Sure, the most commonly used methods are the same across all of them (sign and decrypt with public keys, verify and encrypt with private keys) but there's some variance that is dependent on the key type and not whether or not it's public or private.

In so far as IDEs are concerned, Laravel makes very heavy use of Magic methods, which limits its usefulness with IDEs as well. Not that that's a good justification for phpseclib doing what it's doing but still... other big projects do IDE unfriendly things as well.

Anyway, your concerns are valid but idk that much can realistically be done about them in the 3.0 branch. I mean, I guess I could do PublicPublicKeyLoader and PrivatePublicKeyLoader but... yuck lol.

[tomsommer]

AsymmetricKeyLoader is fine. You could even do AsymmetricKeyLoader::loadPrivateKey AsymmetricKeyLoader::loadPublicKey

Or even PublicKeyLoader::loadPrivateKey() - that wouldn't break BC either.

It would also help if the @return docs were better, if you don't want to drop PHP 5.6 support :)

[terrafrost]

AsymmetricKeyLoader is fine. You could even do AsymmetricKeyLoader::loadPrivateKey AsymmetricKeyLoader::loadPublicKey

Well, like I said, too, I just like PublicKeyLoader better, as well, lol.

Like the Public-key cryptography article may start off with "Public-key cryptography, or asymmetric cryptography, is a cryptographic system which uses pairs of keys" but the URL and article title is "public-key cryptography".

In RFC4252: The Secure Shell (SSH) Authentication Protocol it doesn't talk about "Private Key Authentication" or "Asymmetric Authentication" - the section that talks about it is entitled Public Key Authentication Method: "publickey", even tho the key that you need to login with is the private key.

Maybe it would have been better if "asymmetric key cryptography" was the name that had caught on but it isn't - the name that the majority of people know is public key cryptography.

It's kinda like... Indian... in the United States that could refer to a Native American or someone from the country of India. Native American might be less ambiguous BUT, even so, I think Indian is the term that's really stuck. Like it's called the Bureau of Indian Affairs - not the Bureau of Native American Affairs.

I'm not really one to buck the system. People call it what people call it.

Or even PublicKeyLoader::loadPrivateKey() - that wouldn't break BC either.

Let me mull on that. I mean, I, personally, prefer PublicKeyLoader::load(...) but just because that's my personal preference doesn't mean that I couldn't add loadPrivate() and loadPublic() (I like loadPrivate() better than loadPublic() BTW lol). BUT if we're gonna do that we probably ought to also add loadParameters(). And just because I add that as an option doesn't mean I have to update the examples in the documentation either. I mean, adding a note that loadPrivate() and loadPublic() exist is one thing, but updating all the SSH examples to use loadPrivate() instead of load()... not really my cup of tea! (currently lol)

Internally I could make loadPrivate() and loadPublic() make use of load(). eg.

public function loadPrivate($key)
{
    $key = self::load($key);
    if (!$key instanceof PrivateKey) {
        throw new NoKeyLoadedException('The key that was loaded was not a private key');
    }
    return $key;
}

Moving away from load(), all together, would require some pretty intensive work, as even the plugins work in the same paradigm. eg. it's PKCS8::load(...) - not PKCS8::loadPrivate(...) and PKCS8::loadPublic(...) (only PKCS8 supports "parameters").

I'm leaning towards doing it but yah, give me a few days.

It would also help if the @return docs were better

I agree. And people are free to submit PR's to improve them lol. Of all the things in phpseclib, however, that's, personally, the thing I am the least interested in doing, so it'd prob be best if a PR was submitted for that stuff. It's also relatively low hanging fruit, I think. It's something most anyone can do whereas I'm probably the only person on the planet who's going to be able to effectively refactor X509.php and SSH2.php

if you don't want to drop PHP 5.6 support :)

I will for 4.0 lol. But when I drop PHP 5.6 support I'm going to embrace all the relevant features the next minimum version requires, 110%. Which, in and of itself, means fairly substantive rewrites. Like PHP 5.6 introduced [] as a new syntax for arrays and so I replaced all the array() references to [] in phpseclib v3.

[terrafrost]

Does 3bddf4d do what you want?

[tomsommer]

Fantastic, could loadParameters() not be merged into load()?

Also I would suggest loadPrivateKey vs. loadPrivate

[terrafrost]

Fantastic, could loadParameters() not be merged into load()?

It kinda already is lol. load() will load parameters along with public and private keys.

Also I would suggest loadPrivateKey vs. loadPrivate

I was on the fence about whether or not to do this but decided to go ahead and do so since the object it returns isn't Private but is PrivateKey:

55f2333

[terrafrost]

This is live in the new 3.0.5 release.

Thanks!