Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated getKeyInfo function with actual implementation #249

Merged
merged 3 commits into from
Feb 26, 2023

Conversation

ganesha289
Copy link
Contributor

@ganesha289 ganesha289 commented Jul 18, 2022

Hello @cjbarth, @LoneRifle, @yaronn,
I have added implementation for getKeyInfo function, please check if it looks good, then I'll proceed with unit tests.

I am getting confused with below line of codes,

For FileKeyInfo, I'm assuming it accepts file which will be public X509 certificate and this.getKeyInfo function will return the X509Data. But, what is the use of this.getKey()?? I'm assuming it will return public certificate. But, in some places it used as privateKey, as shown in below statements,

In node-saml->src->xml.ts->signXml, signingKey gets the privateKey value assigned, as shown below,
image

However, in xml-crypto->lib->signed-sml.js, signingKey gets the value from KeyInfoProvider.getKey(this.keyInfo) which looks to be public certificate..
image

Can you please help me to clear this..

@cjbarth
Copy link
Contributor

cjbarth commented Jul 19, 2022

@ganesha289 , I'm not sure what the confusion is. Signing should be done with a private key and verification of a signature is done with a public key. The code that you reference in signed-xml.js is part of the function called checkSignature, so it would make sense that it would use a public cert. As for node-saml, that looks like it is correctly using a private key to sign an XML document.

@LoneRifle LoneRifle force-pushed the master branch 3 times, most recently from 91dd3d5 to 7511a02 Compare July 24, 2022 09:41
lib/signed-xml.js Outdated Show resolved Hide resolved
Co-authored-by: LoneRifle <LoneRifle@users.noreply.github.com>
lib/signed-xml.js Outdated Show resolved Hide resolved
@cjbarth
Copy link
Contributor

cjbarth commented Nov 18, 2022

@ganesha289 I think we have a general idea of how we'd like to move forward. Since @LoneRifle and I are pretty much the only ones left doing anything that looks like maintenance on this project, I think the three of us can figure out the correct pattern here, implement it, and close this out. I know node-saml will benefit from it, and I expect others will too, especially if the pattern is clearly established.

Copy link
Collaborator

@LoneRifle LoneRifle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@LoneRifle LoneRifle merged commit 58263a6 into node-saml:master Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants