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

ignore invalid cookies so we don't get 400 errors on servers, which i… #80

Merged
merged 4 commits into from Sep 9, 2015

Conversation

@mark-bradshaw
Copy link
Contributor

mark-bradshaw commented Sep 9, 2015

…s unexpected

Takes care of #65

@mark-bradshaw mark-bradshaw added the bug label Sep 9, 2015
@mark-bradshaw mark-bradshaw added this to the 3.0.5 milestone Sep 9, 2015
@mark-bradshaw

This comment has been minimized.

Copy link
Contributor Author

mark-bradshaw commented Sep 9, 2015

@devinivy or @nlf I'm looking for a code review, if either of you have some time. As a quick run down, yar encrypts a cookie to keep track of session data. Before this PR if you changed the password on the server used to encrypt the cookie all the client side cookies would become invalid, and Hapi would throw an error and respond with a 400. I believe this is a user fail, and so this PR causes invalid cookies to silently drop without causing a 400 response. The session comes out empty, which is unavoidable in this situation, but is infinitely preferable to a 400 fail I believe.

@nlf

This comment has been minimized.

Copy link
Member

nlf commented Sep 9, 2015

from a code standpoint, this makes sense. what i don't like is that this feels too invasive..

i personally feel like this is more of a documentation problem than anything, maybe it should just be clearly documented what happens when the encryption password is changed and some suggestions on working around it.

@hueniverse thoughts?

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Sep 9, 2015

It should match whatever I did in hapi-auth-cookie which I can't remember right now.

@mark-bradshaw

This comment has been minimized.

Copy link
Contributor Author

mark-bradshaw commented Sep 9, 2015

@nlf What do you mean by too invasive? Can you clarify?

I could address this as a documentation thing. "If you change your password then you should also add these cookieOptions to have the old session cookies drop." Which would work, but my preference would be to not have a server throwing 400 errors when the cookie password change was intended.

@hueniverse It turns out you did the same thing. :) https://github.com/hapijs/hapi-auth-cookie/blob/master/lib/index.js#L53

@nlf

This comment has been minimized.

Copy link
Member

nlf commented Sep 9, 2015

well then, consider my comment withdrawn.

with ignoreErrors on does something get logged during a failure? otherwise this feels like an avenue for abuse and no real way to identify it. i.e. a malicious user sends thousands of requests encrypting their cookie with a different key each time hoping to brute force the server's password.

@mark-bradshaw

This comment has been minimized.

Copy link
Contributor Author

mark-bradshaw commented Sep 9, 2015

I guess they could try to brute force the password that way. I'm not sure if the time involved in that as an attack vector would make it feasible, but that's a possible worry that I can document.

I know that the hapi docs indicate that on a route you can add a config option for state.failAction and set that to 'log', which is supposed to emit error events that you can catch and deal with. I have no idea if that applies to the ignoreErrors config option or not. @hueniverse Happen to know off the top of your head how that works?

One thing that would be nice is if there could be some indication on the request object that a cookie decrypt had failed. Some sort of object that could be reviewed real time, and used to mitigate an attack of that nature.

@devinivy

This comment has been minimized.

Copy link
Member

devinivy commented Sep 9, 2015

My inclination would be to use hapi defaults (which would serve a 400), then document how to make yar behave in certain ways.

hapi-auth-cookie is a much more specific use-case, which is why ignoreErrors isn't even configurable there. hapi-auth-cookie behaves this way so that the user can receive an authentication error, which is much more pertinent. It seems to me that this wouldn't work for yar.

That said, consistency across the hapi ecosystem is of high value when it makes sense.

@mark-bradshaw also– if ignoreErrors is true, then the error is truly ignored by hapi (or rather, by statehood, which means hapi doesn't know anything went awry).

@mark-bradshaw

This comment has been minimized.

Copy link
Contributor Author

mark-bradshaw commented Sep 9, 2015

Hmm... Well now I'm left with a dilemma. Nathan and Devin both felt it would be better handled with a documentation change, but hapi-auth-cookie is ignoring errors there, and I prefer not causing a mysterious 400 response. I'd rather just have session empty and restart, which seems pretty normal for a session storage solution. Since it's stored in cookies you assume that the session can be emptied for lots of reasons, and that's ok. To me this feels like the same situation.

I take Nathan's comments about brute forcing to heart, but this actually makes the attack more difficult since you don't get a nice easy 400 error back. It would be harder to tell if your password succeeded or failed.

Bottom line, I can't think of a normal situation where just ignoring the bad cookie and continuing on would cause unexpected behavior. I guess what I can do is make this a documentation change thing, and guide people toward what I think is the better setup, which is to ignoreErrors, but give them enough information to make the choice themselves.

@devinivy

This comment has been minimized.

Copy link
Member

devinivy commented Sep 9, 2015

Though I'm inclined to support that, I don't think either choice is inherently bad or wrong given that both use-cases are valid– solid docs to help people understand how the plugin will behave is the key here either way :)

@nlf

This comment has been minimized.

Copy link
Member

nlf commented Sep 9, 2015

it almost feels like we need a 'verbose' log event in hapi where a log event will trigger when a cookie is ignored.. but i digress.

i think this patch is fine, but document what it's doing and the consequences of it as clearly as you can.

@mark-bradshaw

This comment has been minimized.

Copy link
Contributor Author

mark-bradshaw commented Sep 9, 2015

Thanks gents. I appreciate the comments.

@OmniJeff

This comment has been minimized.

Copy link
Contributor

OmniJeff commented Sep 9, 2015

I think Mark said it all at the very beginning: "[Returning an empty session] is unavoidable in this situation, but is infinitely preferable to a 400 fail."

mark-bradshaw added a commit that referenced this pull request Sep 9, 2015
ignore invalid cookies so we don't get 400 errors on servers, which i…
@mark-bradshaw mark-bradshaw merged commit 05958e6 into master Sep 9, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@mark-bradshaw mark-bradshaw deleted the fix-invalid-cookie branch Sep 9, 2015
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Sep 9, 2015

@nlf how is that attach any worse with this change than just wasting decryption time on DOS?

@nlf

This comment has been minimized.

Copy link
Member

nlf commented Sep 9, 2015

with the invalid cookies not being ignored you would at least see a flood of 400 status codes in your access logs, which would give you a hint that some shenanigans were going on

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Sep 9, 2015

I think this is more of a concern for a login/auth use case and less for this. Unless it becomes a problem, I can't imagine anyone will ever bother logging these.

@nlf

This comment has been minimized.

Copy link
Member

nlf commented Sep 9, 2015

i tend to agree, it's just something to think about

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 10, 2015

This is under 3.0.5 but the fix was published under 4.0.0, and neither one is marked as breaking change and no release notes. This is not ideal.

@mark-bradshaw

This comment has been minimized.

Copy link
Contributor Author

mark-bradshaw commented Nov 12, 2015

Semver automatically marks v4 as a breaking change, and the release notes are tagged to the release:

"In v3.x, if a cookie was invalid the server would respond with a HTTP 400 error. This would happen if you ever changed your server password. Starting in v4.x we silently discard those bad cookies and start with new session cookies."

I'll update the issue and tag it to v4, which I forgot to do.

@mark-bradshaw mark-bradshaw modified the milestones: 4.0.0, 3.0.5 Nov 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.