middle tier - implement CSRF

[grechaw]

This is a front-end requirement, not well understood yet, but not anticipated to be complex.

[popzip]

Whats the status of this issue? Which milestone? Thanks.

[laurelnaiad]

I believe we concluded to move the middle-tier CSRF support to ea3.

The webapp has the code in place for it, I've disabled it while the Java tier because it's not supported in the Java tier. I'll set to ea3 milestone.

[grechaw]

Will tackle after EA-3.

[popzip]

Setting to major so it gets looked at - expectation of customer that this can work in production. time box effort and will re-evaluate if time does not permit.

[laurelnaiad]

@grechaw to follow up: I think this was documented a while back, but here's the skinny, anyway.

The browser is prepared (or can be "re-prepared") to make a GET /v1/session prior to attempting login. For that initial GET /v1/sesson, a CSRF token is provided in the response. The browser will then "latch on" to that token and provide it in future requests via header as the extra mechanism of security. Once the CSRF token has been given to the browser, no further requests should be honored by the REST server for that session if they lack the CSRF token header (including login -- which is a PUT on that session, and other requests GETs/POSTs/etc.)

I have a TODO to resurrect this code in the browser. IIRC, it is still there and disabled. I will report back when I think it's ready to be turned back on at the browser. Will you also provide an assessment of where the middle tier stands? Tx.

[grechaw]

@laurelnaiad In the middle tier I made a branch that renables the crsf and brings all the testing up-to-date with regard to it. All of the unit tests pass, but I'm sill missing the interaction piece that I'd like in order to see the exchange in flight (in part to reverse engineer how Spring does it).

So when you've verified the browser code can be resurrected as far as you can tell, let me know how to enable/disable, and I'll pull out the browser dev tools and take a good look at why we were having issues before -- I think it came down to dispensing new tokens each request rather than using one for the session, but I really don't get it all yet (how java is behaving)

[laurelnaiad]

I will do that. In the meantime, a question -- are your tests testing at the REST level? In past attempts, the Java code tests were passing but at the REST level we had (permissions?) errors that prevented endpoints from being used after CSRF token exchange, IIRC.

[grechaw]

There's been no appreciable change to testing the REST layer -- I'm relying on Spring's tools to exercise the endpoints. Their mock HttpObjects do appear to be too lax for proper testing of token exchange, and I recognize that's a weakness of the testing harness. rest-assured would be a nice RFE enhancement -- that kind of testing in Java is still considered secondary after doing Http mocks. (I'm not defending that practice, just trying to represent for the java hordes.)

[laurelnaiad]

I think it's a pretty big gap not to have the tests going against the http layer and its code. We have gone in circles quite a bit over bugs that show up in this layer. Would be good to have that in place as soon as is feasible (written with full knowledge that it's not feasible now :smile: ).

[popzip]

Is this current issue sufficient to track work on CSRF? If so i'd like to close this internal task (and get off my plate - unless something from me needed): https://github.com/marklogic/samplestack-internal/issues/36

[laurelnaiad]

Yes.

[grechaw]

I'm back taking a look here again. OK to ping you @laurelnaiad for help with browser activation of csrf exchange?

[laurelnaiad]

Lets coordinate once I have merges in?

[grechaw]

I figured it out! There is one rough edge to the implementation, but I'll be able to figure it out too at some point. The last lingering issue I have is with authentication success. The X-CSRF-TOKEN returned from the login endpoint is stale; its from the previous session. I find that redireting to GET /v1/session solves the issue completely, but it's a redirect.

forward is just buried deeper in the servlet stack -- but I think once I have a forward at the end of my login handler rather than a redirect, this will all be in good shape.

[laurelnaiad]

@grechaw can you walk me through the endpoints you're exposing and their behavior?

I think the getting of a CSRF token is a one-time thing (not repeatable for a server-side session, to retry with the same session ID should do something like a 500 error). If a token is being located which is stale, whose session does it belong to? There shouldn't be a new token generated for the same session id, this would allow CSRF attacks.... I suppose that if someone tries to "re-get" a token that a new one could be generated, but if so, the credentials should be dropped so that they need to log in again. This won't actually happy unless someone hacks the browser, so as long as the first/valid token is never re-sent, then I think we're good.

Happy path (i.e. no browser hacking):

GET /v1/session -> returns a token, associates it with cookie session id -> all further requests on this session require the token, you must use this to authenticate so the only case where the browser won't request this is for people who are guests PUT /v1/sesssion -> requires token and its related session id cookie, and valid credentials, logs in, associates with db roles DELETE /v1/session -> forgets the sessionid and token as well as credentials/db conn. association

Of course, the guest use cases should all work without any of this taking place.... no token, no GET /v1/session.

[grechaw]

What I'm seeing is very slightly different -- probably a bug. PUT /v1/session after authentication, the token changes -- and this new token is valid for the duration of the session.

From what you're saying, it seems the same token should be in use from the first GET /v1/session until logout.

Also, in my scenario, even without auth, the POST endpoints are requiring a token, so I need to look into that. A good catch.

[laurelnaiad]

Thanks!

after authentication, the token changes

Is the session being lost after GET /v1/session? That seems like a reasonable explanation for why a new token is being generated. I think that the user, after GET /v1/session, should have a real session which is (temporarily) associated with the guest role (until they PUT their credentials).

[grechaw]

Aha, I'm not completely bonkers -- this creation of new session on login is deliberate. See here

http://stackoverflow.com/questions/14176853/how-to-stop-spring-security-from-creating-a-new-session

Evidently it prevents a particular kind of attack. It looks like I can disable it, but do you think it might be preferable to leave the behavior as I'm seeing it (new session upon login)?

[grechaw]

Well, I've taken my knocks today on this one, and sunk too much time into it. the above configuration works to keep session id in place, but a new token is still generated at login time nonetheless. I believe I have located a bug in Spring Boot's integration with Spring Security whereby you cannot control the order of security filter application. At this particular moment it's looking doubtful that I can overcome this last obstacle.

I was able to customize the application of the csrf filter such that guest can use /v1/tags and /v1/search.

[laurelnaiad]

If you put the new csrf token in the response to PUT, I can get it updated in the browser.

[grechaw]

Is a 302 redirect an appropriate way to get it? That's what I had working. (And it's still a POST as before FWIW)

[grechaw]

(I realize this is all annoying -- Spring has gotten rather byzantine in its efforts to be Even More Automagical)

[laurelnaiad]

Not really. Can you include it in the response?

[laurelnaiad]

(As header) same as with GET

[grechaw]

Well the response now includes the old (!) CSRF token which is no longer valid. This is because of the hackery of adding the filter where it needs to go, when it was already configured to process requests ahead I'll see what I can do.

Not fun, breakpointing around spring boot. would that we could start over with a newerfangled framework, but thems the breaks.

[popzip]

Charles to push code to separate branch for Daphne to look at // Charles to clean it up (with #40)

[laurelnaiad]

IIRC, where the CSRF/CORS features lie in the middle-tier has two issues that are outstanding, one of which we've certainly discussed and one of which may only have been a stray comment in a chat or something.

• CORS/CSRF combination breaks
• browser can't distinguish types of bad auth

The first is relatively straightforward: Turning on CORS shouldn't change the way CSRF functions.

The second requires some explanation.

When the browser makes a request, it may or may not have tokens that the server considers to be associated with a valid session.

If the browser thiinks it has a valid session, but the server does not, then the workflow for the user should be that they are somehow returned to a known state (and possibly prompted that their session is invalid). This is different than a case where the server agrees on the token(s), but still does not permit a given action by a user.

In HTTP terms, the cases could be distinguished with status codes and messages this way:

• 401 (you're not sending any tokens, so I have no idea who you are, and so you can't have what you're requesting)
• 403 (you ARE sending valid tokens and I know who you are, but you're still not allowed to have what you're requesting)
• 400/InvalidSession (I see you are sending token/s but I don't agree with it/them)

If these codes can be lined up in the middle-tier, then the browser will have what it needs in order to react properly for the user by either:

• silently ditching the session (if the browser is trying to revive one but the server say 400/InvalidSession)
• informing the user their session is "dead" (if the 400/InvalidSession response is received in non-session-revival situations)
• informing the user that they need to log in to do whatever they're trying to do ("You must be logged-in..."), if the server gives 401
• informing the user that the operation isn't permitted for their role(s), if the server gives 403 (we don't actually have business requirements that would trigger this, but many apps would)

404/Not Found handling seems related at first glance, but is a separate issue due to the fact that when we get a 404 it might be due to permissions and it might not be. Nonetheless, any time the middle tier tries to find content for a user in a GET scenario and it isn't there, a 404 should be the result, regardless of authentication circumstances, since the middle-tier has no way of knowing whether it is a permissions or a plain old non-existent content problem.

So, as of now, I believe the server issues 401s and 403s in patterns that may not always follow the above, and has no separate concept of a 400+"Invalid Session" response.

Can we talk about this?

Tx!

P.S. Re: 403.... due to our business requirements and the way we're stopping people from doing forbidden things in the UI, we may not see 403 in the wild if the browser app hasn't been hacked. However, from the middle-tier perspective and so that the server properly protects itself, a 403 would be expected, for example, in a case where a contributor is trying to accept an answer to a question that they didn't ask, or if a contributor tries to vote on a content item a second time.

[grechaw]

Seems reasonable to me, and like I should be able to implement.
I think #1 was actually working, but when cors was turned on, the browser didn't send a token header specifically with the POST /v1/login. Another option would be for you to implement this as part of the node js work, and then I can just mimic the implementation that's working for the browser + node middle tier. I can devote some effort to this now if you want to tie up the functionality before node work, or wait for later in the cycle.

[laurelnaiad]

My preference would be to pursue this earlier than later, vis-a-vis timing re: node.js work, but if it makes more sense for you to be working on other things for a while, that's ok.

Re #1 working, are you saying that the browser was given a token in response to GET /v1/session, and then didn't use it in POST session, but that problem only happened when CORS was turned on? This seems pretty unlikely, but I suppose it's possible. It seems unlikely because AFAIK there was no browser app code changed when you enabled CORS, I just pointed the browser REST client to 8090 instead of 3000.... but of course we were trying to make changes pretty fastly/furiously that day.... so we should certainly check again.

[grechaw]

I'll get it back to where I had it previously, and can slate it earlier so complete the feature. Expectations for 8.0-2 timeframe (through April) is that I'll keep samplestack work to bugfixes and feature completion (triage pending milestone changes in SS) while I start closing semantics REST gaps

[grechaw]

Hi @laurelnaiad would you like to hold this issue now? I'm pretty sure that my branch implements our agreed CSRF and CORS at this point.

[laurelnaiad]

What does the commit with no comment do? Does that address the 400 error w/informative status issue? The commit doesn't have any information in it so I don't know whether or not we've either done what I was proposing or you're positing that for now we shoudn't?

[grechaw]

This commit ? 10f808a That was from 1/5, nothing new. -- it's one of the original add-a-security filter commit. Probably it showed up above because of a push -f The most recent commit in grechaw/csrf-w-browser-fixes does the 400 error with status issue.