search

mozilla / identity-ops

Tools and Chef cookbooks used by Mozilla Services Operations to provision and manage Persona
Other
24 stars 12 forks source link

Test zeus logic that's been converted to nginx #31

Closed gene1wood closed 11 years ago

gene1wood commented 11 years ago

Tester https://github.com/fmarier/checkurl/blob/no_security_improvements/check-persona-url.py

Possible concerns

gene1wood commented 11 years ago

Ok, all tests passed (that I think are supposed to pass) here are the results and my notes on the tests which failed but I think are supposed to fail @fmarier may want to confirm this

GET  http://diresworb.org/                               301 https://login.anosrep.org/
GET  http://diresworb.org/about                          301 https://login.anosrep.org/about
GET  http://www.diresworb.org/                           301 https://login.anosrep.org/
GET  http://www.diresworb.org/about                      301 https://login.anosrep.org/about
GET  http://anosrep.org/                                 301 https://login.anosrep.org/
GET  http://anosrep.org/about                            404 
GET  http://www.anosrep.org/                             301 https://login.anosrep.org/
  ERROR: Wrong redirection URL: got: https://login.anosrep.org/, expected: https://anosrep.org/
  (This is ok since the tool expects a sequence of 2 redirects and I'm skipping to the end)
GET  http://www.anosrep.org/about                        404 
GET  http://verifier.login.anosrep.org/                  301 https://login.anosrep.org/
GET  http://static.login.anosrep.org/                    301 https://login.anosrep.org/
GET  http://static.login.anosrep.org/v/13baff70a3/production/communication_iframe.js  404 
GET  http://static.login.anosrep.org/v/abca29d0cb/production/en/dialog.css  404 
GET  http://static.login.anosrep.org/v/4c35c324fb/common/i/grain.png  404 
GET  http://login.anosrep.org/                           301 https://login.anosrep.org/
GET  http://login.anosrep.org/about                      301 https://login.anosrep.org/about
GET  http://static.login.anosrep.org/include.js          301 https://login.anosrep.org/include.js
GET  http://login.anosrep.org/include.js                 301 https://login.anosrep.org/include.js
GET  http://static.login.anosrep.org/include.orig.js     301 https://login.anosrep.org/include.orig.js
GET  http://login.anosrep.org/include.orig.js            301 https://login.anosrep.org/include.orig.js
GET  http://static.login.anosrep.org/authentication_api.js  301 https://login.anosrep.org/authentication_api.js
GET  http://login.anosrep.org/authentication_api.js      301 https://login.anosrep.org/authentication_api.js
GET  http://static.login.anosrep.org/provisioning_api.js  301 https://login.anosrep.org/provisioning_api.js
GET  http://login.anosrep.org/provisioning_api.js        301 https://login.anosrep.org/provisioning_api.js
GET  https://diresworb.org/                              301 https://login.anosrep.org/
GET  https://diresworb.org/about                         301 https://login.anosrep.org/about
GET  https://www.diresworb.org/                          301 https://login.anosrep.org/
GET  https://www.diresworb.org/about                     301 https://login.anosrep.org/about
GET  https://anosrep.org/                                301 https://login.anosrep.org/
GET  https://anosrep.org/about                           404 
GET  https://www.anosrep.org/                            301 https://login.anosrep.org/
  ERROR: Wrong redirection URL: got: https://login.anosrep.org/, expected: https://anosrep.org/
  (This is ok since the tool expects a sequence of 2 redirects and I'm skipping to the end)
GET  https://www.anosrep.org/about                       404 
GET  https://verifier.login.anosrep.org/                 301 https://login.anosrep.org/
GET  https://verifier.login.anosrep.org/verify           404 
  ERROR: Wrong response code: got: 404, expected: 405
  (This is a 404 in production, why should it be a 405?)
GET  https://login.anosrep.org/verify                    404 
GET  https://static.login.anosrep.org/                   301 https://login.anosrep.org/
GET  https://static.login.anosrep.org/v/13baff70a3/production/communication_iframe.js  200 
GET  https://static.login.anosrep.org/v/abca29d0cb/production/en/dialog.css  200 
GET  https://static.login.anosrep.org/v/4c35c324fb/common/i/grain.png  200 
GET  https://login.anosrep.org/                          200 
GET  https://login.anosrep.org/about                     200 
GET  https://anosrep.org/include.js                      301 https://login.anosrep.org/include.js
GET  https://static.login.anosrep.org/include.js         301 https://login.anosrep.org/include.js
GET  https://verifier.login.anosrep.org/include.js       301 https://login.anosrep.org/include.js
GET  https://login.anosrep.org/include.js                200 
GET  https://anosrep.org/include.orig.js                 301 https://login.anosrep.org/include.orig.js
GET  https://static.login.anosrep.org/include.orig.js    301 https://login.anosrep.org/include.orig.js
GET  https://verifier.login.anosrep.org/include.orig.js  301 https://login.anosrep.org/include.orig.js
GET  https://login.anosrep.org/include.orig.js           404 
  ERROR: Wrong response code: got: 404, expected: 200
  (This is a 404 in production, why should it be a 200?)
GET  https://anosrep.org/authentication_api.js           301 https://login.anosrep.org/authentication_api.js
GET  https://static.login.anosrep.org/authentication_api.js  301 https://login.anosrep.org/authentication_api.js
GET  https://verifier.login.anosrep.org/authentication_api.js  301 https://login.anosrep.org/authentication_api.js
GET  https://login.anosrep.org/authentication_api.js     200 
GET  https://anosrep.org/provisioning_api.js             301 https://login.anosrep.org/provisioning_api.js
GET  https://static.login.anosrep.org/provisioning_api.js  301 https://login.anosrep.org/provisioning_api.js
GET  https://verifier.login.anosrep.org/provisioning_api.js  301 https://login.anosrep.org/provisioning_api.js
GET  https://login.anosrep.org/provisioning_api.js       200 
POST http://diresworb.org/verify                         400 
  ERROR: Wrong response code: got: 400, expected: 404
  (These should be 400's not 404, see ( https://github.com/fmarier/checkurl/blob/no_security_improvements/check-persona-url.py#L210 ))
POST http://www.diresworb.org/verify                     400 
  ERROR: Wrong response code: got: 400, expected: 404
  (These should be 400's not 404, see ( https://github.com/fmarier/checkurl/blob/no_security_improvements/check-persona-url.py#L210 ))
POST http://anosrep.org/verify                           400 
  ERROR: Wrong response code: got: 400, expected: 404
  (These should be 400's not 404, see ( https://github.com/fmarier/checkurl/blob/no_security_improvements/check-persona-url.py#L210 ))
POST http://www.anosrep.org/verify                       400 
  ERROR: Wrong response code: got: 400, expected: 404
  (These should be 400's not 404, see ( https://github.com/fmarier/checkurl/blob/no_security_improvements/check-persona-url.py#L210 ))
POST http://verifier.login.anosrep.org/verify            400 
  ERROR: Wrong response code: got: 400, expected: 404
  (These should be 400's not 404, see ( https://github.com/fmarier/checkurl/blob/no_security_improvements/check-persona-url.py#L210 ))
POST http://static.login.anosrep.org/verify              400 
  ERROR: Wrong response code: got: 400, expected: 404
  (These should be 400's not 404, see ( https://github.com/fmarier/checkurl/blob/no_security_improvements/check-persona-url.py#L210 ))
POST http://login.anosrep.org/verify                     400 
  ERROR: Wrong response code: got: 400, expected: 404
  (These should be 400's not 404, see ( https://github.com/fmarier/checkurl/blob/no_security_improvements/check-persona-url.py#L210 ))
POST http://login.anosrep.org/                           400 
  ERROR: Wrong response code: got: 400, expected: 405
  (These should be 400's not 405, see ( https://github.com/fmarier/checkurl/blob/no_security_improvements/check-persona-url.py#L210 ))
POST https://diresworb.org/verify                        200 
POST https://www.diresworb.org/verify                    200 
POST https://anosrep.org/verify                          200 
POST https://www.anosrep.org/verify                      200 
POST https://verifier.login.anosrep.org/verify           200 
POST https://verifier.login.anosrep.org/                 400 
  ERROR: Wrong response code: got: 400, expected: 404
  ERROR: wrong response: got non conforming json response: {"error": "This is not a valid URL to POST to"}
  (I think we should go with 400 here with a meaningful message instead of 404)
POST https://static.login.anosrep.org/verify             200 
POST https://login.anosrep.org/verify                    200 
POST https://login.anosrep.org/                          400 
  ERROR: Wrong response code: got: 400, expected: 405
  (I think we should go with 400 here with a meaningful message instead of 404)
jrgm commented 11 years ago 
GET  https://login.anosrep.org/include.orig.js           404 
  ERROR: Wrong response code: got: 404, expected: 200
  (This is a 404 in production, why should it be a 200?)

That's https://github.com/mozilla/browserid/issues/3212. It's a bug that that file is not there in train-2013.03.29.

fmarier commented 11 years ago 
GET  http://www.anosrep.org/                             301 https://login.anosrep.org/
  ERROR: Wrong redirection URL: got: https://login.anosrep.org/, expected: https://anosrep.org/
  (This is ok since the tool expects a sequence of 2 redirects and I'm skipping to the end)
GET  https://www.anosrep.org/                            301 https://login.anosrep.org/
  ERROR: Wrong redirection URL: got: https://login.anosrep.org/, expected: https://anosrep.org/
  (This is ok since the tool expects a sequence of 2 redirects and I'm skipping to the end)

Do you mean that there is in fact a sequence of 2 redirects but the tool doesn't see it? (I believe the reason behind that was that we might have a non-login landing page on http://persona.org in the future and so the second redirect would be removed.)

GET  https://verifier.login.anosrep.org/verify           404 
  ERROR: Wrong response code: got: 404, expected: 405
  (This is a 404 in production, why should it be a 405?)

405 is a more accurate error code in this case. The URL is fine, it's the method (GET) that isn't.

POST http://diresworb.org/verify                         400 
  ERROR: Wrong response code: got: 400, expected: 404
  (These should be 400's not 404, see ( https://github.com/fmarier/checkurl/blob/no_security_improvements/check-persona-url.py#L210 ))
....
POST http://login.anosrep.org/                           400 
  ERROR: Wrong response code: got: 400, expected: 405
  (These should be 400's not 405, see ( https://github.com/fmarier/checkurl/blob/no_security_improvements/check-persona-url.py#L210 ))

I think that comment in the script is wrong. The 404 or 405 error codes are more specific. Any thoughts @jrgm ?

POST https://verifier.login.anosrep.org/                 400 
    ERROR: Wrong response code: got: 400, expected: 404
    ERROR: wrong response: got non conforming json response: {"error": "This is not a valid URL to POST to"}
    (I think we should go with 400 here with a meaningful message instead of 404)

Fair enough, I'll update the script.

POST https://login.anosrep.org/                          400 
  ERROR: Wrong response code: got: 400, expected: 405
  (I think we should go with 400 here with a meaningful message instead of 404)

I'm not sure we need an error message here. This is so far from the right URL for the verifier that people can't expect this to work with a POST. On the other hand, doing a GET on that URL is perfectly fine. So 405 seems like the right error code.

jrgm commented 11 years ago 
GET  https://verifier.login.anosrep.org/verify           404 
  ERROR: Wrong response code: got: 404, expected: 405
  (This is a 404 in production, why should it be a 405?)
405 is a more accurate error code in this case. The URL is fine, it's the method (GET) that isn't.

So, it's true that a GET on that URL is 404 in production, and that 405 is a better code. But I personally don't want to put logic into nginx where it could/should be done in the browserid repo code. POST for /verify is implemented here: https://github.com/mozilla/browserid/blob/dev/bin/verifier#L154.

jrgm commented 11 years ago

Actually, settling this with github comments seems not the most effective way to do this. Can we agree to move this to an etherpad for rapid iteration?

fmarier commented 11 years ago

I agree with the desire to avoid patching in nginx what should be done in the code. In the above 404 v. 405, should we just file a (very low priority) bug against the browserid codebase?

+1 on the etherpad idea. I assume what you'd like to put in the etherpad is the output that gene pasted?

gene1wood commented 11 years ago

@fmarier Can you provide a link to the updated script code so I can retest?

fmarier commented 11 years ago

Updated test script here: https://github.com/fmarier/checkurl/tree/no_security_improvements

The remaining issues to fix are:

GET  http://www.anosrep.org/                             301 https://login.anosrep.org/
  ERROR: Wrong redirection URL: got: https://login.anosrep.org/, expected: https://anosrep.org/

301 to the base domain name over https and then let that redirect to login.persona.org.

GET  http://verifier.login.anosrep.org/include.js        301 https://login.anosrep.org/
  ERROR: Wrong response code: got: 301, expected: 404

Right now, it's 400 on prod. It should be a 4xx, ideally 404.

GET  https://www.anosrep.org/                            301 https://login.anosrep.org/
  ERROR: Wrong redirection URL: got: https://login.anosrep.org/, expected: https://anosrep.org/

301 to the base domain name and then let that redirect to login.persona.org.

GET  https://login.anosrep.org/include.orig.js           404 
  ERROR: Wrong response code: got: 404, expected: 200

Known issue, so no need to fix this one on your end.

gene1wood commented 11 years ago

@fmarier Regarding these 2 cases :

GET  http://www.anosrep.org/                             301 https://login.anosrep.org/
  ERROR: Wrong redirection URL: got: https://login.anosrep.org/, expected: https://anosrep.org/

301 to the base domain name over https and then let that redirect to login.persona.org.

and

GET  https://www.anosrep.org/                            301 https://login.anosrep.org/
  ERROR: Wrong redirection URL: got: https://login.anosrep.org/, expected: https://anosrep.org/

301 to the base domain name and then let that redirect to login.persona.org.

Do you really prefer to have multiple redirects instead of a single redirect to the right location? I only ask because multiple chained redirects are slower and they both accomplish the same thing. Previously we had chained redirects as a side effect of shortcuts in the redirection logic which are now made explicit.

If you'd like to have the chained redirects, not a problem, I can do that, just wanted to give you one last chance to change your mind. Let me know and I'll either make the change or leave it today.

gene1wood commented 11 years ago

Regarding :

GET  http://verifier.login.anosrep.org/include.js        301 https://login.anosrep.org/
  ERROR: Wrong response code: got: 301, expected: 404

Right now, it's 400 on prod. It should be a 4xx, ideally 404.

I've fixed that here https://github.com/mozilla/identity-ops/commit/8e2390b6e8ca12839777be936c90bd983322c0b2 and it'll be present in the new stack coming up

fmarier commented 11 years ago

@gene1wood The reason why the www.anosrep.org ones should have a double redirect is that the first redirect is permanent (removing the www) but the second (going from anosrep.org to login.anosrep.org) could be dropped in the future if we decide to host something else on the main domain.

So ideally: www.anosrep.org ==301==> anosrep.org ==302==> login.anosrep.org

gene1wood commented 11 years ago

Ok, I've updated this here : https://github.com/mozilla/identity-ops/commit/89d2ecede78e0a5b8c564a8a9f9b8cb95096d0cb and it'll be present in future stacks

gene1wood commented 11 years ago

I'm pretty sure we got sign off from @fmarier and @jrgm on this. Closing