search

lizardsystem / lizard-auth-server

Django backend for the old SSO server
http://lizard-auth-server.readthedocs.io/
MIT License
2 stars 1 forks source link

Added credential-verification view for use by APIs #51

Closed reinout closed 8 years ago

reinout commented 8 years ago

Server-side fix for https://github.com/lizardsystem/lizard-auth-client/issues/48

reinout commented 8 years ago

@byrman, kun je ernaar kijken?

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.8%) to 79.942% when pulling 5198add899b04daa6ef08abdbdd3917728caa17a on reinout-check-password-directly into f9c5df1a95caf57bab9cb14b298ff423f30fd57a on master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.8%) to 79.942% when pulling dcc5d6c8ddf0b3e06cfae030dd88e52e1a50495a on reinout-check-password-directly into f9c5df1a95caf57bab9cb14b298ff423f30fd57a on master.

byrman commented 8 years ago

Bedankt, zal ik doen.

2016-09-12 15:04 GMT+02:00 Reinout van Rees notifications@github.com:

@byrman https://github.com/byrman, kun je ernaar kijken?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lizardsystem/lizard-auth-server/pull/51#issuecomment-246340550, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_cSX3M0AXIzPpQTZDBPzT5XScOnNbJks5qpU3sgaJpZM4J6j3Z .

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.8%) to 79.942% when pulling 1c69a3b4e95d18d228816cbe268c15d6ba3303a1 on reinout-check-password-directly into f9c5df1a95caf57bab9cb14b298ff423f30fd57a on master.

reinout commented 8 years ago

Ik heb nog een tikkie documentatie toegevoegd en nog een view/url hernoemd.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.8%) to 79.942% when pulling 484d431c20e2529be43f7fa55ee7e62a00fdc762 on reinout-check-password-directly into f9c5df1a95caf57bab9cb14b298ff423f30fd57a on master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.8%) to 79.942% when pulling be029f249c62ddf0a31eae24d3f59053328abf5e on reinout-check-password-directly into f9c5df1a95caf57bab9cb14b298ff423f30fd57a on master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.8%) to 79.942% when pulling a398191b424261e245f05508f6373e71ae524a4a on reinout-check-password-directly into f9c5df1a95caf57bab9cb14b298ff423f30fd57a on master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.8%) to 79.942% when pulling 35ebbe5a2a6e164768914440641b6f9d7ffd4338 on reinout-check-password-directly into f9c5df1a95caf57bab9cb14b298ff423f30fd57a on master.

reinout commented 8 years ago

Stapeltje pep8/pyflakes fixes toegevoegd.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.8%) to 79.934% when pulling e42963bafa9bec3a94a396d3648a355b5448329f on reinout-check-password-directly into f9c5df1a95caf57bab9cb14b298ff423f30fd57a on master.

byrman commented 8 years ago

@reinout : ik heb de boel lokaal nog niet werkend gekregen. Kan ook aan mijn configuratie liggen.

Het gaat op dit punt fout:

https://github.com/lizardsystem/lizard-auth-server/blob/reinout-check-password-directly/lizard_auth_server/views_api_v2.py#L227

        user = django_authenticate(username=form.cleaned_data.get('username'),
                                                     password=form.cleaned_data.get('password'))

Mijn cleaned_data ziet er op dit punt zo uit:

{'key': 'u2pLw5zDCGuJ9tYeezfsZCgp05G087VknbDy05V5Cn0bO98aT5elgksQzI9Hv9z2', 'exp': 1473767790, 'domain': None}

De keys 'username' en 'password' ontbreken volledig en de authenticatie mislukt dus. Wat me verder opvalt is dat 'domain' een None waarde heeft.

reinout commented 8 years ago

Domain hoeft alleen gezet te worden als het onduidelijk is, zoals bij nxt die op meerdere urls reageert. Normaliter heeft één site één url. Dus dat klopt wel.

Wat niet klopt is dat lizard-auth-client geen username/password doorgeeft. Ik heb de fix daarvoor net gepusht.

Zodra lizard-auth-client gemerged is wil ik met een docker setup daarvoor beginnen om testen makkelijker te maken, zucht.

Er zijn ook wat zaken die eigenlijk beter moeten: api URLs die in het /api/v2/ beginpunt expliciet worden genoemd, bijvoorbeeld. En de inhoud van de JWT message zou eigenlijk ook als een form gespecificeerd moeten worden, maar dan zit je met een form in een form te werken.

Ik had een JWTField voorgesteld waarin dit afgehandeld kon worden, maar @jackieleng was daar sterk op tegen omdat er dan iets dubbel zou gebeuren.

byrman commented 8 years ago

Krijgen we nu ook niet alle username/passwords in onze access logs (als base64)? Deze info zit nu immers in de parameters van de GET requests. Dat lijkt me zeer onwenselijk.

reinout commented 8 years ago

Goed punt. Het klopt dat de data geëncoded is, niet geëncrypt. En met een GET komt het in de logfile terecht.

Dan ben ik toch wat in de war gebracht door de verschillende views (die bijna allemaal ook GET ondersteunen).

Op lange termijn (=morgen) moet er toch eens django-rest-framework in. Zal ik in de tussentijd de GET op deze view maar uitzetten?

byrman commented 8 years ago

Wacht nog maar even, want ik kom daar geloof ik langs...

reinout commented 8 years ago

Run anders ook even bin/sphinx, misschien dat her en der de documentatie ook wat helpt. doc/build/html/index.html

byrman commented 8 years ago

Hier wordt een GET gedaan waar ik langskom tijdens het inloggen op onze API:

https://github.com/lizardsystem/lizard-auth-client/blob/reinout-direct-login/lizard_auth_client/client.py#L252

Als je de GET op deze view uitzet, kan ik niet meer inloggen.

reinout commented 8 years ago

Da's toch een kwestie van allebei de kanten op POST ombouwen?

byrman commented 8 years ago

Of het password encrypted in de payload stoppen. De site en de SSO-server delen een SSO_SECRET, dus decrypten aan de SSO zijde moet geen probleem zijn.

byrman commented 8 years ago

Bonus: dan kan het ook over HTTP.

reinout commented 8 years ago

decrypten: zat ik ook even aan te denken, maar ik vroeg me af hoe veilig dat was. Dan ga je met hetzelfde secret zowel encrypten als een jwt signeren: geeft dat geen mogelijkheid tot een crypto-aanval?

Aan de andere kant: laag risico. En in de oude situatie gebeurde het ook, meen ik me te herinneren.

(Niet github-gerelateerde opmerking: ik ga nu wat eerder naar huis, ik moet nog inkopen doen voor eten. Morgen ga ik weer a/d slag. Kun je de docker PR voor lizard-auth-client bekijken? Dan zit dat ook lekker in de Jenkins tests.)

byrman commented 8 years ago

Ik ben geen cryptoloog, maar zie je bezwaar niet:

Voelt een stuk beter dan de POST in plain (base64) text.

reinout commented 8 years ago

Oh, het voelt inderdaad een stuk beter dan een POST in (effectief) plain text.

Mijn cryptologisch bezwaar is dat de Duitse Enigma code gebroken kon worden omdat een letter mechanisch nooit op zichzelf kon mappen! Dus hoe meer verschillende dingen je met dezelfde key doet, hoe beter je gekraakt kan worden.

Maar.... in de praktijk is je suggestie gewoon een goede :)

byrman commented 8 years ago

JWT kan zowel met non-encrypted als met encrypted data worden gebruikt, dat is geloof ik het probleem niet. Maar misschien maken we het onszelf te moeilijk.

Wat er nu gebeurt is het volgende:

Dit lijkt een betere workflow:

Zouden we de eerste workflow wel moeten willen ondersteunen?

reinout commented 8 years ago

De tweede workflow is de normale.

De eerste heb je nodig als je via /admin of met kale http upload requests vanuit een toeleverancier loopt te werken.

byrman commented 8 years ago

Dus je wilt de eerste workflow blijven ondersteunen? Dan moeten we het password echt encrypten (of desnoods de hele payload).

Een leverancier zou natuurlijk ook eerst een token kunnen ophalen bij de SSO en dat kunnen meesturen met zijn requests, maar dat zijn ze nu niet gewend.

reinout commented 8 years ago

Password encrypten hoeft niet. Het gaat nu ook onversleuteld over de lijn als je je wachtwoord intypt. Dus we hoeven het niet moeilijker te maken dan het is.

Wat wel moet gebeuren: POST ipv GET voor dit request. Ik ga dat even fixen.

byrman commented 8 years ago

Hmmm, ik weet niet wat ik daar van vind: door er gevoelige informatie in te stoppen, gebruiken we tokens niet zoals ze zijn bedoeld. Eigenlijk hebben we dit dan nodig: https://github.com/jpadilla/pyjwt/issues/143.

reinout commented 8 years ago

We gebruiken JWT nu eigenlijk alleen om op een ondertekende manier info door te sturen. Gebruik als access token kàn, maar da's echt wat anders.

JWT vervangt nu het voordeur/achterdeur mechanisme van de SSO.

In de oude situatie ging username/password (voor de eerste workflow) via de achterdeur. Nu via de voordeur, met een handtekening. Niks mis mee dat user/pass over de lijn gaat, want dat gaat het ook bij een inlogformulier. Alleen even zorgen dat het niet als GET parameter in de access log terecht komt.

byrman commented 8 years ago

JWT voegt in deze workflow dus weinig toe (we gebruiken al end-to-end encryption)?

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.9%) to 79.95% when pulling dd3c5fd642a61b37d3b5bd83633dc3f624b95607 on reinout-check-password-directly into f9c5df1a95caf57bab9cb14b298ff423f30fd57a on master.

reinout commented 8 years ago

Wat JWT hier toevoegt is een beetje extra controle dat het verzoek van een geconfigureerde server af komt.

En een beetje consistentie met de andere api calls.

byrman commented 8 years ago

Werkt prima. Wel zie ik in de logs van de SSO-server dubbele informatie langskomen: in requests wordt een (public) key als query parameter meegestuurd, terwijl die key ook al in de payload van de JWT te vinden is:

[14/Sep/2016 11:31:31] "GET /api/v2/authenticate/?message=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJmb3JjZV9zc29fbG9naW4iOnRydWUsImRvbWFpbiI6Imh0dHA6Ly9sb2NhbGhvc3Q6ODAwMC8iLCJleHAiOjE0NzM4NDU3OTEsImtleSI6InUycEx3NXpEQ0d1Sjl0WWVlemZzWkNncDA1RzA4N1ZrbmJEeTA1VjVDbjBiTzk4YVQ1ZWxna3NRekk5SHY5ejIifQ.lQbmlQkLz7AYChhUMTqIqPTVCYP-JQiqje96gjySO3s&key=u2pLw5zDCGuJ9tYeezfsZCgp05G087VknbDy05V5Cn0bO98aT5elgksQzI9Hv9z2 HTTP/1.1" 302 0
[14/Sep/2016 11:31:31] "GET /accounts/login/?next=%2Fapi%2Fv2%2Fauthenticate%2F%3Fkey%3Du2pLw5zDCGuJ9tYeezfsZCgp05G087VknbDy05V5Cn0bO98aT5elgksQzI9Hv9z2%26message%3DeyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJmb3JjZV9zc29fbG9naW4iOnRydWUsImRvbWFpbiI6Imh0dHA6Ly9sb2NhbGhvc3Q6ODAwMC8iLCJleHAiOjE0NzM4NDU3OTEsImtleSI6InUycEx3NXpEQ0d1Sjl0WWVlemZzWkNncDA1RzA4N1ZrbmJEeTA1VjVDbjBiTzk4YVQ1ZWxna3NRekk5SHY5ejIifQ.lQbmlQkLz7AYChhUMTqIqPTVCYP-JQiqje96gjySO3s HTTP/1.1" 200 6079
[14/Sep/2016 11:33:54] "POST /accounts/login/?next=%2Fapi%2Fv2%2Fauthenticate%2F%3Fkey%3Du2pLw5zDCGuJ9tYeezfsZCgp05G087VknbDy05V5Cn0bO98aT5elgksQzI9Hv9z2%26message%3DeyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJmb3JjZV9zc29fbG9naW4iOnRydWUsImRvbWFpbiI6Imh0dHA6Ly9sb2NhbGhvc3Q6ODAwMC8iLCJleHAiOjE0NzM4NDU3OTEsImtleSI6InUycEx3NXpEQ0d1Sjl0WWVlemZzWkNncDA1RzA4N1ZrbmJEeTA1VjVDbjBiTzk4YVQ1ZWxna3NRekk5SHY5ejIifQ.lQbmlQkLz7AYChhUMTqIqPTVCYP-JQiqje96gjySO3s HTTP/1.1" 302 0
[14/Sep/2016 11:33:54] "GET /api/v2/authenticate/?key=u2pLw5zDCGuJ9tYeezfsZCgp05G087VknbDy05V5Cn0bO98aT5elgksQzI9Hv9z2&message=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJmb3JjZV9zc29fbG9naW4iOnRydWUsImRvbWFpbiI6Imh0dHA6Ly9sb2NhbGhvc3Q6ODAwMC8iLCJleHAiOjE0NzM4NDU3OTEsImtleSI6InUycEx3NXpEQ0d1Sjl0WWVlemZzWkNncDA1RzA4N1ZrbmJEeTA1VjVDbjBiTzk4YVQ1ZWxna3NRekk5SHY5ejIifQ.lQbmlQkLz7AYChhUMTqIqPTVCYP-JQiqje96gjySO3s HTTP/1.1" 302 0
reinout commented 8 years ago

Dat is een dubbeling inderdaad. Ik weet niet meer precies de reden. Volgens mij wilde @jackieleng het omdat je anders de jwt message twee keer uit moet pakken. Een keertje zonder handtekening-controle om de key uit de message te halen en vervolgens een tweede keer MET controle omdat je dan de key al hebt waarmee je de secret key op kan halen. Zoiets.

Mij lijkt het op zich prima om het dubbel te doen (dus eerst een keertje zonder, zie https://pyjwt.readthedocs.io/en/latest/usage.html#reading-the-claimset-without-validation) en daarna met verificatie.

Maar... daar moeten we eerst even over overleggen, liefst met @jackieleng erbij, die weet nog wat meer details. OK om dat in een andere PR te doen? Dan kunnen we deze en die van -client mergen.

jackieleng commented 8 years ago

Denk dat dit de reden is: https://github.com/lizardsystem/lizard-auth-server/blob/master/lizard_auth_server/forms.py#L79

Wat overigens overgenomen is van de vorige DecryptForm. Dus een soort extra nacontrole.

reinout commented 8 years ago

Op zich is nacontrole niet nodig als je alleen de key uit de message gebruikt. Die klopt dan per definitie.

Het vorige decrypt form is volgens mij iets dat echt encrypt i.p.v. alleen maar ondertekent/verifieerd. Dacht ik. Wij kunnen gewoon bij de key..

reinout commented 8 years ago

Hm, nee. Vorige decrypt form doet ongeveer hetzelfde.

Dus op zich kunnen we het nog versimpelen. In een ander pull request.

byrman commented 8 years ago

Ik heb nog iets gevonden dat niet backwards compatible is. Het lijkt mij een verbetering, maar ik meld het toch maar even.

Ik heb me verbaast over deze regel:

https://github.com/lizardsystem/lizard-auth-server/blob/reinout-check-password-directly/lizard_auth_server/models.py#L330

if self.user.is_staff:
    # staff can access any site
    return True

Iemand met staff status kan dus op iedere site inloggen. Ik zie niet in waarom dit zo zou moeten zijn. En waarom mag iemand met staff status dit wel en iemand met (alleen) superuser status dit niet?

In V2 kun je met staff status alleen niet meer inloggen op een site. Ik kan niet overzien of dit voor sommige gebruikers en/of sites gevolgen heeft.

reinout commented 8 years ago

Die "is_staff" is meer "alles en iedereen van N&S mag overal bij". Iedereen met de juiste ldap groep krijgt dat vinkje.

Maar inderdaad, we horen zelf niet overal meer bij te kunnen. En als dat toch moet dan moet het maar op een betere manier geregeld worden.