Allow FileKeyInfo to accept a string for the cert

[cjbarth]

Currently FileKeyInfo only accepts a file path for construction. The means that the cert must be on a file system somewhere. In our security context, the certs and keys aren't on disk, they are in memory, so ideally we'd like to construct FileKeyInfo using a string that is the cert, not a path.

Would you be open to a PR that changed this code to accept a string that is either a file path or a cert? The basic switch would be that if the string isn't a valid path, then treat the string as if it were a cert and proceed as normal.

[cjbarth]

I also note that the parameter key isn't used in getKeyInfo(), which seems like it might be a bug. Thoughts?

[cjbarth]

Finally, in this same area of code, it doesn't seem that there is support for multiple certs as seems is supported by SAML. Is that something that could be added?

[cjbarth]

Here is another reference to the spec: https://www.w3.org/TR/xmldsig-core1/#sec-X509Data.

[ganesha289]

@cjbarth, Have added some changes here - #249. Its in draft state currently. Please review..
Also, I have some questions added on the PR.. please check..

[LoneRifle]

@cjbarth - FileKeyInfo is a convenience implementation of a KeyInfoProvider interface, which exposes just two methods, getKeyInfo() and getKey(). As the name suggests, the class is backed by a file handle.

Is there any reason why you don't wish to maintain your own implementation of KeyInfoProvider?

[cjbarth]

Is there any reason why you don't wish to maintain your own implementation of KeyInfoProvider?

There are at least two reasons:

1. We want to allow the community to benefit from this work, so we want it to be upstream.
2. We want to make sure that the work is done correctly and in a maintainable way, so we want to discuss the changes in the correct repo.

[cjbarth]

FileKeyInfo is a convenience implementation of a KeyInfoProvider interface, which exposes just two methods, getKeyInfo() and getKey(). As the name suggests, the class is backed by a file handle.

In that case, our suggested changes might need to focus on how to make KeyInfoProvider meet our needs instead of focusing on FileKeyInfo. This is one of the big reasons we want to have this discussion here, with this projects maintainers, instead of trying to roll our own over at node-saml.

[cjbarth]

@LoneRifle As I look over the tests for this project more thoroughly, I see that there are no tests that exercise FileKeyInfo. In every case, a stub is used instead of the actual implementation.

I also note that the parameter key isn't used in getKeyInfo(), which seems like it might be a bug. Thoughts?

Given the above, my question above still stands. Even if we decide to implement this function as a StringKeyInfo (a KeyInfoProvider backed by a string), I have no tests that I can review to make sure that a) your FileKeyInfo provider is correct, or b) my StringKeyInfo will be correct.

[LoneRifle]

I also note that the parameter key isn't used in getKeyInfo(), which seems like it might be a bug. Thoughts?

@cjbarth - I unfortunately inherited the codebase with this already in place. Unsure of the original author's intent here (perhaps it's just a stub?), especially since the codebase that uses this at work is not dependent on this and is migrating to OIDC.

I'm happy for you to decide what the behaviour should be!

[cjbarth]

I'm happy for you to decide what the behaviour should be!

@ganesha289, care to comment on this?

[djaqua]

I had to reverse engineer a few things to figure out how to even use KeyInfoProvider, and from context. I actually explain it "abstractly" in the JSDocs I added in my PR, but the gist of what I think those are supposed to do is encapsulate the relationship that "Security//Signature//KeyInfo" element has to the "Security//BinarySecurityToken" element.

Yaron seems to have intended to provide an interface that allowed exotic implementations while providing an implementation that could do one thing and do it well. It's useful to understand this objective by separating the abstraction (KeyInfoProvider) and its implementation (FileKeyInfo):

The abstraction (KeyInfoProvider) enables two use cases:
1 - generate the contents of a KeyInfo element based on a signing certificate
2 - query a signing certificate by information contained within a KeyInfo element

The implementation (FileKeyInfo) accomplishes three things:
1 - conforms to the abstraction (KeyInfoProvider)
2 - generates a generic X509Data element to be a child for a KeyInfo element
3 - provides exactly one signing certificate (the cert loaded from a file)

The only real question would be: is there actually a schema-compatible use-case for the abstraction?
NO -> xml-crypto should simplify this in a new major version breaking change :evil_grin:
YES -> xml-crypto should document this relationship at a minimum (*cough cough my PR)

**EDIT: in my examples, I refer to the WS-Security strongly recommended BinarySecurityToken, but according to the actual XML Digital Signature schema, the relationship would be as follows:

1. getKeyInfo(signingKey, prefix) - builds the contents of a xmldsig KeyInfo element; could be something like

<X509Data>
  <X509Certificate>MIICHKHLLJBJNJKHLJLUJ...</X509Certificate>
</X509Data>

or

<BinaryScurityToken id="sneakyBoi">MIICHKHLLJBJNJKHLJLUJ...</BinarySecurityToken>

<!--other security header elements -->

<SecurityTokenReference>
  <Reference URI="#sneakyBoi" />
</SecurityTokenReference>

2. Conversely, getKey parses a KeyInfo element and returns "MIICHKHLLJBJNJKHLJLUJ..." (either example)

@ganesha289 @LoneRifle @cjbarth I hope this sheds some light on how these pieces seem to work together

[cjbarth]

I think this can probably be closed now that @djaqua code has been merged. I think the next step is to get it in node-saml using these change. See node-saml/node-saml#36 and other linked PRs in this discussion.

[LoneRifle]

Agreed. Fixed by #273.