-
Notifications
You must be signed in to change notification settings - Fork 190
Add ECDSA Support #531
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
base: master
Are you sure you want to change the base?
Add ECDSA Support #531
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /* eslint-disable no-console */ | ||
| // Run with `npm run example`, requires one-time `npm run build` to generate `/lib` code (and re-run if you update `/src`) | ||
|
|
||
| const select = require("xpath").select | ||
| const dom = require("@xmldom/xmldom").DOMParser; | ||
| const SignedXml = require("../").SignedXml; | ||
| const fs = require("fs"); | ||
|
|
||
| function signXml(xml, xpath, key, dest, cert) { | ||
| const sig = new SignedXml(); | ||
| sig.canonicalizationAlgorithm = "http://www.w3.org/2001/10/xml-exc-c14n#" | ||
| sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256" | ||
| sig.privateKey = fs.readFileSync(__dirname + "/" + key); | ||
| sig.publicCert = fs.readFileSync(__dirname + "/" + cert); // To populate KeyInfo, as an example | ||
| sig.addReference({ | ||
| xpath, | ||
| digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", | ||
| transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], | ||
| }); | ||
| sig.computeSignature(xml); | ||
| fs.writeFileSync(__dirname + "/" + dest, sig.getSignedXml()); | ||
| } | ||
|
|
||
| function validateXml(xml, key) { | ||
| const doc = new dom().parseFromString(xml); | ||
| const signature = select( | ||
| "/*/*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", | ||
| doc, | ||
| )[0]; | ||
| const sig = new SignedXml(); | ||
| sig.publicCert = fs.readFileSync(__dirname + "/" + key); // Note since the XML has a KeyInfo, this cert is NOT doing anything! | ||
| // Validate the cert in `KeyInfo` on your own if that is your security model. See: <https://github.com/node-saml/xml-crypto/discussions/399> | ||
| sig.loadSignature(signature.toString()); | ||
| const res = sig.checkSignature(xml); | ||
| if (!res) { | ||
| console.log(sig.validationErrors); | ||
| } | ||
| return res; | ||
| } | ||
|
|
||
| const xml = "<library>" + "<book>" + "<name>Harry Potter</name>" + "</book>" + "</library>"; | ||
|
|
||
| //sign an xml document | ||
| signXml(xml, "//*[local-name(.)='book']", "client.pem", "result.xml", "client_public.pem"); | ||
|
|
||
| console.log("xml signed successfully"); | ||
|
|
||
| const signedXml = fs.readFileSync(__dirname + "/" + "result.xml").toString(); | ||
| console.log("validating signature..."); | ||
|
|
||
| //validate an xml document | ||
| if (validateXml(signedXml, "client_public.pem")) { | ||
| console.log("signature is valid"); | ||
| } else { | ||
| console.log("signature not valid"); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| <library><book Id="_0"><name>Harry Potter</name></book><Signature xmlns="http://www.w3.org/2000/09/xmldsig#"><SignedInfo><CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/><SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/><Reference URI="#_0"><Transforms><Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/></Transforms><DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256"/><DigestValue>9d/ciWlVZkaJnJ3KBB5WY1H2Y8WRXPB2DquM0goT8jY=</DigestValue></Reference></SignedInfo><SignatureValue>uxmxGw2O3B6ylkhEXOaZd1Iupgy3sHtCoBTgbmSMSnHYOitiHXRdHjJGJdMG4EMSgItsB6k5gxrKeyQ/5LkwvMqSc0VRPXd9vavt0pYatqwWDO/r6WITLb0jzymJfNDJ4lr4OcqH4zBKX8Deb6EpS9L7S6OXNqd1vOZ0STMSSaM=</SignatureValue><KeyInfo><X509Data><X509Certificate>MIIBxDCCAW6gAwIBAgIQxUSXFzWJYYtOZnmmuOMKkjANBgkqhkiG9w0BAQQFADAWMRQwEgYDVQQDEwtSb290IEFnZW5jeTAeFw0wMzA3MDgxODQ3NTlaFw0zOTEyMzEyMzU5NTlaMB8xHTAbBgNVBAMTFFdTRTJRdWlja1N0YXJ0Q2xpZW50MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQC+L6aB9x928noY4+0QBsXnxkQE4quJl7c3PUPdVu7k9A02hRG481XIfWhrDY5i7OEB7KGW7qFJotLLeMec/UkKUwCgv3VvJrs2nE9xO3SSWIdNzADukYh+Cxt+FUU6tUkDeqg7dqwivOXhuOTRyOI3HqbWTbumaLdc8jufz2LhaQIDAQABo0swSTBHBgNVHQEEQDA+gBAS5AktBh0dTwCNYSHcFmRjoRgwFjEUMBIGA1UEAxMLUm9vdCBBZ2VuY3mCEAY3bACqAGSKEc+41KpcNfQwDQYJKoZIhvcNAQEEBQADQQAfIbnMPVYkNNfX1tG1F+qfLhHwJdfDUZuPyRPucWF5qkh6sSdWVBY5sT/txBnVJGziyO8DPYdu2fPMER8ajJfl</X509Certificate></X509Data></KeyInfo></Signature></library> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,36 @@ export class RsaSha256 implements SignatureAlgorithm { | |
| }; | ||
| } | ||
|
|
||
| export class EcdsaSha256 implements SignatureAlgorithm { | ||
| getSignature = createOptionalCallbackFunction( | ||
| (signedInfo: crypto.BinaryLike, privateKey: crypto.KeyLike): string => { | ||
| // Maybe the fix for ts-ignore below? | ||
| // const parsedPrivateKey = crypto.createPrivateKey(privateKey); | ||
| const signer = crypto.createSign("SHA256"); | ||
| signer.update(signedInfo); | ||
| // @ts-ignore | ||
| const res = signer.sign({ key: privateKey, dsaEncoding: 'ieee-p1363' }, "base64"); | ||
|
|
||
| return res; | ||
| }, | ||
| ); | ||
|
|
||
| verifySignature = createOptionalCallbackFunction( | ||
| (material: string, key: crypto.KeyLike, signatureValue: string): boolean => { | ||
| const publicKey = crypto.createPublicKey(key); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. potential for key confusion attacks i.e. rsa public key being passed here
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had looked into this, but the crypto functions seem to be validating this already. Do you mind demonstrating a key confusion attack? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's best practice to add checks (for this and the other signature-algorithms) that the verification key is the correct type (ec for this one). Currently there's no security vulnerability. If a rsa public key is passed in here, node:crypto will just verify with RSA (the key isn't converted to EC). |
||
| const verifier = crypto.createVerify("SHA256"); | ||
| verifier.update(material); | ||
| const res = verifier.verify({ key: publicKey, dsaEncoding: 'ieee-p1363' }, signatureValue, "base64"); | ||
|
|
||
| return res; | ||
| }, | ||
| ); | ||
|
|
||
| getAlgorithmName = () => { | ||
| return "http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha256"; | ||
| }; | ||
| } | ||
|
|
||
| export class RsaSha256Mgf1 implements SignatureAlgorithm { | ||
| getSignature = createOptionalCallbackFunction( | ||
| (signedInfo: crypto.BinaryLike, privateKey: crypto.KeyLike): string => { | ||
|
|
@@ -126,6 +156,36 @@ export class RsaSha512 implements SignatureAlgorithm { | |
| }; | ||
| } | ||
|
|
||
| export class EcdsaSha512 implements SignatureAlgorithm { | ||
| getSignature = createOptionalCallbackFunction( | ||
| (signedInfo: crypto.BinaryLike, privateKey: crypto.KeyLike): string => { | ||
| // Maybe the fix for ts-ignore below? | ||
| // const parsedPrivateKey = crypto.createPrivateKey(privateKey); | ||
| const signer = crypto.createSign("SHA512"); | ||
| signer.update(signedInfo); | ||
| // @ts-ignore | ||
| const res = signer.sign({ key: privateKey, dsaEncoding: 'ieee-p1363' }, "base64"); | ||
|
|
||
| return res; | ||
| }, | ||
| ); | ||
|
|
||
| verifySignature = createOptionalCallbackFunction( | ||
| (material: string, key: crypto.KeyLike, signatureValue: string): boolean => { | ||
| const publicKey = crypto.createPublicKey(key); | ||
| const verifier = crypto.createVerify("SHA512"); | ||
| verifier.update(material); | ||
| const res = verifier.verify({ key: publicKey, dsaEncoding: 'ieee-p1363' }, signatureValue, "base64"); | ||
|
|
||
| return res; | ||
| }, | ||
| ); | ||
|
|
||
| getAlgorithmName = () => { | ||
| return "http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha512"; | ||
| }; | ||
| } | ||
|
|
||
| export class HmacSha1 implements SignatureAlgorithm { | ||
| getSignature = createOptionalCallbackFunction( | ||
| (signedInfo: crypto.BinaryLike, privateKey: crypto.KeyLike): string => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| -----BEGIN EC PRIVATE KEY----- | ||
| MHcCAQEEIOg1FiE/iu8uoRXX3UvBs53JIEsjkcf9IbMpJsfkvG30oAoGCCqGSM49 | ||
| AwEHoUQDQgAE69ImJGeiClnYW20zXK3L+w5q463+PN302fpmEDE/6xTEbG/KIxcA | ||
| d77nrzo5Iq4ve2SqL0Bk1Yxk2V/1f8t52g== | ||
| -----END EC PRIVATE KEY----- |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| -----BEGIN CERTIFICATE----- | ||
| MIIB3zCCAYWgAwIBAgIUYjTFVRq9oJ9JsEdzs9GEp+Ro2nYwCgYIKoZIzj0EAwIw | ||
| RTELMAkGA1UEBhMCQVUxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoMGElu | ||
| dGVybmV0IFdpZGdpdHMgUHR5IEx0ZDAeFw0yNjAxMjcyMDA0MjhaFw0yNzAxMjIy | ||
| MDA0MjhaMEUxCzAJBgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYD | ||
| VQQKDBhJbnRlcm5ldCBXaWRnaXRzIFB0eSBMdGQwWTATBgcqhkjOPQIBBggqhkjO | ||
| PQMBBwNCAATr0iYkZ6IKWdhbbTNcrcv7Dmrjrf483fTZ+mYQMT/rFMRsb8ojFwB3 | ||
| vuevOjkiri97ZKovQGTVjGTZX/V/y3nao1MwUTAdBgNVHQ4EFgQUKdQQ4ogzLU06 | ||
| Gypz35quxaLJr50wHwYDVR0jBBgwFoAUKdQQ4ogzLU06Gypz35quxaLJr50wDwYD | ||
| VR0TAQH/BAUwAwEB/zAKBggqhkjOPQQDAgNIADBFAiADI3VXNdnYMIIFlVLS6Ss2 | ||
| E+tamOigyNvruaKT+0YiGQIhALYU9Dyu2fRRvULX7sBpv7Dxk/4ynUCcCTJ1L9SK | ||
| O9bJ | ||
| -----END CERTIFICATE----- |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| -----BEGIN CERTIFICATE----- | ||
| MIIBEDCBuKADAgECAggVM8YfT8EdfTAKBggqhkjOPQQDAjAPMQ0wCwYDVQQDEwR0 | ||
| ZXN0MB4XDTIxMDczMDA3NTU1NVoXDTIxMDczMDA3NTU1NVowDzENMAsGA1UEAxME | ||
| dGVzdDBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABHpb/uly4GB+9G2BKgToi57/ | ||
| XzqapEdo2Pys48RMtj8tc6WE2BO0TJoR1KJZ1Bu05OQ0aOwyFGo1QY65V6sgONIw | ||
| CgYIKoZIzj0EAwIDRwAwRAIgV50ULGELC8aTSxOmTPptqHjOgKlbKLlQ+CuErOUB | ||
| CucCIBvn/IWSLPVqwoQNzP7VnRgk9mZvUuTW0MaIf/4lhOc7 | ||
| -----END CERTIFICATE----- |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| <message><content>Just remember ALL CAPS when you spell the man name</content><Signature xmlns="http://www.w3.org/2000/09/xmldsig#"><SignedInfo><CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#" /><SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha256" /><Reference URI=""><Transforms><Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature" /><Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#" /></Transforms><DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256" /><DigestValue>9A4u9FDDvQS0fcqS76EbS5Ir95wh3JOu2QldyyfWrHs=</DigestValue></Reference></SignedInfo><SignatureValue>A1Vz+93PgSq3auxwqW087exDtOYgSazYTSgYlXZgWVZI6tKXwrZZ9O4SdQiHHI4Y2Ro8Ho5zgf+HjpN/ushvPw==</SignatureValue><KeyInfo><X509Data><X509Certificate>MIIBEDCBuKADAgECAggVM8YfT8EdfTAKBggqhkjOPQQDAjAPMQ0wCwYDVQQDEwR0ZXN0MB4XDTIxMDczMDA3NTU1NVoXDTIxMDczMDA3NTU1NVowDzENMAsGA1UEAxMEdGVzdDBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABHpb/uly4GB+9G2BKgToi57/XzqapEdo2Pys48RMtj8tc6WE2BO0TJoR1KJZ1Bu05OQ0aOwyFGo1QY65V6sgONIwCgYIKoZIzj0EAwIDRwAwRAIgV50ULGELC8aTSxOmTPptqHjOgKlbKLlQ+CuErOUBCucCIBvn/IWSLPVqwoQNzP7VnRgk9mZvUuTW0MaIf/4lhOc7</X509Certificate></X509Data></KeyInfo></Signature></message> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Use package name import instead of relative path.
The import on line 6 uses a relative path (
require("../")) rather than the package name. Example files should userequire("xml-crypto")to demonstrate usage from an end-user's perspective, matching the pattern inexample/example.js.📦 Proposed fix
const select = require("xpath").select const dom = require("@xmldom/xmldom").DOMParser; -const SignedXml = require("../").SignedXml; +const SignedXml = require("xml-crypto").SignedXml; const fs = require("fs");Based on learnings: "Example files in the node-saml/xml-crypto repository should use
require("xml-crypto")(the package name) rather than relative paths to build artifacts, since they demonstrate usage from an end-user's perspective."📝 Committable suggestion
🤖 Prompt for AI Agents