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

Set session cookies, errors appropriately #100

Merged
merged 2 commits into from
Apr 19, 2018
Merged

Set session cookies, errors appropriately #100

merged 2 commits into from
Apr 19, 2018

Conversation

merenbach
Copy link
Contributor

@merenbach merenbach commented Apr 18, 2018

  1. Use proper Set-Cookie response header when returning cookies to HTTP clients.

  2. Fix 500 errors on invalid login (was my newbie error in returning non-gRPC errors from session service on invalid login). Now returning gRPC errors as needed.

  3. Don't do anything with gRPC headers. Simply intercept all session service requests and set a cookie based on the token from any successful response, which is going to be either a login (Create) or logout (Delete).

4. Security improvement: Don't return token as text in response if client is HTTP: the HTTP client never gets to see the token now, except in the cookie (which, being HttpOnly, cannot be accessed by JavaScript).

  1. Set Secure flag on cookie only when --insecure is false.

@merenbach merenbach added this to the 0.3.0 milestone Apr 18, 2018
@merenbach merenbach added the bug Something isn't working label Apr 18, 2018
@jessesuen
Copy link
Member

Security improvement: Don't return token as text in response if client is HTTP: the HTTP client never gets to see the token now, except in the cookie (which, being HttpOnly, cannot be accessed by JavaScript).

I don't understand this improvement to security. If it's set in the cookie, it might as well be in the response. I think having consistent behavior is important if this doesn't improve security.

@merenbach
Copy link
Contributor Author

@jessesuen on some further thought (as discussed in Slack, but here for the record), since login will likely be over JavaScript API, I've removed the HttpOnly flag, set Secure only when --insecure is false (which I should have done before), and am now returning the token.

@merenbach merenbach merged commit 80964a7 into argoproj:master Apr 19, 2018
@merenbach merenbach deleted the fix-auth-cookies branch May 7, 2018 16:59
alexec added a commit that referenced this pull request Apr 24, 2019
leoluz pushed a commit to leoluz/argo-cd that referenced this pull request Sep 29, 2023
1. Instead of global semaphore use a
per-cache semaphore. Removes thread-safety
issues, allows fine control over limiting
if multiple caches are used in a program

2. Use the semaphore to guard whole
sections that use expensive list
operations, not just the list API call.
This ensures that memory usage is capped,
not the list operations.

3. Allow to control list pager.
Reduce default prefetch limit to 1 page
from 10.

Co-authored-by: Alexander Matyushentsev <Alexander_Matyushentsev@intuit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants