cloudfoundry-incubator / admin-ui

Need new main contributor - An application for viewing Cloud Foundry metrics and operations data.
Apache License 2.0
71 stars 44 forks source link

Allow secure cookies without terminating TLS. #183

Closed jmcarp closed 6 years ago

jmcarp commented 6 years ago

Part of my goal in #182 was to allow the use of secure cookies without terminating tls within the application. I noticed that got dropped from the patch, so here's my second attempt. WDYT @rboykin ?

cfdreddbot commented 6 years ago

Hey jmcarp!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

rboykin commented 6 years ago

@jmcarp FYI. I won’t be able to handle this PR until January.

rboykin commented 6 years ago

@jmcarp Please attempt to run the admin ui with your modifications included. I cannot run with the config value of secured_client_connection set to either false or true.

rboykin commented 6 years ago

@sharms I don't need your approval for the PR before I pull it in. I have that authority. I need the PR to work before I pull it in. Thus my message above to @jmcarp

jmcarp commented 6 years ago

Updated and confirmed that the app runs locally.

rboykin commented 6 years ago

@jmcarp I tried your latest change. I notice you (inadvertently?) removed the secret from the secure_web case. I also note that the service does come up with the config value of secured_client_connection set to false, but when I hit http://localhost:8070, after logging in, the admin ui continually reloads. Something is still not correct there. I am using the latest Firefox for my browser.

jmcarp commented 6 years ago

@rboykin: are you running the app without tls? If so, you'll have to set cookie_secure to false. I'm passing the cookie secret via set :sessions {...} for simplicity, but I can restore to a separate set if you prefer.

rboykin commented 6 years ago

@jmcarp If needed to run out of the box without modification, then the default value for cookie_secure needs to be false. Also, as I stated above, you removed the secret setting from secure_web.rb

jmcarp commented 6 years ago

@rboykin: I updated to disable secure cookies by default. Also, I didn't remove cookie secrets from secure--since I'm setting the cookie secret with set :sessions ..., and since sinatra merges option hashes for multiple calls to set, secure gets the cookie secret from being set in web.

rboykin commented 6 years ago

Manually merged with the following additions:

Thanks @jmcarp for your contribution.