[Feature] Enhance Security Middleware

[someone1]

I think there may be room to increase security of Hydra with some simple headers. I suggest possibly integrating https://github.com/unrolled/secure as a middleware. Some suggested headers to allow configuration for:

• HSTS headers
• XSS Filter
• Content Type No Sniff
• CSP Header - possibly dynamic like CORS to limit the frame-ancestors option to only whitelisted domains (and maybe X-Frame-Options as well?)
• Replace current TLS enforcement (here and here)
• Cache headers (specifically for the /.well-known/ path)?

[aeneasr]

Interesting, that could indeed be an easy win. It would be great to initialize this with some "works out-of-the-box" configuration, like setting allowed domains to IssuerURL.

[someone1]

Word of caution: The AllowedHosts setting killed hours of debugging for me - my health checks weren't passing in a Host header (or rather, a value of '-') which was being rejected.

[aeneasr]

That's the trade off for these headers. Browsers are typically fine with implementing them correctly, but other software - such as LBs, client libs, sdks, sometimes even proxies - do not always adhere to those as most HTTP servers are very forgiving wrt to headers.

I am thus also a bit hesitant on my decision. On the one hand, we do have TLS and CORS support in the server, but is it really supposed to be there? Shouldn't that be something a global proxy handles for all of the application? Then again, some custom logic is required, for example for client-specific cors handling.

If we add every feature above (and make it configurable) to the stack, we will end up with a ton of options for configuration. I always knew that this would happen in the end but I really want to avoid configuration nightmares that are e.g. apache configs.

Any ideas?

[someone1]

I think we should put some easy-win security headers in hydra - given that hydra's function is explicit to a certain purpose and is supposed to be hardened, some headers such as the XSS, No Sniff, CSP (dynamic for frame-ancestors?) should be set without question (e.g. not user configurable). I think adding a HSTS option should be quick/useful given there is already so many controls around TLS (I'd suggest to make this automatic as well but it can have lasting effects on a users domain that may be unintended).

CORS in hydra needs to be revisited in general - we can leave that for the other ticket.

Users don't always have control over load balancer's/reverse proxies that sit in front of hydra, or may have to setup one for adding such headers. Overall, I think if hydra is billed as a hardened security appliance it should put some default, hard-coded headers in place as appropriate for its function, leaving the rest to user's where deployment specifics may warrant tweaks (e.g. forwarding TLS requests over HTTP, CORS, HSTS, etc.)

With cobra/viper in hydra, config can be as easy as a YAML file - though I know there are a lot of options already. Sensible defaults are already provided and I think it's great hydra gives advanced control over its operation for advanced deployment scenarios.

[aeneasr]

Ok, that makes sense to me. Would you be open to tackle this?

[someone1]

Yes I can tackle this. Just so we're on the same page, here is what I'd plan to do:

• Introduce github.com/unrolled/secure middleware - statically set BrowserXssFilter & ContentTypeNosniff options to true
• Introduce HSTS options to hydra to enable the feature in the new middleware - Preload, Subdomains, Seconds - or is 3 options too many? Should I give the option to pass in the header's value or just an on/off which puts the typical subdomain=false, preload=true, seconds=315360000?

Although the middleware does overlap in terms of enforcing HTTPS connections, it doesn't have a health check bypass or IP enforcement check like hydra gives options for so I think it's best to leave those as-is. I could complicate the middleware to bypass the secure middleware on health checks and still keep an IP check middleware somewhere after this one - what do you think?

As for CSP - I think this should be set statically here, but only have we introduce dynamic CSP options on a per-client basis. This can be done by auto-whitelisting redirect URIs like some providers do or introduce a CSP Referrer option to clients like there are for CORS. I can open a new issue to track this.

[aeneasr]

Introduce github.com/unrolled/secure middleware - statically set BrowserXssFilter & ContentTypeNosniff options to true

Sounds good.

Introduce HSTS options to hydra to enable the feature in the new middleware

Here is where I'm a bit confused. HSTS is a useful technology, but I don't see the use for it in Hydra. Hydra runs only in two modes - HTTPS or HTTP. If HTTPS is enabled, no HTTP can be exposed. As such, it's not possible to "accidentally" connect to HTTP instead of HTTPS in Hydra. If we do have some proxy magic (with potential TLS term / http & https) going on, wouldn't that imply that we want to solve HSTS for the whole domain in the proxy?

As for CSP - I think this should be set statically here, but only have we introduce dynamic CSP options on a per-client basis. This can be done by auto-whitelisting redirect URIs like some providers do or introduce a CSP Referrer option to clients like there are for CORS. I can open a new issue to track this.

Per-client CORS is already enabled, it's the origin attribute.

[someone1]

HSTS will ensure no MITM attacks are possible - e.g. ALWAYS disallow HTTP at the client level, it doesn't matter if hydra is configured to only accept HTTPS, but HSTS explicitly forbids HTTP. This will make it so a client (e.g. a web browser) won't accidentally connect to a HTTP server if a MITM attack occurs.

Not all Load Balancers support adding headers on responses (e.g. Google's cloud load balancer) so adding the option in hydra would make sense. It's up to the user to understand the ramifications of enabling this feature if the domain is shared with other services - I personally deploy hydra in a dedicated host with a dedicated subdomain.

Re CORS vs CSP: I understand Per-client CORS is already enabled, I was asking if we should do the same for CSP headers. My idea is to explicitly disallow everything via CSP by default and only enable domains on a per-client basis as we do CORS.

[aeneasr]

Not all Load Balancers support adding headers on responses (e.g. Google's cloud load balancer) so adding the option in hydra would make sense. It's up to the user to understand the ramifications of enabling this feature if the domain is shared with other services - I personally deploy hydra in a dedicated host with a dedicated subdomain.

Ok, let's add it then. However, will this be disabled in https / ALLOW_TLS_TERMINATION_FROM mode? Or should we deploy this independently from that? In either case, I think configuring this with env vars is an option.

Re CORS vs CSP: I understand Per-client CORS is already enabled, I was asking if we should do the same for CSP headers. My idea is to explicitly disallow everything via CSP by default and only enable domains on a per-client basis as we do CORS.

What attack surface does hydra offer for CSP-relevant issues? The only endpoint that could potentially be vulnerable is the /oauth2/auth endpoint which is accessed through the browser. Assuming there would be a bug where you could set /oauth2/auth?foo=<script>alert('xss')</script> CSP could be a valuable mitigation. For all other endpoints I see little use as they return JSON only and are (usually) not accessed directly by a browser.

Therefore, because we do not actually serve any HTML, CSP could be simply default-src 'none' (if that works), indicating that no resources at all may be loaded. It seems that some new tags have been added as well, like frame-ancestors, maybe they make sense too but I haven't looked them up yet.

[someone1]

I think HSTS is independent of the other HTTP settings - regardless of whether or not hydra is running with its own cert, behind a secure proxy, or in HTTP mode; if the user enables the option(s) then it should be used. It's only possibly incorrect to enable the feature if running in HTTP-only mode so we could add a check in for that to warn the user.

I think we should still discuss the flexibility of the HSTS options though - should it be a single option with sensible defaults, a single option which passes the header through (e.g. the user passes in the header's value as an option), or break each part into separate options for the user to configure and we assemble the header in code (e.g. preload, subdomain, age).

I think we're on the same page for CSP - nothing in hydra should load anything external so by default the value you gave should be used (default src 'none'). The only exception would be when we want to try a silent refresh from an iframe, in which case we need to whitelist via the frame-ancestors option. Most providers automatically whitelist redirect URLs for both CSP/CORS but I see that in hydra this is broken out separately. Should CSP Frame Ancestors be a new client option?

[aeneasr]

if the user enables the option(s) then it should be used. It's only possibly incorrect to enable the feature if running in HTTP-only mode so we could add a check in for that to warn the user.

And disable/warn if we're running on localhost? I think sensible defaults are good for now. If people complain we can update it. I'd set a long max-age and would not include subdomains as it doesn't make sense for hydra to control this.

Most providers automatically whitelist redirect URLs for both CSP/CORS but I see that in hydra this is broken out separately. Should CSP Frame Ancestors be a new client option?

Hm I don't think it adds any value to let clients configure Frame Ancestors. If I can do bad stuff with an iframe I can bypass your CSP policy by registering a client and whitelisting the frame, right?

[aeneasr]

On second thought, I don't think restricting the frames makes a lot of sense. It's actually by design allowed. This is definitely different for the login and consent page which should set those headers as you want to disallow messing with that.

[someone1]

I think it should only be allowed on whitelisted domains? Here were my thoughts on it:

• Every request, by default, is set with the default-src 'none'; frame-ancestors 'none'; CSP header (frame-ancestors does not fallback to default-src)
• Only the /oauth2/auth handler should overwrite the header with something like default-src 'none'; when the referrer is a whitelisted URI?

Regarding the login/consent pages within the iframe - yes the developer will need to figure this out on their own. There is a section from the spec that discourages the use of iframes completely. Some providers allow it (e.g. Google) and some don't (Fitbit, Intuit). For silent refresh to work in the background, I think hydra must allow it.

Additionally, since frame-ancestors is fairly new (not supported in IE), setting X-Frame-Options may also be valuable alongside the CSP settings.

I think you have a much better understanding of all this so if you think we should just leave it be in hydra, I'd be happy to oblige - we can at least set the default CSP to default-src 'none' which does not affect frame-ancestors and would prevent any sort of external content loading in web browsers from any of the hydra page handlers.

[aeneasr]

Yeah, I think we should go with default-src 'none' for now. I am intrigued by disabling iframe all-together (due to the things from the spec and because there are always ways to break out of iframes). As I said before, I don't think it makes sense to whitelist iframes based on client redirect urls. It's not a security measure, just making it a bit harder to penetrate if client registration isn't open (which doesn't make a ton of sense for oauth2 servers). The iframe CSP could make sense for other endpoints as it would disallow inclusion of the payloads in the iframe. I'm not sure if it's a big issue but if it doesn't break anything, why not?

[aeneasr]

Regarding disabling iframe alltogether..well..openid connect is "special" here, they rely on iframes for things like silent refresh. I think FitBit is doing the right thing by disabling this. IMO silent refresh is obselete anyways because we have PKCE flows which give you refresh tokens.

[someone1]

Regarding PKCE + Silent Refresh - there is no way to secure data in the browser, and since PKCE was written with "apps" in mind - not necessarily all public clients such as a web browser, I don't think there is a good way to use refresh tokens with PKCE in the browser. That said, refresh tokens should be avoided in the browser and mechanisms such as silent refresh should instead be used IMHO.

I think it's a defense-in-depth mechanism to only allow the auth handler in an iframe from a whitelist - let the developer decide if it's ok to embed the authorization handler as an iframe, and only on trusted domains.

[aeneasr]

Regarding PKCE + Silent Refresh - there is no way to secure data in the browser, and since PKCE was written with "apps" in mind - not necessarily all public clients such as a web browser, I don't think there is a good way to use refresh tokens with PKCE in the browser. That said, refresh tokens should be avoided in the browser and mechanisms such as silent refresh should instead be used IMHO.

Point made, point taken. The question for me is if it's less secure to keep secrets in the browser (like refresh tokens) or to enable iframes (and clickjacking) in ORY Hydra. I'm not sure which one's which, but attacking one specific app that keeps the secret in the browser (using XSS to extract the refresh token) has less impact then clickjacking with an iframe which is for the whole domain. I'm really not sure what's best here.

I think it's a defense-in-depth mechanism to only allow the auth handler in an iframe from a whitelist - let the developer decide if it's ok to embed the authorization handler as an iframe, and only on trusted domains.

I think frame-ancestors gives you a false sense of security. We can enable it for all endpoints other than /oauth2/auth but it's only a cosmetic fix. For the auth endpoint it just doesn't make sense to me. Here's why:

1. We use http-only and secure cookies for csrf and auth cookies. Even if there was some way of performing XSS on the iframe (due to a browser bug and misconfigured CSP) there is nothing to be stolen. Maybe we could modify the redirect to a forged login page? Not sure if that's possible.
2. Let's say frame-ancestors blocks everything. Since we need silent refresh, OAuth2 clients would need some way of saying which iframe is ok (e.g. myapp.com). But now we're enabling issue 1 from happening. Why? Well, I'm a hacker. I want to exploit an iframe bug. I create an OAuth2 app and set the allowed url to my malicious domain. Voila, frame-ancestors is useless.

As the authorization server owner, allowing iframes globally is as secure as allowing iframes per client. I hope it's possible to follow me here.

[someone1]

I think I follow and I guess most situations are avoidable as long as the login, consent, and actual app correctly have CSP headers in place to prevent framing them. The auth handler will always redirect to one of these, all of them being whitelisted (hopefully HTTPS) destinations. I think the RFC assumes that the authorization server's /oauth2/auth page is the login/consent pages which is where clickjacking would occur, but in hydra this is not the case.

The attack also assumes that the owner of the hydra server is allowing public or otherwise non-vetted clients to register, thus allowing a malicious actor access to carry out such an act (this goes to your second point). For users of hydra where clients are more strictly created, allowing per-client CSP options may be valuable but I guess arguably not necessary.

I'll get a PR together. To re-cap:

• Introduce github.com/unrolled/secure middleware - statically set BrowserXssFilter & ContentTypeNosniff options to true
• Introduce an option to enable HSTS options to hydra to enable the feature in the new middleware - using a cautious value of subdomain=false, preload=false, seconds=315360000 (do we want to set preload=true here?)
• Every request, by default, is set with the default-src 'none'; frame-ancestors 'none'; CSP header (frame-ancestors does not fallback to default-src) except for the /oauth2/auth path which will only get the default-src 'none'; header.

[aeneasr]

Awesome! Yeah let's disable HSTS preloading for now.

[aeneasr]

Closing due to lack of public interest.

[TroutZen]

Just flagging that I hit a production use case for being able to have out of the box support for HSTS headers in case interest returns. ty!

[doubliez]

Hi, I also have a need to add HSTS headers to Hydra, so just dropping a message here to show interest for this :)