Create a KeyPair service to handle general key management

Created on 24 November 2024, 10 days ago

Problem/Motivation

The existing module uses phpseclib to get some information about the keys. This function is the sole procedural function in the .module file.

Proposed resolution

I'd like to move the key_asymmetric_get_key_properties() function into a KeyPair service with the intention of expanding the service to handle other key management activities. Doing so will provide a strong API for such things that will allow us to remove the dependency on phpseclib.

Remaining tasks

API changes

Add a new KeyPair class with a getKeyProperties() method that is identical in function to the key_asymmetric_get_key_properties() function. Use the new service where the existing function was formerly called.

The update should include a wrapper function in the key_asymmetric.module file for backwards compatibility, marked
as deprecated.

✨ Feature request
Status

Active

Version

1.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States John Franklin

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @John Franklin
  • πŸ‡ΊπŸ‡ΈUnited States John Franklin

    @roderik, can you take a minute to review this?

  • πŸ‡³πŸ‡±Netherlands roderik Amsterdam,NL / Budapest,HU

    This looks good. One naming nit in MR.

    At this point, if people do not want to be dependent on phpseclib, they need to overwrite the key_asymmetric.key_pair service with their own, and they need to extend the KeyPair class because there is no interface yet.

    Two separate areas of future extension:

    1) so that other services don't need to extend KeyPair:

    • define an interface
    • make KeyPair implement the interface
    • mention the interface (not KeyPair) in the constructor method signatures

    2) If we want this to be code-free, i..e people just install another similar service that uses another library, and it (usually) gets picked up automatically, we need to make the current KeyPair service just one of potentially many, and

    • give each service a applies(): bool method (our implementation being e.g. {return class_exists('phpseclib3\Crypt\PublicKeyLoader';}
    • add an extra service (e.g. KeyPairManager) which is a service collector β†’ and also implements
      getKeyProperties() {
        foreach ($this->sortedKeypairProviders as $provider) {
          if ($provider->applies()) {
             return $provider->getKeyProperties();
          }
        }
        return some-default;
      }
      
    • tag the current KeyPair service to be such a provider
    • use the new KeyPairManager whereever we now use KeyPair, in our code.

    Those can be later extensions, when this actually gets implemented by another service / needs to be swappable. Nothing in this issue stands in the way of it.

  • πŸ‡³πŸ‡±Netherlands roderik Amsterdam,NL / Budapest,HU

    Needs Work for at least the proposed class name change. If you don't agree, you can merge.

  • πŸ‡ΊπŸ‡ΈUnited States John Franklin

    Thank you for reviewing the MR. I think #1 makes sense and I'll update the MR to go that route. #2 is more complicated than I was intending.

    For clarity, my road map is:

    step 1, refactor code into KeyPair class, no implementation changes (this MR)
    step 2, change the class implementation of KeyPair to replace phpseclib with base PHP OpenSSL calls in a way that doesn't change the behavior of KeyPair. (near future MR, possible new major version)
    step 3, add new functionality to KeyPair, such as key generation, again using base PHP OpenSSL calls. (future MR)

Production build 0.71.5 2024