Add sign({detached: true}) to PKCS#7

[vbuch]

Adds signDetached() to the PKCS#7 implementation (see #279) as in my usecase it worked perfectly fine the way it was proposed 3 years ago by @fadlijuniarso. The same piece of code is being used in a number of repos on GitHub but what lead me here was the piece used in pdfsign.js so thanks to @tbocek as well.
I've also added a test to cover signDetached().

[tbocek]

awesome!

[mattcollier]

Thanks for the PR!

[mattcollier]

please add @param etc to the function docs.

[vbuch]

Fixed in 09afde4

[mattcollier]

Should this be _baseSign? Do we want this API to be part of the public API?

[vbuch]

Totally correct. Fixed in 09afde4

[mattcollier]

since contentInfo is no longer being used here, the above comment becomes irrelevant. Can it be moved to some other useful location?

[vbuch]

Moved the comment where it makes sense in 09afde4

[mattcollier]

@vbuch I don't know what the future holds for _baseSign. I really don't like the look of msg._baseSign(true). How do you feel about the signature being msg._baseSign({detached: true}), and for the default case, it can just be msg._baseSign()?

[vbuch]

Would introduce a couple of lines of more code, but would look much better, yes. Will do it.

[vbuch]

Hm... I'm thinking, now that I see the code... baseSign could be needless in this case. As long as sing() has an optional options parameter we could call sign() and sign({detached: true}) instead of sign() and signDetached() What do you think @mattcollier ?
See: https://github.com/vbuch/forge/blob/master/lib/pkcs7.js#L352

I will also include a line in the README in the PKCS#7 section once you confirm either _baseSign(options) or sign(options).

[mattcollier]

@vbuch good point. It will be up to @dlongley and @davidlehn to answer this question.

[davidlehn]

I'll be a buzzkill and suggest the commit descriptions could be rewritten to remove the emoji. We haven't done that elsewhere and sparkles and ok_hand don't really mean anything in this context.

[vbuch]

@davidlehn Agreed. I'm just used to gitmoji. I'll create another PR once I'm done with @mattcollier's remarks.

[davidlehn]

@vbuch Huh, I didn't realize gitmoji was a "spec". I just thought emoji were random and people doing that were crazy. :-) It's not a bad idea I guess, but kind of weird to just use it for a few commits. Also seems a "spec" mapping keywords like ":bugfix:" to emoji would make lots more sense. But I'm getting off topic. Could just rewrite the commit message history on your branch and force push. Or we could just not worry about it. Sorry for the hassle and thanks for the patch.

[vbuch]

@davidlehn Will do that. And what about this #605 (comment)

[davidlehn]

@vbuch: I don't have strong opinions on the api, mainly because I don't know what detached signatures are used for! ;-) @dlongley? I'd say just have sign(options), and if people get confused we can add the helper function signDetached = () => sign({detached: true}).

[davidlehn]

@vbuch Was the test case generated by some other app or by the new code itself? Just wondering where to look to see if the detached sig construction is correct. Not quite sure where spec text is for that.

[vbuch]

What I am using detached signatures for is signing PDFs. The one in the test is generated by the code. I'm working on an actual test for its validity here https://github.com/vbuch/node-signpdf Once I'm done I could possibly have better testing for the code here thou I'm not sure more is really needed.

[davidlehn]

Ok. Testing against itself is good for regressions, but hard to say if it's compatible with other tools. I'm not keen on reading that whole adobe pdf doc about this stuff. ;-) Once that options api issue is addressed will likely just merge and release this. Can fix and/or improve testing later if there are issues.

[vbuch]

@davidlehn @mattcollier @dlongley I think I did all the things we agreed on. CI passed. Seems to me this piece is good to go. 🎉

[davidlehn]

0.7.6 released with these changes. Thanks.

[dirkx]

@vbuch A bit of an odd issue - using your detached:true I am still seeing the payload in the PKCS#7.

I.e running node x.js | openssl pkcs7 -print -text shows the contents: type: pkcs7-data field in the output.

var forge = require('node-forge');
var fs = require('fs');


// openssl req -x509 -subj /CN=foo -keyout x.key -out x.crt -nodes
var certAsPem = fs.readFileSync('x.crt');
var cert = forge.pki.certificateFromPem(certAsPem)

var keyAsPem = fs.readFileSync('x.key');
var privateKey = forge.pki.privateKeyFromPem(keyAsPem);

var p7 = forge.pkcs7.createSignedData();

p7.content = forge.util.createBuffer('TEK:12321378, TEK:1278391287189, TEK:12321378, TEK:1278391287189,TEK:12321378, TEK:1278391287189,TEK:12321378, TEK:1278391287189,TEK:12321378, TEK:1278391287189,TEK:12321378, TEK:1278391287189', 'utf8');
p7.addCertificate(cert);

p7.addSigner({
  key: privateKey,
  certificate: cert,
  digestAlgorithm: forge.pki.oids.sha256,
  authenticatedAttributes: [{
    type: forge.pki.oids.contentType,
    value: forge.pki.oids.data
  }, {
    type: forge.pki.oids.messageDigest
    // value will be auto-populated at signing time
  }, {
    type: forge.pki.oids.signingTime,
    // value can also be auto-populated at signing time
    value: new Date()
  }]
});
p7.sign();

var pem = forge.pkcs7.messageToPem(p7);

// PKCS#7 Sign in detached mode.
// Includes the signature and certificate without the signed data.
//
p7.sign({detached: true});

var out = forge.pkcs7.messageToPem(p7);

console.log(out);

[vbuch]

Sorry @dirkx. I won't be able to help. I see the lib has gone 2 versions ahead since my addition was released. Maybe something changed meanwhile.
I remember the output signature was always the same length, given the same key no matter the length of the content. I would think that is caused by not including the contents but rather only the md. I may be wrong. Not sure. Dont have the time to browse and check whats up.