Github user rsafonseca commented on the pull request:

    https://github.com/apache/cloudstack/pull/308#issuecomment-106818248
  
    Hi Rohit, thank you for your view on this :)
    Here's my comment on it:
    
    1. No use on protecting the sessionkey from MITM attacks, as the 
username/password are sent in plain text, and so are all the other commands, so 
anyone can pickup your credentials if they're listening on the wire ;). For 
MITM protection SSL should be used. I did notice that 
isSecureSessionCookieEnabled i actually placed the cookie in raw to be exactly 
the same as the sessioncookie. I did use the cookie class for reading though 
:). So.. both cookies should be inserted in the same fashion.
    
    2. Will take care of that :)
    
    3. I miss the purpose.. session cleanup? Can you explain a bit more? :)
    
    4. If you don't return it in the loginResponse, regular (non-browser) calls 
to the API will fail, because i guess all integrations with the API depend on 
it, that's why i didn't remove it. It can be in the response, as long as 
javascript doesn't store it for later access.
    
    Regarding the non-bulleted stuff:
    
    Secure cookie:
    I don't agree with this one, that will make it just not work at all in 
HTTP. We aim to fix a browser vulnerability here, not a communication channel 
one. If someone wants on the wire security, they should just switch to SSL.
    
    Not depend on sessionkey as API argument:
    Will that not break some API implementations? I think supporting both 
methods is the best for now.
    
    About SAML and redirect based auth not working:
    Are you sure? Did you test? I left it in a way it would likely work... 
cookie is still read an put into the javascript variable if it was created on 
redirection. Since the SAML plugin just puts it back in a cookie so i didn't 
bother destroying it in that scenario, the plugin should be modded for that. 
Either way.. it *should* work :)
    
    Thanks for reviewing.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to