Another attempt at "Provide default sign method, add external signer and webcrypto example" #230
Conversation
vbuch
force-pushed
the
signer-external
branch
from
8201567 to
d147584
Compare
|
I don't really know what's going wrong, so I'll leave it up to you or do you need my help with this? |
|
I'll give it a go when I have the time.
|
Will be reintroduced with future work. Co-authored-by: dcbr <15089458+dcbr@users.noreply.github.com>
|
I cannot modify this PR branch, so I cloned it to my own repository and rebased onto the latest development branch. You can see the result here. |
|
@vbuch Let me bother you on run-time compatible of this signpdf: |
There was a problem hiding this comment.
I've left some comments that are (hopefully) self-explanatory.
I also want to reiterate that I think this approach is too oppinionated and too tightly bound to the pki.js lib. I don't think we should be so tightly bound to that library that we bring their typings into ours.
We should have an abstraction layer that sits between our interfaces and whichever crypto engine is being used, that way we aren't going to be opinionated about what crypto libraries are used and it'll make the implementation of external signers much more simple (and will allow us to move the default engine away from pki.js without breaking our APIs or requiring downstream refactors.
| /** Signature algorithm used for PDF signing | ||
| * @type {string} | ||
| */ | ||
| signAlgorithm: string; | ||
| /** Hash algorithm used for PDF signing | ||
| * @type {string} | ||
| */ | ||
| hashAlgorithm: string; | ||
| /** | ||
| * Method to retrieve the signature algorithm used for PDF signing. | ||
| * To be implemented by subclasses or set in the `signAlgorithm` attribute. | ||
| * @returns {Promise<string>} | ||
| */ | ||
| getSignAlgorithm(): Promise<string>; | ||
| /** | ||
| * Method to retrieve the hashing algorithm used for PDF signing. | ||
| * To be implemented by subclasses or set in the `hashAlgorithm` attribute. | ||
| * @returns {Promise<string>} | ||
| */ | ||
| getHashAlgorithm(): Promise<string>; |
There was a problem hiding this comment.
I don't think we need the redundant functions here. A "getter" function can be used if a static prop is not desirable (or we should just always use a function). I don't see a benefit to increasing the API surface here.
For example (if you need a dynamic prop or run some logic):
class MySigner extends ISigner {
get signAlgorithm() {
// some logic here
return alg;
}
There was a problem hiding this comment.
I like the "getter" approach. To be honest, I didn't know javascript had this functionality.
The original idea for having both a static attribute and the method is that subclasses that don't need custom logic (e.g. request to external service) can just use the "shorthand" notation
class MySigner extends Signer {
signAlgorithm = '...';
}But it's probably less confusing if there is just a single method/getter.
| * Get a "crypto" extension. | ||
| * @returns {pkijs.ICryptoEngine} | ||
| */ | ||
| getCrypto(): pkijs.ICryptoEngine; |
There was a problem hiding this comment.
Our abstract class should not be tightly bound to pkijs types. If you have a signer and you don't want to be using PKIJS, then this typing is overly restrictive. We should have our own interface for what a crypto engine looks like (it should be really simple IMO, just really a "sign" interface, I'd have thought), and that's it.
What is the "crypto engine" needed for?
There was a problem hiding this comment.
This should be a "private" method that does not need to be further modified/extended by subclasses (see my comment below).
| /** | ||
| * Obtain the certificates used for signing (first one) and verification (whole list). | ||
| * @returns {pkijs.Certificate[]} | ||
| */ | ||
| obtainCertificates(): pkijs.Certificate[]; | ||
| /** | ||
| * Obtain the private key used for signing. | ||
| * @returns {CryptoKey} | ||
| */ | ||
| obtainKey(): CryptoKey; | ||
| /** | ||
| * Obtain the signed attributes, which are the actual content that is signed in detached mode. | ||
| * @returns {pkijs.Attribute[]} | ||
| */ | ||
| obtainSignedAttributes(signingTime: any, data: any, signCert: any): pkijs.Attribute[]; | ||
| /** | ||
| * Obtain the unsigned attributes. | ||
| * @returns {pkijs.Attribute[]} | ||
| */ | ||
| obtainUnsignedAttributes(signature: any): pkijs.Attribute[]; |
There was a problem hiding this comment.
There seems to be mixing of sync/async methods. Why would these not be async whilst others are? If we want to allow the use of external signers, it's feasible that obtaining certificates may well be an async task.
There was a problem hiding this comment.
These should all be async, but the generated typings do not show this. How can I fix this? I also now notice that the return type has to be changed to Promise as well.
| * Obtain the signed attributes, which are the actual content that is signed in detached mode. | ||
| * @returns {pkijs.Attribute[]} | ||
| */ | ||
| async obtainSignedAttributes(signingTime, data, signCert) { |
There was a problem hiding this comment.
This is async, but the typings don't match (neither in the interface nor in the JSDoc)
There was a problem hiding this comment.
RE #230 (comment) - if you update line 124 to be @returns {Promise<pkijs.Attribute[]>} then they typings will follow
| * Obtain the unsigned attributes. | ||
| * @returns {pkijs.Attribute[]} | ||
| */ | ||
| async obtainUnsignedAttributes(signature) { |
| this.signAlgorithm = await this.getSignAlgorithm(); | ||
| this.hashAlgorithm = await this.getHashAlgorithm(); | ||
| // Get a crypto extension | ||
| this.crypto = this.getCrypto(); |
There was a problem hiding this comment.
This is a bad technique here. We're assigning the crypto prop here, and then other methods rely on it later, but if they were called out of order, or if this sign method were to be overridden in a child class, then implementors need to know this prop is meant to be set as part of the sign method.
It would be much better if this was either set in the constructor, or the crypto engine was passed as a parameter to the functions (a bit like how cmsSignedData.sign() requires it as an argument).
There was a problem hiding this comment.
Yes I agree, this should be moved to the constructor instead. I have to check why this was added here exactly.
| /** | ||
| * Verify whether the signature generated by the sign function is correct. | ||
| * @param {Buffer} cmsSignedBuffer | ||
| * @param {Buffer} pdfBuffer | ||
| * @returns {boolean} | ||
| */ | ||
| async verify(cmsSignedBuffer, pdfBuffer) { | ||
| // Based on cmsSignedComplexExample from PKI.js | ||
| const cmsContentSimpl = pkijs.ContentInfo.fromBER(cmsSignedBuffer); | ||
| const cmsSignedSimpl = new pkijs.SignedData({schema: cmsContentSimpl.content}); | ||
|
|
||
| return cmsSignedSimpl.verify({ | ||
| signer: 0, | ||
| trustedCerts: [], | ||
| data: pdfBuffer, | ||
| }, this.getCrypto()); | ||
| } |
There was a problem hiding this comment.
Is there any real-world use for this function in our library or is it just a testing helper? I'd suggest there's no need to provide a verify method as part of PDF signing, unless we are also offering PDF verification too
There was a problem hiding this comment.
No it's for testing purposes only.
|
This may be better discussed in a dedicated issue, but I don't really like this approach as it's mixing the responsibilities of signature generation (the actual signing), with the CMS structure generation. At the moment the signing interface is really simple and allows for any external signer to be used to generate signatures - all you need to do is effectively provide a single function that signs the provided data. see https://github.com/vbuch/node-signpdf/blob/v3.2.4/packages/utils/src/Signer.js What this change is really doing is taking the CMS structure generation in-house (which is all good) but it's mixed-in with the signing logic. IMO we need to separate these concerns and provide a way to generate the CMS structure, and then a way to do the signing. We can think of it a bit like a pipeline. First the PDF needs to be created (or loaded), then a CMS structure is created, then the CMS structure is signed, then the CMS is embedded in the PDF. The signing part of this pipeline shouldn't have to be concerned with anything other than a buffer/uint8array that it receives and then returns a buffer/uint8array that represents the signature. |
| /** | ||
| * Get a "crypto" extension and override the function used by SignedData.sign to support | ||
| * external signing. | ||
| * @returns {pkijs.ICryptoEngine} | ||
| */ | ||
| getCrypto() { | ||
| const crypto = super.getCrypto(); | ||
| crypto.sign = async (_algo, _key, data) => { | ||
| // Calculate hash | ||
| const hash = await crypto.digest({name: this.hashAlgorithm}, data); | ||
| // And pass it to the external signature provider | ||
| const signature = await this.getSignature(Buffer.from(hash), Buffer.from(data)); | ||
| return signature; | ||
| }; | ||
| return crypto; | ||
| } |
There was a problem hiding this comment.
This is the reason for having this getCrypto method. But it should be considered a "private" method that is not needed for most Signer implementations.
| * Get a "crypto" extension. | ||
| * @returns {pkijs.ICryptoEngine} | ||
| */ | ||
| getCrypto(): pkijs.ICryptoEngine; |
There was a problem hiding this comment.
This should be a "private" method that does not need to be further modified/extended by subclasses (see my comment below).
|
Regarding your last point, I'm not sure I fully understand what you mean.
I would argue that the current situation is even worse/more coupled and that the implementation here is a first step towards decoupling it. For example, when subclassing the current
IMO that is exactly how the signing part is implemented in the new |
|
I agree that this is an improvement on what we've currently got (which, as you rightly point out, is either a very rigid implementation from It's really the architectural approach I don't agree with here. If we have "private" methods that we are implementing for the crypto aspect, then I'd argue they should not even be in the class at all. If we want this "all-in-one" signer class, I feel it should probably work something like: // Use the JWA spec for algs as a fairly well shared standard
type SigningAlg = 'RS256' | 'RS384' | 'RS512' | 'ES256' | 'ES384' | 'ES512' | 'PS256' | 'PS384' | 'PS512';
// we may want to add `crls` or `ocsp` info here?! or a way to fetch that info??
interface SignerOptions {
certificate: string|UInt8Array, // the certificate being used for signing
signer: (alg: SigningAlg; payload: UInt8Array) => Promise<UInt8Array>;
certificateChain?: (string|UInt8Array)[]; // the certificate chain
digestAlgorithm?: SigningAlg;
authenticatedAttributes?: Record<string, string>; // provide a way to override / add authenticated attrs
unauthenticatedAttributes?: Record<string, string>; // provide a way to override / add unauthenticated attrs
}
class Signer {
constructor(opts: SignerOptions) {
// just pseudo code, this would need to be validated, and stored against props properly
this.opts = opts;
}
async sign(pdfBuffer: UInt8Array): Promise<UInt8Array> {
// construct CMS encoded attrs for signing
const sig = await this.opts.signer(algsignedAtrrs);
// finish constructing CMS data
return cmsData;
}This is obviously really simplified, it doesn't provide any way to support a After writing this out, I still don't like this approach. I think that we really need our own I will try to find some time to put something together... |
|
Ok, thanks for the further clarifications. So if in the meantime you can come up with an alternative implementation, I'm all for it and very curious to see the result :) Edit: interface SignConfiguration {
// Signature dictionary
subFilter: string; // Subfilter of the signature dictionary
// Signed attributes
signingTimeAttr: boolean; // Flag indicating whether the `signing-time` signed attribute should be included in the signature
signingCertAttr: boolean; // Flag indicating whether the `signing-certificate-V2` signed attribute should be included in the signature
revocationInfoAttr: boolean; // Flag indicating whether the `adbe-revocationInfoArchival` signed attribute should be included in the signature
// Unsigned attributes
signatureTimestamp: boolean; // Flag indicating whether the signature should be timestamped (using the `signature-time-stamp` unsigned attribute)
// Other
dss: boolean; // Flag indicating whether the signature certificates and revocation information should be added to the DSS
documentTimestamp: boolean; // Flag indicating whether the document should be timestamped
}; |
|
Sorry. I'm really detached from the communication here. Just sharing an opinion I think I've shared before that is pretty shallow and maybe doesn't help: I prefer to have a minimal scope to maintain and maximum extensibility + modularity. So I imagine if you figure out a way to keep all the pkijs stuff in a separate package it would be the most awesome thing. Again, I didn't read the whole discussion you two have here and I don't think I will in the coming couple of months. |
The external dependencies are not mocked in the tests which may cause all kinds of problems on different platforms. E.g. tests pass on my Mac but fail on my Windows and in Github Actions.
To make sure develop is still workable on I've reverted #220 and readded its commits on top of the reverted version.
We need mocks so that these commits can be merged in.