Typings

[shunkica]

Update the typings with improved type annotations and documentation

I don't know what to do with KeyInfo.

Right now the StringKeyInfo constructor accepts only a string, but I think there is no obstacle for it accepting a crypto.KeyLike (which is an alias for string|Buffer|KeyObject, because the key is only ever used when it is passed to crypto.Verify and crypto.Sign

Also the getKey function declares that it returns a Buffer, but currently it can return either string for StringKeyInfo or Buffer for FileKeyInfo.

Maybe something like this can be done:

export abstract class KeyInfo {
  getKey(keyInfo?: Node[] | null): crypto.KeyLike;

  getKeyInfo(key?: crypto.KeyLike, prefix?: string): string;

  attrs?: { [key: string]: any } | undefined;
}
export class StringKeyInfo extends KeyInfo {
  constructor(key?: crypto.KeyLike);
}
export class FileKeyInfo extends KeyInfo {
  constructor(filePath?: string);
}

[cjbarth]

This is some nice work here, and I have no objection to merging it as-is, but it is incomplete. Ideally we'd as JSDoc references in the code to further improve the experience of working with this library, but, at a minimum, we should sort out our long-standing issues with SignedXml.getKeyInfo() and KeyInfo.getKeyInfo(). We need much better types around those two items as they look the same, but do different things with different arguments.

[cjbarth]

You might be interested in some documentation about how to use TypeScript with JS projects. I think this command is great for getting things going and making sure we have complete coverage: npx -p typescript tsc lib/*.js --declaration --allowJs --emitDeclarationOnly --outDir types

[cjbarth]

If we can get this to a good place, I can do a semver-major release and then bring this into passport-saml and node-saml and do their imminent semver-major releases.

[shunkica]

I think this command is great for getting things going and making sure we have complete coverage

Is complete coverage actually needed here?
The purpose of the library is to sign and validate xml, and the functions typed here provide the end user with all the means to do so.

[shunkica]

Ideally we'd as JSDoc references in the code to further improve the experience of working with this library

I agree but maybe that is a new PR.

but, at a minimum, we should sort out our long-standing issues with SignedXml.getKeyInfo() and KeyInfo.getKeyInfo(). We need much better types around those two items as they look the same, but do different things with different arguments.

SignedXml.getKeyInfo() is not really a function intended to be used by the end-user anyway.
I mean, if this were a TS library that function probably wouldn't be public.

[shunkica]

Can this package-lock.json issue be fixed some other way?
I use pnpm, not npm.

[shunkica]

I have experimented with JSDoc references, and generating types with tsc, but it has problems understanding the prototype-based inheritance.

[cjbarth]

I have experimented with JSDoc references, and generating types with tsc, but it has problems understanding the prototype-based inheritance.

We can move to JS classes if that would help. They are supported in Node@14, which is our new minimum.

[cjbarth]

@shunkica , I gave these a quick overview, and they look fine to me. Types are those things that sometimes you have to iterate over, but it isn't a breaking change, so I'm not worried. When you're happy with this, ping me and I'll approve and merge it. then we can fix that KeyInfo thing and move on with life :)

[shunkica]

@cjbarth It's not perfect but it's better than it was. If you have no changes to add consider this done for now.

[djaqua]

So, I'm late to the party, but I think I have caught up with what's happening here. Bare with me -- read this slowly because this should clear up a lot of confusion. I know I can be a bit long-winded, but I promise this will shed some light on the madness.

Private Keys and Public Keys

@shunkica , @cjbarth , @LoneRifle I ended up having to learn a lot more about digital signatures than I ever really wanted to learn in order to make a PR a few months ago. According to the Wikipedia on Digital Signatures (and you can verify this on the command line with openssl):

Two main properties are required. First, the authenticity of a signature generated from a fixed message and fixed private key can be verified by using the corresponding public key. Secondly, it should be computationally infeasible to generate a valid signature for a party without knowing that party's private key.

So this means that either a private key or a public key (i.e. a certificate) can be used to generate the signature itself. The API specs that dragged me into this project in the first place even say that the service will generate a signature in the response based on the public key I use in my request (e.g. a BinarySecurityToken from the WS-Security standard)

Current xml-crypto Implementation

So, with that understanding of the digital signatures, lets look at the two ways that SignedXml uses signingKey

1. generate a signature using a private key (e.g. line 897 in signed-xml.js)
2. validate a signature using a public key/certificate (e.g. line 407 in signed-xml.js)

When generating a signature, one need only set SignedXml.signingKey to the Private Key. Optionally, an implementation of KeyInfoProvider that implements getKeyInfo by returning a valid formatting of the public key can be used in order to transmit the public key. In this case, if you use StringKeyInfo or FileKeyInfo, it doesn't matter what you construct it with since the default implementation of getKeyInfo does nothing with the key parameter

When validating a signature, SignedXml.keyInfoProvider must be an instance of KeyInfoProvider that implements getKey by returning the private key. This means that if you use FileKeyInfo or StrignKeyInfo, you must construct it with the private key.

The Reason for the Confusion

you cannot use the same instance of KeyInfoProvider (regardless of implementation) for both the generation and validation of a signature on the same instance of SignedXml because of line 380. This is because in one context, SignedXml.signingKey is being treated as the Private Key (for generating a signature) and in the other context, SignedXml.signingKey is being treated as the Public Key (for validating a signature).

The solution

• deprecate SignedXml.signingKey altogether in favor of using SignedXml.publicKey and SignedXml.privateKey -- we may be able to deprecate KeyInfoProvider in favor of something like KeyInfoProviderV2
• implement the Facade that the other guy mentioned on that other thread. I have an idea of how to do this -- whose interested?

Conclusion

Again, sorry for the long explanation, but the situation needs to be understood holistically in order to put KeyInfoProvider issues to bed ... forever. Strange as it may sound, the purpose of KeyInfoProvider is pretty ingenious (@yaronn I actually love this thing even if it causes a lot of problems), but it needs some work.

[cjbarth]

@djaqua That is a really good analysis. As a point of clarification, only a private key can generate a signature, and a public key is used to validate a signature. A public key and be derived from a private key.

Having said that, please have a look at this PR and see the changes that are being made to clarify all this and see if that makes sense with with what you've recently researched.

[shunkica]

So this means that either a private key or a public key (i.e. a certificate) can be used to generate the signature itself. The API specs that dragged me into this project in the first place even say that the service will generate a signature in the response based on the public key I use in my request (e.g. a BinarySecurityToken from the WS-Security standard)

This is incorrect, you can not generate a signature with the public key. The public key (BinarySecurityToken in wssecurity) is used to validate the signature.

The fact that the signingKey property was used to load the public key for validation has been rectified by cjbarth in #301, where he changed it into signingCert.
The property was called signingKey, but the public key was actually loaded into it during validation.

When validating a signature, SignedXml.keyInfoProvider must be an instance of KeyInfoProvider that implements getKey by returning the private key. This means that if you use FileKeyInfo or StrignKeyInfo, you must construct it with the private key.

You can validate the signature with the private key because you can generate a public key from a private key. But the person validating the signature will (should) not have you private key.
That is how the whole thing works.
I sign something with my private key, which only I have, and give you the public key. You use my public key to validate that it was actually me who signed it with my private key.

[djaqua]

@cjbarth & @shunkica

Yes, my bad. I was working from some notes I wrote several months ago. In my notes, under the "Digital Signatures" section, I noted that its based on Public Key Encryption, and under Public Key Encryption, I noted that data can be encrypted with a Public Key and require a Private Key for decryption, so I assumed that detail applied to Digital Signatures as well. And, upon further inspection, the API specs that I mentioned do indicate that content would be encrypted using the Public Key provided by the caller and that the signature generated in the response could be verified with their certificate, but its been a few months since I read that exact detail, so I assumed it applied to both encryption and digital signatures.

So yes, my bad; I have since updated my notes. Ironically, I misread the Wikipedia quote -- it specifically says that it should be infeasible to generate a valid signature without a private key.

That said, the implementation details I provided in my response ("Current xml-crypto Implementation") still apply regardless of my misunderstanding of what should and should not be possible.

I looked at the PR @cjbarth and added a comment. I'm about to start another discussion, so be ready!