No Function Comments in classes and Methods

[kezenwa]

Hello
Really love your work especially with this multi Ca feature (i.e ability to use the class not for just LetsEncrypt alone but also ZeroSsl and BuyPass).

In the comments you promised to merge this branch with Master. May I make a suggestion ?

Observation #1 : No Function Comments in classes and Methods

I noticed none of the class methods and function have comments / comment headers e.g

public function generateECKey() {
}

ought to look this way

/**
 * Generate Elliptic Curve (EC) private key (used as account key or private key for a certificate).
 * @param  string $curve_name Default is  'P-384' 
 * Supported Curves by Let’s Encrypt: P-256 (prime256v1), P-384 (secp384r1), P-521 (secp521r1)
 * @return Returns the generated EC private key as PEM encoded string. Throws an Exception if the EC key could not be generated.
 * /
public function generateECKey() {
}

Would have made the changes myself BUT don't know Git much. Guess when I have more time, would try knowing some GIT.

I understand these information already are on the index page but we need not always consult that page to get more information on what a class method does.

Once again, Really appreciate the work.

Thanks

[skoerfgen]

Hello @kezenwa

Thanks!

Sounds like a good idea, I will definitely consider adding this information to the source code. However currently I have to solve some problems before Bypass and ZeroSSL are usable, see #26 (comment)

[kezenwa]

Observation #2 : USE of same KEY both for ACCOUNT Key (account_key) and PRIVATE Key

Noticed this script uses same key both for LetsEncrypt account and for Certificate Private key which I believe isn't really cool from a security point.

On a lighter note, the script creates a FullChain certificate but we sometimes equally need a Chain Certificate and a Cert e.g array('cert' => $certificate, 'chain' => $certChain) . Please kindly direct on how to achieve these also.

Thanks

[kezenwa]

Observation #3 : Please make all PRIVATE Methods PROTECTED

This is to allow those wanting to further extend the class the ability to cleanly do so without having to unnecessarily modify the core.
I understand why methods are made PRIVATE especially in the ACMEv2 class but because everyone's needs differs, we sometimes need make modifications and PRIVATE methods means we need modify the core also

[skoerfgen]

Observation 2 is not true, ACMECert does not use the same key for account key and for the private key for the certificate:

You have to pass the account key to the loadAccountKey function. And the private key for the certificate to the getCertificateChain function. If accidentally the same key is used in both places Let's Encrypt does not allow it. You get the following error in this case:

ACME_Exception: Error finalizing order :: certificate public key must be different than account key (urn:ietf:params:acme:error:malformed) in ACMECert.php

also see: https://community.letsencrypt.org/t/use-existing-private-key-as-an-account-key/35766/3

---

On a lighter note, the script creates a FullChain certificate but we sometimes equally need a Chain Certificate and a Cert e.g array('cert' => $certificate, 'chain' => $certChain) . Please kindly direct on how to achieve these also.

This should not be difficult to implement (The first certificate in the fullchain is the main certificate and the rest is the chain). I'll add a function for this.

[skoerfgen]

Observation 3: sounds reasonable! I'll probably to change this in the next release.

[kezenwa]

Observation 2 is not true, ACMECert does not use the same key for account key and for the private key for the certificate:

You have to pass the account key to the loadAccountKey function. And the private key for the certificate to the getCertificateChain function. If accidentally the same key is used in both places Let's Encrypt does not allow it. You get the following error in this case:

ACME_Exception: Error finalizing order :: certificate public key must be different than account key (urn:ietf:params:acme:error:malformed) in ACMECert.php

also see: https://community.letsencrypt.org/t/use-existing-private-key-as-an-account-key/35766/3

On a lighter note, the script creates a FullChain certificate but we sometimes equally need a Chain Certificate and a Cert e.g array('cert' => $certificate, 'chain' => $certChain) . Please kindly direct on how to achieve these also.

This should not be difficult to implement (The first certificate in the fullchain is the main certificate and the rest is the chain). I'll add a function for this.

Just seeing this BUT I have done the Modifications already.

Am kinda new to most things Certificates hence didn't understand a lot BUT after hours of going through different source codes + few Googlings, was able to observe that which I have done.

Reason why I needed those 2 anyways was to be able to submit certificates to SCT logs.

Thanks for your response and time