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

CSRF Overhaul #2026

Merged
merged 9 commits into from Aug 30, 2018
Merged

CSRF Overhaul #2026

merged 9 commits into from Aug 30, 2018

Conversation

@jmcardon
Copy link
Member

@jmcardon jmcardon commented Aug 21, 2018

closes #1989

This changes our CSRF middleware significantly. In particular:

  • CSRF tokens are now newtyped... because passing around String everywhere was honestly giving me a goddamn headache
  • CSRF token signatures are encoded in hexadecimal strings, making it url-safe. For example: In the case of websockets, clients (if using a browser) often have no way of embedding the csrf token in the header, forcing sending in a query parameter or as part of the path.
  • CSRF Internal functions are now not private, as I've had to define my own combinators with them before. Just in case of a very special use case for whatever reason (i.e csrf token in multipart payload), the functions are exposed to be able to construct csrf actions from a specific endpoint.
  • CSRF now takes a conditional Request[G] => Boolean, which is meant to be used to check Origin headers in particular, as stated by OWASP. However, given that behind a proxy, neither Origin nor Referrer may be present, the conditional is left open-ended (with a few default constructors), for the sake of custom use cases
  • Default error status is Forbidden, given that Unauthorized (despite being correct in name), for whatever silly reason is for Authentication Errors (lol http)
@@ -7,7 +7,6 @@
</array>
</option>
<option name="sortAsScalastyle" value="true" />
<option name="USE_SCALAFMT_FORMATTER" value="true" />
Copy link
Member Author

@jmcardon jmcardon Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My editor does this EVERY SINGLE TIME AHHHHH

It must be my IJ version...

Loading

Copy link
Member

@rossabaker rossabaker Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still need to be reverted?

Loading

.exists(u =>
u.uri.host.exists(_.value == host) && u.uri.scheme.contains(sc) && u.uri.port == port)

def proxyOriginCheck[F[_]](r: Request[F], host: Host, xff: `X-Forwarded-For`): Boolean =
Copy link
Member Author

@jmcardon jmcardon Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably could use a convenience constructor?

Loading

Copy link
Member

@rossabaker rossabaker Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where's the inconvenience?

Loading

Copy link
Member Author

@jmcardon jmcardon Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean a way to create a CSRF without forcing the person to use apply and pass this around

Loading

@jmcardon
Copy link
Member Author

@jmcardon jmcardon commented Aug 23, 2018

one big note on this PR: Should CSRF actually have two parameters?

In reality, I sort of don't think so. In particular, there's very liberal use of OptionT in CSRF. If F is OptionT[F, we end up with some OptionT[OptionT[F for no reason.

Loading

@@ -7,7 +7,6 @@
</array>
</option>
<option name="sortAsScalastyle" value="true" />
<option name="USE_SCALAFMT_FORMATTER" value="true" />
Copy link
Member

@rossabaker rossabaker Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still need to be reverted?

Loading

* @param data the array
* @return a hexadecimal encoded string
*/
def encodeHexString(data: Array[Byte]): String =
Copy link
Member

@rossabaker rossabaker Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've started migrating things that used to go in util into internal, so we don't promise to maintain them forever for everybody. What do you think?

Loading

Copy link
Member Author

@jmcardon jmcardon Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair compromise, I forgot internal was a thing

Loading

@@ -52,83 +53,128 @@ final class CSRF[F[_], G[_]] private[middleware] (
val headerName: String = "X-Csrf-Token",
val cookieName: String = "csrf-token",
key: SecretKey,
clock: Clock = Clock.systemUTC())(implicit F: Sync[F], G: Applicative[G]) {
clock: Clock = Clock.systemUTC(),
Copy link
Member

@rossabaker rossabaker Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird to have defaulted arguments come after non-default arguments.

I think the idiom is to be to pass the Clock implicitly, but it's new, so I'm not sure. I'm not entirely comfortable with that for non-typeclasses, but I'm not sure it's worse than a default argument.

Loading

@@ -161,28 +207,101 @@ final class CSRF[F[_], G[_]] private[middleware] (

object CSRF {

//Newtype hax
type CSRFToken <: String
Copy link
Member

@rossabaker rossabaker Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really like this better than the "opaque" encoding?

Loading

Copy link
Member Author

@jmcardon jmcardon Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IF you want the opaque encoding I can do it, I'll just do anything to stop passing around String

Loading

Copy link
Member

@rossabaker rossabaker Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the motivation, and I prefer the opaque solution.

Loading


/** Default method for constructing CSRF middleware **/
def default[F[_]: Sync, G[_]: Applicative](
headerName: String = "X-Csrf-Token",
Copy link
Member

@rossabaker rossabaker Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be CaseInsensitiveStrings?

Loading

.exists(u =>
u.uri.host.exists(_.value == host) && u.uri.scheme.contains(sc) && u.uri.port == port)

def proxyOriginCheck[F[_]](r: Request[F], host: Host, xff: `X-Forwarded-For`): Boolean =
Copy link
Member

@rossabaker rossabaker Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where's the inconvenience?

Loading


/** Extract a csrftoken, if present, from the request,
* then generate a new token signature
* @return newly refreshed toen
Copy link
Member

@rossabaker rossabaker Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

token

Loading

/** Encode a string to a Hexadecimal string representation
* Adapted from apache commons Hex.encodeHex
*/
private[http4s] final def encodeHex(data: Array[Byte]): Array[Char] = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is something people learn when they go to school.

Loading

Copy link
Member Author

@jmcardon jmcardon Aug 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've written this code more than once unfortunately to avoid pulling in a dependency just for this.

Loading

Copy link
Member

@rossabaker rossabaker left a comment

Looks solid. Just that one nitpick.

Loading


/** Embed a token into a response **/
def embedNewInCookie[M[_]: Sync](res: Response[G]): M[Response[G]] =
Copy link
Member

@rossabaker rossabaker Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

embedNewInResponseCookie, for consistency?

Loading

Copy link
Member Author

@jmcardon jmcardon Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix

Loading

*/
private[middleware] def validateOrEmbed(r: Request[G], http: Http[F, G]): F[Response[G]] =
private[middleware] def validate(r: Request[G], response: F[Response[G]])(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Odd to take a value in a context, why does this need to be F[Response[G]] rather than just a Response[G] especially as you just evaluate it inside the comprehension in flatMap on both sides of the match

Loading

Copy link
Member Author

@jmcardon jmcardon Aug 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can still fail. It's the reason from F.fromEither(extractRaw(c.content))

extractRaw not only relies on the format of the csrf token being a particular way, but also that the signature checks out. If you feed a different key, on say, app restart, this will change the signature and you wouldn't pass this check.

Loading

@rossabaker rossabaker merged commit 209c20e into http4s:master Aug 30, 2018
2 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants