Document csrf protection

[jmcarp]

Is the intention that users who want csrf protection should overwrite gothic.SetState to set a random state, add the state to the session, and verify in the callback handler? If that's right, it would be useful to document. Also, would you be interested in implementing csrf in gothic by default?

[ncantelmo]

I just added #159 to start addressing this issue.

I've also added some code to my project to replace SetState with a version that returns random strings. Not sure whether that sort of thing should go into gothic itself, but at the very least it would be good to have it in the example.

If you'd like a PR for either approach @markbates, let me know.

[ncantelmo]

Following up on my previous comment, I ran into a slight issue updating SetState because it doesn't return an error. The problem is that rand.Read from crypto/rand can return an error, so generating a cryptographically secure random string from within SetState prevents that error from being returned.

Since changing the SetState signature isn't really an option, I came up with a workaround that I believe could work within gothic.GetAuthURL without breaking the existing gothic API.

The general idea is to generate the state token before SetState is called, check for an error, and then set the new state token on the request context passed to SetState.

Here's an example:

import  "crypto/rand"

const gothStateKey = "goth.state"

gothic.SetState = func(r *http.Request) string {
  if state, ok := r.Context().Value(gothStateKey).(string); ok {
    return state
  }   
  panic("State not found on request context")
}

func getRandomString(n int) (string, error) {                              
  b := make([]byte, n)
  if _, err := rand.Read(b); err != nil {                                  
    return "", err                                                         
  }
  return base64.URLEncoding.EncodeToString(b), nil                         
}             

func providerAuth(w http.ResponseWriter, r *http.Request) { 
  // Check for existing session...

  state, err := getRandomString(stateTokenLength)
  if err != nil {
    http.Error(w, http.StatusText(code), code)
    return
  }
  ctx := context.WithValue(r.Context(), gothStateKey, state)
  gothic.BeginAuthHandler(w, r.WithContext(ctx))
}

Combining this code with the check I added in #159 should do everything a gothic user would need to generate and verify state tokens to prevent CSRF attacks. Setting the cookie.Options.HttpOnly flag as described in #160 and then the state token should also be protected from XSS attacks.

As for integrating this logic into gothic, there are a couple potential issues I can see.

First, we may not want to do the work of generating state tokens in gothic if SetState is overridden by a custom implementation. That could probably be avoided by checking for something like gothic.SetState == gothic.defaultSetState. But then there's a potential issue with gothic users expecting the goth.state context value to be set, because maybe they were just overriding it with a wrapper that lets them extract and log the state somewhere server-side.

Second, I don't think r.context is really supposed to be used this way. There are other approaches to tracking the request/state mapping, but they all seem worse. Maybe from inside gothic a closure could be used somehow to avoid the issue.

As an alternative to all this, SetState could be deprecated/become a no-op, and it could be replaced in GetAuthURL with a call to something like setStateSecure that handles everything behind the scenes (and can return an error).

[bentranter]

Closing this due to age, feel free to re-open if it's still an issue. Also, I'm comfortable with the CSRF protection as of #159.