Skip to content

Signature shouldn't ever be valid #3

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

Open
Nerzal opened this issue Oct 9, 2018 · 8 comments
Open

Signature shouldn't ever be valid #3

Nerzal opened this issue Oct 9, 2018 · 8 comments

Comments

@Nerzal
Copy link

Nerzal commented Oct 9, 2018

Steps to Reproduce

  1. Execute the example code

Description of the Problem

In the Process of creating the signature, the data is canonicalized etc. and in the end of that process this altered data is used to calculate the digest.
While validating the signature, you'd take this digest and the data you have and calculate the digest on your own.

So whats the Problem?
The Method CreateSignature only returns an signature object, but not the canonicalized and transformed, etc. xml data. So the data in the signature and the data we use will never match.

When does the Problem apply?
The Problem applies mostly when using enveloped signature algorithm (http://www.w3.org/2000/09/xmldsig#enveloped-signature)

	canonData, id, err := canonicalize(data)
	if err != nil {
		return nil, err
	}
	if id != "" {
		signature.SignedInfo.Reference.URI = "#" + id
	}

	//signature.CanonicalizedInput = string(canonData)
	// calculate the digest
	digest := s.digest(canonData) // <-- We calculate the digest on the canonicalized data, but we never return this data to the user
	signature.SignedInfo.Reference.DigestValue = digest
	// canonicalize the SignedInfo
	canonData, _, err = canonicalize(signature.SignedInfo)
	if err != nil {
		return nil, err
	}

	sig, err := s.Sign(canonData)
	if err != nil {
		return nil, err
	}
	signature.SignatureValue = sig
`
@RichardLindhout
Copy link

We can confirm this issue.

@Nerzal
Copy link
Author

Nerzal commented Oct 11, 2018

I may create a PR the next days. But first i need to separate that fix from all the other bad things i've done to the code in the forked repo.

Update: I will provide an PR at the 20th October. I'll use a hackathon to clean up my repository and provide the fix, as well as some other improvements and extensions to this.
You could use the fix from my fork till then. I added a the canonicalized string to the Signature struct.

@amdonov
Copy link
Owner

amdonov commented Oct 12, 2018

@Nerzal, I don't understand your issue, and I'm afraid that you may be misusing the library. It definitely creates valid signatures for my use cases so "never match" definitely isn't true.
In general, signing code does not need to return the canonicalized XML. Code validating the signature will perform its own canonicalization and arrive at the same digest. This library does not support validation, and cannot perform canonicalization on arbitrary XML. Perhaps that where you're hitting a problem? I'd recommend that you take a look at https://github.com/russellhaering/goxmldsig before spending much time on a pull request for this library. It's more widely used and already supports validation.

@Nerzal
Copy link
Author

Nerzal commented Oct 12, 2018

@amdonov i'll try to clarify the problem:

The library does create a valid signature for the canonicalized XML. That's true.

The Problem is, the library does not return the canonicalized XML in any form.

So there are 2 possibilities:

  1. Canonicalize the XML a second time, after creating the Signature
  2. Return the canonicalized XML

Why do i think, that this is necessary?
The SOAP API i talk to says my XML is not correctly signed. So i wrote an validator, which comes to the same conclusion.

What does my validator do?
It calculates the digest of the message body and compares it to the digest embedded in the signature.

The digest does not match, when signing the xml with this library and using the "encoding/xml" marshaller to marshal the body.

Why is that so?
The "encoding/xml" marshaller does not canonicalize the xml, therefor the calculated digest of the message body does not match the digest.

I hope that clearifies the Problem.
So if you think i'm using this library wrong, please tell me.
If you think, my assumption is correct, that the lib generates a valid signature, but it needs to provide the canonicalized XML beside the Signature object, you can tell me how you'd imagine i should implement it, and i'd be happy to implement it that way.

Btw: with some changes the XML canonicalization of all XML that i have works pretty well :)
Edit: Maybe the title of the Issue is misleading. I'm sorry for that

@amdonov
Copy link
Owner

amdonov commented Oct 13, 2018

If possible, please post a snippet of code where you create your signature and marshal the resulting struct to XML. I'll take a look. For a straightforward real world example of the library in use see, https://github.com/amdonov/lite-idp/blob/master/sp/metadata.go. It generates and signs a SAML metadata XML element.

@renathoaz
Copy link

Hi there, I really liked the way your lib works. But I'm stumbled upon the same problem that @Nerzal. The web service that I'm talking to, says that signature doesn't match. Probably what's happening is what Nerval mentioned. will it have some PR to fix it?

@amdonov
Copy link
Owner

amdonov commented Dec 27, 2018

@RenathoAzevedo, same comment that I provided to @Nerzal. If you post your code, I'll take a look at it.

@rodrigorodriguescosta
Copy link

Hi there, I really liked the way your lib works. But I'm stumbled upon the same problem that @Nerzal. The web service that I'm talking to, says that signature doesn't match. Probably what's happening is what Nerval mentioned. will it have some PR to fix it?

hey @renathoaz , are you using that lib to sign XML of NFE? I'm trying to do the same, did you make it work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants