Skip to content
This repository has been archived by the owner on Apr 23, 2023. It is now read-only.

Allow creating an unsigned CSR and adding the signature later. #82

Merged
merged 4 commits into from
Apr 24, 2021

Conversation

sanderkruger
Copy link
Contributor

This is a requirement if you want to use an HSM to sign the CSR.

This use case came up integrating with a bank's API that requires asymmetric signing with a keypair whose private key must reside in an HSM while the public key must be submitted wrapped in a CSR.

The workflow is to create the CSR, then retrieve the unsigned sequence, submit it to the HSM to sign, then add the signature to the CSR and retrieve the completed CSR.

…s a requirement if you want to use an HSM to sign the CSR.
$this->signature = $signature;
$this->signatureAlgorithm = $signatureAlgorithm;

$this->createCSRSequence();
Copy link
Collaborator

@afk11 afk11 Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note createCSRSequence is non-public, perhaps setSignature should check if $this (extends Sequence) has any children yet? It shouldn't be possible to set more than the necessary elements on this object

$this->addChild($certRequestInfo);
$this->addChild($signatureAlgorithm);
$this->addChild($signature);
}

  • someone creates CSR() with a signature and calls setSignature to override it
  • someone creates CSR() without a signature, and somehow calls setSignature twice

Maybe it should error, or remove all the children before calling createCSRSequence, or maybe clone $this, remove children, call createCSRSequence so the original object remains valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted for clearing the sequence inside createCSRSequence().

Copy link
Collaborator

@afk11 afk11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, this makes total sense.

See comments about 'setSignature', and the feature should be covered by tests

@sanderkruger
Copy link
Contributor Author

I added test cases, including behaviour when setSignature is called twice.

@fgrosse
Copy link
Owner

fgrosse commented Apr 14, 2021

Thanks also from my side for the contribution. LGTM from my side. @afk11 should we merge?

@afk11
Copy link
Collaborator

afk11 commented Apr 15, 2021

@sanderkruger Perfect, thanks for those!

@fgrosse Yep, all good! I think this one makes 2.3.0?

@fgrosse fgrosse merged commit e2806e3 into fgrosse:master Apr 24, 2021
@fgrosse
Copy link
Owner

fgrosse commented Apr 24, 2021

Just merged the PR and tagged it as v2.3.0. Thanks @sanderkruger for the contribution and @afk11 for reviewing 👍

@sanderkruger sanderkruger deleted the unsigned_csr branch May 7, 2021 06:25
Comment on lines +126 to +127
$csr->setSignature($signature, OID::SHA256_WITH_RSA_SIGNATURE);
$csr->setSignature($signature, OID::SHA256_WITH_RSA_SIGNATURE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is duplicated.

$digest = $csr->getSignatureSubject();
$signature = null;
openssl_sign($digest, $signature, $private_key, OPENSSL_ALGO_SHA256);
$csr->setSignature($signature, OID::SHA256_WITH_RSA_SIGNATURE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks in PHP 8, because CSR expects the signature to be hex-encoded, while you pass in a binary string. Generally this API appears to be unsafe.

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

Successfully merging this pull request may close these issues.

4 participants