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

SNS signature verification #567

Closed
robbiet480 opened this issue Feb 23, 2016 · 14 comments
Closed

SNS signature verification #567

robbiet480 opened this issue Feb 23, 2016 · 14 comments
Assignees
Labels
guidance Question that needs advice or information. help wanted We are asking the community to submit a PR to resolve this issue.

Comments

@robbiet480
Copy link

Hello,

I, and at least two others, have been unable to do proper SNS HTTP(S) signature verification. I have pasted my most recent attempt below. Currently, I and the others get the following error when attempting validation: crypto/rsa: verification error which is a generic Go RSA error (found here)

I am hoping that either A) someone can figure out what's wrong with the below (or any of the above implementations) and let me know so I can get it working, at which point I would be happy to clean up and then submit a PR for or B) SNS signature validation is added as a feature in aws-sdk-go (or an external library like the below linked JS and PHP versions).

Here's some documentation to assist anyone interested in helping:

Thanks so much!

type SNSData struct {
    Type             string    `json:"Type"`
    MessageId        string    `json:"MessageId"`
    TopicArn         string    `json:"TopicArn"`
    Subject          string    `json:"Subject"`
    Message          string    `json:"Message"`
    Timestamp        string `json:"Timestamp"`
    SignatureVersion string    `json:"SignatureVersion"`
    Signature        string    `json:"Signature"`
    SigningCertURL   string    `json:"SigningCertURL"`
    UnsubscribeURL   string    `json:"UnsubscribeURL"`
}

func (sns *SNSData) verifySNS() (bool, error) {
    var signStrings []string
    signStrings = append(signStrings, "Message")
    signStrings = append(signStrings, sns.Message)
    signStrings = append(signStrings, "MessageId")
    signStrings = append(signStrings, sns.MessageId)
    if sns.Subject != "" {
        signStrings = append(signStrings, "Subject")
        signStrings = append(signStrings, sns.Subject)
    }
    signStrings = append(signStrings, "Timestamp")
    signStrings = append(signStrings, sns.Timestamp)
    signStrings = append(signStrings, "TopicArn")
    signStrings = append(signStrings, sns.TopicArn)
    signStrings = append(signStrings, "Type")
    signStrings = append(signStrings, sns.Type)

    newSignString := strings.Join(signStrings, "\n")

    log.Infoln("newSignString", newSignString)

    signed, err := base64.StdEncoding.DecodeString(sns.Signature)

    if err != nil {
        log.Error("Base64 decoding error!", err)
    }

    resp, err := http.Get(sns.SigningCertURL)
    if err != nil {
        log.Error("HTTP GET error!", err)
    }
    defer resp.Body.Close()
    body, err := ioutil.ReadAll(resp.Body)
    if err != nil {
        log.Error("IOUtil error!", err)
    }
    p, _ := pem.Decode(body)
    cert, err := x509.ParseCertificate(p.Bytes)

    if err != nil {
        log.Error("Certificate parsing error!", err)
    }

    checkErr := cert.CheckSignature(x509.SHA1WithRSA, signed, []byte(newSignString))
    log.Infoln("checkErr", checkErr)

    return true, nil
}
@xibz xibz added guidance Question that needs advice or information. help wanted We are asking the community to submit a PR to resolve this issue. labels Feb 23, 2016
@xibz
Copy link
Contributor

xibz commented Feb 23, 2016

Hello @robbiet480, thank you for contacting us. Is your body an expected pem cert? Print out the URL to ensure that it is also correct. Looking at the RSA VerifyPKCS1v15 function, there are only two spots that returns that error. If the public key is incorrect in size or if the verification fails. If it has failed the verification, then more digging needs to be done.

@robbiet480
Copy link
Author

Hey @xibz I confirmed the URL is correct and when I downloaded the PEM I was able to successfully open it with OSX's Keychain Access. I confirmed that the PEM that Go downloads exactly matches what I download with Chrome. The only difference is absence of a newline character at the very end of the file, but I don't believe that would cause an issue.

@robbiet480
Copy link
Author

When looking at the algorithm map in x509, I noticed that SHA1WithRSA maps to SHA1-RSA but in the JavaScript implementation we use RSA-SHA1. I know little about this level of crypto, so maybe it's nothing but I thought i'd point it out.

@xibz
Copy link
Contributor

xibz commented Feb 23, 2016

The newline could break it, if it is included in the verifying. Any difference whatsoever in signed/signature or public key will break the verification. The SHA1WithRSA is just used to choose the correct algorithm. It has no affect on the verification besides which algorithm to choose. Have you tried including the newline to see if that corrects the problem? It may be a good idea to print the signedand newSignString as byte arrays in both PHP and Golang and see if there is any difference. Lastly, writing the same unit tests as php/js and see which tests fail. This would probably give some nice information.

@robbiet480
Copy link
Author

@xibz I did just test it with and without an extra newline but nothing changed :(. I also had Go write out the exact PEM file it's receiving and diff'ed it against the one downloaded via Chrome and found no differences.

I don't have a handy PHP setup anywhere, so I'll test the byte array suggestion in Javascript and get back to you shortly.

@robbiet480
Copy link
Author

@xibz Okay, I tested with both PHP and Javascript. I built some really simple Go tests to confirm everything that's coming in is the same when it comes out (just in case there was something weird happening like when filling the struct). All of the tests are passing for me. I don't see this being an issue in any of the above code, so everything points to something different or wrong with the way Go is doing the certificate check.

I'm considering ripping out all the Go crypto code and implementing OpenSSL directly using one of the available libraries.

Let me know if you have any other pointers on how to proceed.

@xibz
Copy link
Contributor

xibz commented Feb 24, 2016

@robbiet480, interesting that Golang is behaving that way. I am going to do some small test and see if I can figure something out. I really would like to prevent needing to use external libraries.

@robbiet480
Copy link
Author

@xibz I may have fixed it actually. Testing and retesting right now

@xibz
Copy link
Contributor

xibz commented Feb 24, 2016

@robbiet480 Awesome! I am really curious to what the issue was. Please let us know!

@robbiet480
Copy link
Author

I think that CheckSignature may have been a red herring. We should have instead been using VerifyPKCS1v15 from the rsa package directly. If we don't care about the output of CheckSignature and just use VerifyPKCS1v15 the hashes match without issue. Be warned though, my head is kind of spinning with all the acronyms and functions going around, so CheckSignature may be an important step, but it seems to me that VerifyPKCS1v15 fulfills all requirements. I realized all of this after finding this Gist.

Also note I changed from manually coding in the string generation to a method more like the PHP/JS libraries.

Here's the code I have, let me know if I got it right:

func (sns *SNSData) verifySNS() (bool, error) {
    var buffer bytes.Buffer
    signableKeys := []string{"Message", "MessageId", "Subject", "Timestamp", "TopicArn", "Type"}
    for _, key := range signableKeys {
        r := reflect.ValueOf(sns)
        f := reflect.Indirect(r).FieldByName(key)
        keyString := f.String()
        if keyString != "" {
            buffer.WriteString(key + "\n")
            buffer.WriteString(keyString + "\n")
        }
    }

    base64decodedsignature, err := base64.StdEncoding.DecodeString(sns.Signature)

    if err != nil {
        log.Errorln("Base64 decoding error!", err)
    }

    resp, err := http.Get(sns.SigningCertURL)
    if err != nil {
        log.Errorln("HTTP GET error!", err)
    }
    defer resp.Body.Close()
    // print("URL\n", sns.SigningCertURL)
    body, err := ioutil.ReadAll(resp.Body)
    if err != nil {
        log.Errorln("IOUtil error!", err)
    }

    p, _ := pem.Decode(body)
    cert, err := x509.ParseCertificate(p.Bytes)

    if err != nil {
        log.Errorln("Certificate parsing error!", err)
    }

    // This returns false for some reason
    checkErr := cert.CheckSignature(x509.SHA1WithRSA, base64decodedsignature, buffer.Bytes())
    if checkErr != nil {
        log.Println("CheckSignature error!", checkErr)
    }

    pub := cert.PublicKey.(*rsa.PublicKey)

    h := sha1.New()
    h.Write(buffer.Bytes())
    digest := h.Sum(nil)

    finalVerifyErr := rsa.VerifyPKCS1v15(pub, crypto.SHA1, digest, base64decodedsignature)
    if finalVerifyErr != nil {
        log.Println("verify:", finalVerifyErr)
        return false, finalVerifyErr
    } else {
        return true, nil
    }
}

@xibz
Copy link
Contributor

xibz commented Feb 24, 2016

Nvm, found it. Change this to this.

cert.CheckSignature(x509.SHA1WithRSA, buffer.Bytes(), base64decodedsignature)

The CheckSignature does what you were doing. Let us know if that works! Closing this. If there are anymore questions or issues, we can reopen.

@xibz xibz closed this as completed Feb 24, 2016
@robbiet480
Copy link
Author

@xibz Works for me! Thanks so much for helping out with them. I would offer to submit cleaned up code to do verification but I'm unsure where this would fit. Otherwise, i'll just build this into an external library which i'll release.

@xibz
Copy link
Contributor

xibz commented Feb 24, 2016

@robbiet480, my pleasure. Yea, I don't know if it would make sense to place this in the SDK. It may be a good idea to publish this as an external library since I can see people possibly wanting to just verify the request and not needing the SDK.

Thank you for posting detailed attempts and solutions. I can see people wanting to know how to do this!

@robbiet480
Copy link
Author

@xibz Just published first cut of the library, it's available here.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guidance Question that needs advice or information. help wanted We are asking the community to submit a PR to resolve this issue.
Projects
None yet
Development

No branches or pull requests

2 participants