Added X509 certificate to KeyInfo X509Data, if passed through options #36
Conversation
Updated PR with the changes.. Can you please review again.. |
|
@cjbarth, Updated the PR, please review |
|
I won't be able to get to it until later this week at the earliest. |
@cjbarth, Can you please help with this PR.. |
|
I have been without my computer for more than a month and only just got it back... long story, but it has put me out of the loop, and I need to catch back up. Lots of |
Yikes! Welcome back. |
|
@ganesha289 please fix the build before I review. |
ganesha289
force-pushed
the
add-x509certificate-to-keyinfo
branch
from
472a8e6
to
a228f49
Compare
@cjbarth , I am getting same build errors for master code too. Initially it fails for "prettier-check" script, When I run "prettier-format" script, that issue get resolved, but with so many file modifications, After fixing "prettier-check" issue, 9 unit tests failing, One more issue I see is Code scanning result -> CodeQL, but that is failing for existing function crypto.ts -> removeCertPEMHeaderAndFooter Can you please help.. |
ganesha289
force-pushed
the
add-x509certificate-to-keyinfo
branch
from
1676187
to
a228f49
Compare
@cjbarth, I'm facing same issue with master code.. Can you please help.. |
|
@ganesha289 Please see the PR I made against your branch, this will fix the formatting issues and provide tests for this code change. https://github.com/ganesha289/node-saml/pull/1/files I'm guessing you didn't have prettier using the correct config file and that caused line ending changes in test files. |
@barryhagan , Thank you very much for fixing the formatting issues and adding test cases. I have approved the PR.. Merging is enabled now.. Should I proceed with merging the PR..? |
Yes, that should get your PR ready for review. |
Formatting and tests
@barryhagan , @cjbarth , Thank you for the help in issue resolution. I have merged PR - ganesha289#1 - in to this one.. Please review.. |
|
@cjbarth / @barryhagan, Can you please help with the review..? |
sorry @ganesha289, wish I could help, but I'm not a maintainer of this project. |
|
@ghsyeung , I believe the value of sending multiple certs would be so that the IdP would know both certs so it will work before and after the rotation. However, I have no need of this feature, so I don't want to be insistent. I'm not a stake-holder on this, so I don't want slow things down, however, it does seem reasonable to add this support via node-saml/xml-crypto#249. Basically, the direction there is that we shouldn't be expanding If the PR is rejected, then I'll gladly take a PR against this project per You'll not at the discussion in node-saml/xml-crypto#249 that there is no suite of tests for the |
|
@ganesha289 I'm really close to a 4.0 release, this is the last outstanding PR. Do you think you'll have time to address this in the near future so as to not hold up a 4.0 release longer than necessary? |
|
@ganesha289 I'm preparing a v5 release. Would you like to work with me to get this landed for that release? |
Sure @cjbarth |
|
@ganesha289 , did you want to start by fixing the merge conflicts and then have a look at the https://github.com/node-saml/xml-crypto project and see if we can't use some of the new features there to add this feature there and use it here. Now that we have both projects in the same organization, we can put the code where it belongs very easily. |
Conflicts: src/xml.ts
# Conflicts: # src/xml.ts
|
@ganesha289 , can you give this another look over to see if, after the merge, things look good for you? |
Description
As per issue - https://github.com/node-saml/passport-saml/issues/585 - X509Certificate is not getting added to KeyInfo of AuthnRequest.
Added fix for this issue. If signingCert (service provider's certificate) parameter is passed in options, then X509Certificate will be added to AuthnRequest.
Checklist: