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.

Sign up for GitHub

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

Jump to bottom

Implement PKCS#7 signature verification. #706

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Implement PKCS#7 signature verification. #706

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cfff1d7
Implement PKCS#7 signature verification.
roysks Aug 22, 2019
c1f5084
Add certificate chain verification.
roysks Aug 22, 2019
fd656a1
Add callback for each PKCS#7 signature verification.
roysks Aug 22, 2019
43ff714
Support arbitrary attributes (e.g. S/MIME caps)
roysks Sep 3, 2019
25c7b12
pkcs7: allow for null validityCheckDate for consistency with x509
fergusean Oct 10, 2019
b4385e2
Add example of PKCS#7 verification to README
roysks Mar 11, 2020
fce10da
Add util.compareDN documentation and update variable names
roysks Mar 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add certificate chain verification.
  • Loading branch information
@roysks
roysks committed Oct 11, 2019
commit c1f5084eb81c00b0e6810c0767b85216ddaa1da1
15 changes: 12 additions & 3 deletions lib/pkcs7.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,9 +384,12 @@ p7.createSignedData = function() {
addSignerInfos(mds);
},

verify: function() {
verify: function(caStore, options) {
if(!caStore) {
throw new Error('You must provide a CA store for PKCS#7 verification.');
}
var mds = addDigestAlgorithmIds();
return verifySignerInfos(mds);
return verifySignerInfos(mds, caStore, options || {});
},

/**
Expand Down Expand Up @@ -546,7 +549,7 @@ p7.createSignedData = function() {
msg.signerInfos = _signersToAsn1(msg.signers);
}

function verifySignerInfos(mds) {
function verifySignerInfos(mds, caStore, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you're providing for options.callback. This seems to me to be an unconventional way of implementing a callback which would not be compatible with util.promisify implementations which expect the last param to be a callback function.

That said, can you say more about the use case for callbacks here? Given that all the code here is synchronous, does the callback add value?

If the callback is going to remain part of this API, it appears to me that it is not being called in a number of situations. For example, if the conditional on L556 fails we end up at L677 retrurn rval; and the callback is never called. Also, for the most part this function is written to throw errors at various times when it may be appropriate to call the callback with callback(err); instead.

Do you think it would be simpler to eliminate the callback functionality?

Copy link
Author

Choose a reason for hiding this comment

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

I added a callback so that as a part of the verification process you would get information on good signatures to display to the user. I would like to keep it for that reason. I'll clean it up and make it promisifyable and we'll see how it looks.

Copy link
Author

Choose a reason for hiding this comment

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

Pushed, let me know what you think. Double check the code flow? L556-573 is a contained if/else block.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello, are you saying you implemented the callback in order to return different values from a synchronous call vs a callback call? It does appear that is what is happening? I think we need the sync/callback versions to return the same data structure, is there something preventing consistency here? And if the synchronous version can return the same data structure as the callback, do we still need the callback? https://github.com/digitalbazaar/forge/pull/706/files#diff-d4c741d422d58aea2d7ffefe1687abd2R678

Also, without a process.nextTick or something to defer execution, I believe this code effectively runs synchronously with or without the callback.

Copy link
Author

Choose a reason for hiding this comment

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

The purpose of the callback isn't to get a result so much as status updates on each signature processed. The verify function could return an array of results, yes. At that point I would just eliminate the callback. Is that what you would prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand now that my confusion has been caused by referring to this passed in function as a callback. I think of a callback as a function that is called when the called function completes/errors. This is reflected in the util.promisify API such that the Promise is resolved when the callback is invoked, which is not what you are attempting to do here.

If this were a long running asynchronous process, a solution involving event emitters might be appropriate.

Now that I understand what you're trying to do and options.onSignatureVerificationComplete handler might be appropriate.

Do you have some use case that requires there to be status updates on each signature? If not, then I think removing the status callback function would be best.

Copy link
Author

Choose a reason for hiding this comment

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

If I modify the return to contain the same info as the status callback/emitter then no, I don't need it. I'll do that now.

Copy link
Author

Choose a reason for hiding this comment

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

I ended up going with the per-signature event emitter vs a return of an array of results. Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

There should be a common pattern in the whole lib for events and more detailed results. That's a bigger project. This does do the common return true/false and that will work for now.

var content;
var rval = true;

Expand Down Expand Up @@ -613,6 +616,11 @@ p7.createSignedData = function() {
throw new Error('Unable to find signing certificate.');
}

var verifyOpts = {};
if(options.validityCheckDate) {
verifyOpts.validityCheckDate = options.validityCheckDate;
}

if(signer.authenticatedAttributes.length === 0) {
// if ContentInfo content type is not "Data", then
// authenticatedAttributes must be present per RFC 2315
Expand Down Expand Up @@ -654,6 +662,7 @@ p7.createSignedData = function() {
bytes = asn1.toDer(attrsAsn1).getBytes();
signer.md.start().update(bytes);
}
forge.pki.verifyCertificateChain(caStore, msg.certificates, verifyOpts);

// verify digest
var verified = signerCert.publicKey.verify(signer.md.digest().bytes(), signer.signature, 'RSASSA-PKCS1-V1_5');
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/pkcs7.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -738,19 +738,19 @@ var UTIL = require('../../lib/util');

it('should verify PKCS#7 signature w/o attributes', function() {
var p7 = PKCS7.messageFromPem(_pem.signedDataNoAttrs);
var verified = p7.verify();
var verified = p7.verify(PKI.createCaStore([_pem.certificate]), { validityCheckDate: new Date('2012-12-25T00:00:00Z') });
ASSERT.equal(verified, true);
});

it('should fail to verify bad PKCS#7 signature w/o attributes', function() {
var p7 = PKCS7.messageFromPem(_pem.signedDataNoAttrsBadSig);
var verified = p7.verify();
var verified = p7.verify(PKI.createCaStore([_pem.certificate]), { validityCheckDate: new Date('2012-12-25T00:00:00Z') });
ASSERT.equal(verified, false);
});

it('should verify PKCS#7 signature w/attributes', function() {
var p7 = PKCS7.messageFromPem(_pem.signedDataWithAttrs1950UTCTime);
var verified = p7.verify();
var verified = p7.verify(PKI.createCaStore([_pem.certificate]), { validityCheckDate: new Date('2012-12-25T00:00:00Z') });
ASSERT.equal(verified, true);
});

Expand Down