Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add privateKeyPassword option for private key decryption #465

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

shunkica
Copy link
Contributor

@shunkica shunkica commented Apr 16, 2024

Introduces the privateKeyPassword option, enabling users to define the password for decrypting encrypted private keys.
The option is ignored if privateKey is not set, or if it is an instance of crypto.KeyObject

@shunkica
Copy link
Contributor Author

closes #45

README.md Outdated Show resolved Hide resolved
@@ -339,13 +340,14 @@ export class SignedXml {
private calculateSignatureValue(doc: Document, callback?: ErrorFirstCallback<string>) {
const signedInfoCanon = this.getCanonSignedInfoXml(doc);
const signer = this.findSignatureAlgorithm(this.signatureAlgorithm);
if (this.privateKey == null) {
const privateKey = this.getPrivateKey();
if (privateKey === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is null now permissible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getPrivateKey() will never return null.
It could never have been null to begin with. It could only be undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't appear to be true. While the typing system would prevent anything other than those values, not everyone uses Typescript. I see no code that would prevent any value from being used for privateKey.

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 dont think thats a rabbit hole worth going down.
By that logic this.privateKey == null also does nothing, because i could set privateKey=1, privateKey='' etc.., and that check would not catch it.
You cant reasonably expect the library to work if you set privateKey to null.
In any case an error will be thrown by the crypto library.

Copy link
Contributor

@cjbarth cjbarth left a comment

Choose a reason for hiding this comment

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

There are several little things that would up the quality of this PR in measurable ways. Please see the comments.

@shunkica shunkica requested a review from cjbarth April 26, 2024 07:12
@shunkica shunkica changed the title Add privateKeyPass option for private key decryption Add privateKeyPassword option for private key decryption Apr 26, 2024
@cjbarth
Copy link
Contributor

cjbarth commented Apr 27, 2024

One more thought for you: do you think it would make sense to allow a string or a function that returns string for privateKeyPassphrase? That would allow a function to retrieve that data from a secure vault or environment variable of some sort.

Thank you for the more complete tests.

@cjbarth
Copy link
Contributor

cjbarth commented Aug 24, 2024

@shunkica , I was just looking back at some PRs to see if we could get them landed. Let me know what you think about my last comment.

@shunkica
Copy link
Contributor Author

@shunkica , I was just looking back at some PRs to see if we could get them landed. Let me know what you think about my last comment.

Sorry this is not something I have bandwidth for right now. If you have time to implement it go ahead.

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

Successfully merging this pull request may close these issues.

2 participants