mreinstein / alexa-verifier

✓ Verify HTTP requests sent to an Alexa skill are sent from Amazon
MIT License
76 stars 23 forks source link

validate() looks at Common Name value instead of Subject Alternative Names section #24

Closed navzam closed 7 years ago

navzam commented 7 years ago

In validate-cert.js, this code

// check that the domain echo-api.amazon.com is present in the Subject
// Alternative Names (SANs) section of the signing certificate
if (cert.subject.getField('CN').value.indexOf('echo-api.amazon.com') === -1) {
  return 'subjectAltName Check Failed'
}

checks for the domain in the Common Name field. Shouldn't it be looking in the alt names instead? The alt names are present in cert.extensions[]

mreinstein commented 7 years ago

This is a case of the comment and error string being wrong. This is a "common name", not "alternative name" check. Would happily accept a PR that that updates the error result string and comment. 👍

mreinstein commented 7 years ago

good catch btw!

navzam commented 7 years ago

Amazon's docs for Checking the Signature of the Request say to check: "The domain echo-api.amazon.com is present in the Subject Alternative Names (SANs) section of the signing certificate." That makes me think the comment and error string are correct, but the if statement is checking the wrong field.

I guess it's working in practice b/c the CN is the same as that SAN domain, but it'd be better to match the docs IMO :)

mreinstein commented 7 years ago

it'd be better to match the docs IMO

definitely! PR welcome

mreinstein commented 7 years ago

@navzam maybe after sending a PR for this would be a good time to copy the code/tests/etc over to cortana-verifier and we can pick up from there?