Add support for <X509Certificate /> in <KeyInfo />; remove KeyInfoProvider

[cjbarth]

This PR makes reference to how our handling of certificates in <KeyInfo /> is incorrect. The documentation for this is here.

Further information about this can be found in this comment thread.

[cjbarth]

@shunkica, @LoneRifle, @markstos, @ganesha289, I've made changes to KeyInfo handling and cleaned a lot of things up. I renamed things, so this too is a breaking change, but I think it makes things much clearer. Please let me know what you think and feel free to review the code.

[LoneRifle]

lgtm otherwise

[codecov]

Codecov Report

Merging #301 (599ca5a) into master (67b3a78) will increase coverage by 1.70%.
The diff coverage is 98.41%.

❗ Current head 599ca5a differs from pull request most recent head aa2f62f. Consider uploading reports for the commit aa2f62f to get more accurate results

@@            Coverage Diff             @@
##           master     #301      +/-   ##
==========================================
+ Coverage   81.04%   82.74%   +1.70%     
==========================================
  Files           7        5       -2     
  Lines         865      875      +10     
==========================================
+ Hits          701      724      +23     
+ Misses        164      151      -13     

Impacted Files	Coverage Δ
lib/signed-xml.js	83.36% <97.77%> (+2.09%)	⬆️
lib/utils.js	97.91% <100.00%> (+1.25%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[djaqua]

@cjbarth @LoneRifle @shunkica

Okay, again, sorry for the late response here, took me a minute to catch up with what's going on.

Here's my high level thoughts (without writing a novel like I did for that other conversation):

1. I love the getKeyInfoContent function
2. I think it offering X509Certificate functionality out of the box is a great idea
3. I think it binding KeyInfo to the X509Certificate is a road we should not go down

I know you guys have lives and that the last thing any developer wants is another meeting, but I think it would be really really beneficial if we have a discussion about this project and figure out how to tactfully cross some bridges.

If you guys don't want a meeting, I'll have to write a novel to explain why I think we should not bind KeyInfo to X509Certificate

But I'm not a complainer; I wouldn't drag my feet on this if I didn't have a reasonable solution. The gist of my solution is that we could and should knock off the two suggestions that I wrote in my novel yesterday before proceeding with anything X509Certificate:

1. Deprecate SignedXml.signingKey in favor of using SignedXml.privateKey and SignedXml.publicKey
2. Implement a Facade like lukaszmakuch suggested in this discussion

So then we could move onto X509Certificate by replacing KeyInfoProvider altogether. The totally bodacious paradigm would be to have something called X509CertificateKeyInfoBuilder with the function getKeyInfoContent implemented as you have it.

[LoneRifle]

I'll have to write a novel to explain why I think we should not bind KeyInfo to X509Certificate

@djaqua might actually be helpful, so we can read async and come to any proposed meeting with working knowledge. It also documents in some manner a critical decision we are about to make with this package.

[LoneRifle]

I think it is very clear that we have issues to address with respect to the design of this package (specifically around KeyInfo generation) before we can consider implementing support for X509 certificates.

@cjbarth - I would like to suggest that maybe we close this PR for now, and overhaul KeyInfoProvider/SignedXml. Once that is in place, we might be in a better position to add X509 support.

wdyt?

[cjbarth]

1. Deprecate SignedXml.signingKey in favor of using SignedXml.privateKey and SignedXml.publicKey

This seems reasonable. I've pushed up a change to make this privateKey and publicCert instead of signingKey and signingCert, respectively.

[cjbarth]

I would like to suggest that maybe we close this PR for now, and overhaul KeyInfoProvider/SignedXml. Once that is in place, we might be in a better position to add X509 support.

I'd rather see if we can find some middle ground with this PR. I haven't seen any concrete proposals that we can action on yet. This is the closest thing we have to something that works. The HMAC thing is the biggest outlier, and I haven't seen consensus that we want to drop support for that.

[cjbarth]

I think the next step is to make it so this class can't be instantiated and adjust the tests to match that. Feel free to push up a commit to this branch that does that. Once we do that, the concept of FileKeyInfo will be meaningless and can be removed. Users will be responsible for reading their own files. We already have helper methods to convert between PEM and DER, so that can help users.

I suggest starting with something like this and going from there:

class KeyInfo {
  constructor() {
    throw Error("This class is static; you may not instantiate it.");
  }
...

If we leave it as a class, then it still allows people to extends it should they need to which allows a 'plugable' feel to this part of the library.

[cjbarth]

The other option that was presented here, which I'm starting to like the more that I think about it, is to put most of the new stuff in KeyInfo into a utility class and then add two properties to SignXml: getKeyInfoContent and getCertFromKeyInfo. We can provide defaults for these properties, but still allow others to override them. They they can decide what they want their own cert to do when validating. Thoughts?

[cjbarth]

I can revert this last commit if you all don't like the direction, but I think this really simplifies things. I have a few tests to review after this bulk refactor; I'm not sure why they are failing. Let me know what you think of this direction.

[djaqua]

@cjbarth & @LoneRifle

The more I thought about it, the more I realized that I don't really need to write a novel to explain, so let me attempt to do so concisely:

X509 is one of potentially many paradigms that XML digital signatures can work with. Another one right off the top of my head is the SecurityTokenReference element (a child of KeyInfo) in conjunction with a BinarySecurityToken from the WS-Security specs.

I would not default to using X509 because the default behavior of the current implementation is to skip building a KeyInfo element altogether (SignedXml.prototype.getKeyInfo). If we default to building an X509 thingamajig, that could purposelessly screw up clients who may have written tests to validate their XML against their own schemas. I do understand that this is an inherently breaking change so their stuff is going to break anyways, but we should give clients the option to skip building a KeyInfo element since its not strictly required by the schema.

As a reasonable best-of-all-worlds solution, I would like to propose the following (and see the big NOTE below):

• define a KeyInfoStrategy class that does nothing by default, and default to using this when a client doesn't specify otherwise
• define an X509KeyInfoStrategy class that builds the X509 stuff; make clients manually choose this

My main hesitation to get onboard with these high quality changes (they really are -- I'm not here to knock the code or the idea at all) is that, learning from our experience with KeyInfoProvider, we would do well to be very annoyingly thoughtful about the consequences of our decisions around the complexity-inducing KeyInfo element. Some things to be annoyingly thoughtful about:

• method signatures (i.e. function parameter lists) -- these should be implementation-agnostic
• method parameter and return types -- all strings? all xpath objects (e.g. Node, Element, etc)? combinations thereof?
• method names -- I love getCertFromKeyInfo but I would prefer createKeyInfoContent to getKeyInfoContent (see the NOTE below)

That's pretty much my thoughts, here. I'm neither a dictator nor in charge of this project, so if there is disagreement about these concerns, lets continue the discussion 😃

NOTE
I am very strongly opinionated about naming conventions. I welcome discussions and I'm open to alternative suggestions, so if anyone else feels strongly about such things, lets have the discussions 😄 , otherwise, if it's all the same to y'all, lets just do it my way 😈 😆

[djaqua]

And yes I am aware that I said "concisely" and proceeded to write a novel anyway 😅

[cjbarth]

I've fixed the tests and taken your naming suggestion @djaqua.

I'd like to hear more about what you have to say about method signatures. I'm inclined to pass an object with properties to those functions, thereby allowing a custom implementation to work with other values as needed. As the need arises, we can easily, and without breaking, add more properties to the object we send for use-cases we don't see yet.

[cjbarth]

@djaqua , have a look at that latest commit and let me know if that will address your concerns.

[cjbarth]

I am very strongly opinionated about naming conventions. I welcome discussions and I'm open to alternative suggestions, so if anyone else feels strongly about such things, lets have the discussions 😄 , otherwise, if it's all the same to y'all, lets just do it my way 😈 😆

I was thinking more about this, and it would be nice to have a convention. I did some googling and came across this, which seems like a well-liked (12.7k stars) opinion. @djaqua , do you have a different reference you use? If not, I propose to avoid bike-shedding and use this.

[cjbarth]

@djaqua @LoneRifle @shunkica , I really appreciate your feedback on this. I think I'm ready to land this PR if it doesn't have any objections from you all. I'd really appreciate it if someone could take a look at this, perhaps after it lands, and make sure the documentation, along with a little note about the changes, is updated. It would also be good to include a documentation note about the naming convention that seems worth following here. If @markstos agrees, I'd actually like to roll that out to node-saml and passport-saml as well so that there can be consistency in the organization.

There are a few other things that I'd like to get done in this major release before cutting it, like upgrading the code to not use var and clear up other linting warnings. I'd really like to get a major release out so that we can include it in the next node-saml and passport-saml major releases which are starting to get over-due; we try to release them about the time that NodeJS does their major release/deprecation.

[shunkica]

I have created a pull request addressing some bugs and issues that were discussed.

[cjbarth]

Thanks to much work by @shunkica , I think we are ready to merge. Are there any final comments @LoneRifle or @djaqua ? I'll leave this open all weekend to give you a chance to look and then we'll merge it late on Monday.

I really appreciate all of your comments. I feel that we have something here that really cleans up the interface a lot, but which doesn't over-engineer this for use-cases that are unidentified.

[LoneRifle]

Let's get this one through for now, we'll figure out further changes along the way

[LoneRifle]

@cjbarth - feel free to merge!