Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for Cookie authentication #390

Merged
merged 1 commit into from
Nov 14, 2020
Merged

Conversation

Alevsk
Copy link
Contributor

@Alevsk Alevsk commented Nov 12, 2020

  • Added support for cookie authentication (authorization header authentication still exists and will have priority)
  • Removed local storage token management from UI
  • Cookie hardening (sameSite, httpOnly, secure), leverage on browser existing protections
  • Login endpoint sets cookie via header, logout endpoint expires cookie

Future improvements

  • Look for all places in backend that returns 401 unauthorized, and destroy session there (not a priority since cookie its invalid anyway)
  • Downloading objects in object browser can be simplified since is just a GET request and users will be authenticated via Cookies, no need to craft additional requests

@Alevsk Alevsk self-assigned this Nov 12, 2020
@Alevsk Alevsk linked an issue Nov 12, 2020 that may be closed by this pull request
@dvaldivia dvaldivia added this to Review in progress in Console Version 1 via automation Nov 12, 2020
@harshavardhana
Copy link
Member

why are we doing cookie based?

dvaldivia
dvaldivia previously approved these changes Nov 12, 2020
@Alevsk
Copy link
Contributor Author

Alevsk commented Nov 12, 2020

why are we doing cookie based?

Console is a browser application, there's no need to the use localStorage (we still maintain the support for clients that doesn't support cookies), instead we can use browser cookies and leverage on the existing security protections, right now the session token it's too expose and any injected malicious JS code would be able to read that information

@Alevsk
Copy link
Contributor Author

Alevsk commented Nov 12, 2020

why are we doing cookie based?

Console is a browser application, there's no need to the use localStorage (we still maintain the support for clients that doesn't support cookies), instead we can use browser cookies and leverage on the existing security protections, right now the session token it's too expose and any injected malicious JS code would be able to read that information

cookie based auth expose us to other series of threats such as CSRF but we can also fix those if ever happen, we need to add layers of security

@Alevsk Alevsk added the WIP This PR is WIP and cannot be merged yet label Nov 13, 2020
@Alevsk Alevsk changed the title Support for Cookie authentication [WIP DO NOT MERGE] Support for Cookie authentication Nov 13, 2020
@Alevsk Alevsk changed the title [WIP DO NOT MERGE] Support for Cookie authentication Support for Cookie authentication Nov 13, 2020
@Alevsk Alevsk removed the WIP This PR is WIP and cannot be merged yet label Nov 13, 2020
dvaldivia
dvaldivia previously approved these changes Nov 13, 2020
restapi/configure_console.go Show resolved Hide resolved
restapi/configure_console.go Show resolved Hide resolved
@harshavardhana
Copy link
Member

Console is a browser application, there's no need to the use localStorage (we still maintain the support for clients that doesn't support cookies), instead we can use browser cookies and leverage on the existing security protections, right now the session token it's too expose and any injected malicious JS code would be able to read that information

Almost every browser supports localStorage - Cookies are unnecessarily complicated - unless we are fixing something real.

@harshavardhana
Copy link
Member

Almost every browser supports localStorage - Cookies are unnecessarily complicated - unless we are fixing something real.

Also any injected JS cannot access localStorage because browsers make sure that new JS code is never able to read your application's localStorage.

@harshavardhana
Copy link
Member

Ideally, we should be using https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage if we want to be paranoid about it.

@Alevsk
Copy link
Contributor Author

Alevsk commented Nov 13, 2020

Ideally, we should be using https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage if we want to be paranoid about it.

I'll take a look harsha

@Alevsk
Copy link
Contributor Author

Alevsk commented Nov 13, 2020

Ideally, we should be using https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage if we want to be paranoid about it.

But it's not only that, the fact that JS (our own code or a third-party library) can read our token its pretty scary, right now we are using cookies already because of websocket communication (websockets authentication works only via cookies as far as I know, @cesnietor can confirm that), so this PR will unify the session handling mechanism to use only cookies instead of cookies and authorization header.

@harshavardhana
Copy link
Member

But it's not only that, the fact that JS (our own code or a third-party library) can read our token its pretty scary, right now we are using cookies already because of websocket communication (websockets authentication works only via cookies as far as I know, @cesnietor can confirm that), so this PR will unify the session handling mechanism to use only cookies instead of cookies and authorization header.

Understood

- Added support for cookie authentication (authorization header will have priority)
- Removed local storage token management from UI
- cookie hardening (sameSite, httpOnly, secure)
- login endpoint sets cookie via header, logout endpoint expires cookie
- Refactor Routes and ProtectedRoutes components, improvement on the way
  application check if user session is valid

Future improvements

- look for all places in backend that returns 401 unauthorized, and destroy session there (not a priority since cookie its invalid anyway)
- Downloading objects in object browser can be simplified since is just a GET request and users will be authenticated via Cookies, no need to craft additional requests
Console Version 1 automation moved this from Review in progress to Reviewer approved Nov 14, 2020
@dvaldivia dvaldivia merged commit be569ae into minio:master Nov 14, 2020
Console Version 1 automation moved this from Reviewer approved to Done Nov 14, 2020
@Alevsk Alevsk deleted the cookie-session branch November 14, 2020 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Console sessions
4 participants