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

Proposal: New API for dynamic settings, moving closer to rack #180

Closed
oreoshake opened this issue Oct 6, 2015 · 7 comments
Closed

Proposal: New API for dynamic settings, moving closer to rack #180

oreoshake opened this issue Oct 6, 2015 · 7 comments

Comments

@oreoshake
Copy link
Contributor

Thought: only x-frame-options, content-security-policy, and hpkp generally cannot be applied site wide without exception. So provide an API for managing these three directly and as an added bonus we will set all headers in Rack.

This is backwards compatible with the existing APIs for setting per-request values [1] [2], include script hashes and nonce.

These methods can be called statically from anywhere code path that runs per request. The methods will manipulate a configuration object stored in ENV. The provided Rack middleware will merge SecureHeader::header_hash into the rack headers. By default, the library will read the values out from ENV and fall back to globally configured options. This includes headers not mentioned as having dedicated APIs for changing the settings (i.e. if you really want to, modify the ENV for that config, but you really shouldn't).

# Produces: default-src 'self'; script-src 'self'; object-src 'none'
::SecureHeaders::Configuration.configure do |config|
   config.csp = {
     default_src: %w('self'),
     script_src: %w('self')
   }
   config.x_frame_options = "DENY"
   config.hpkp =  {
    :max_age => 60.days.to_i,
    :include_subdomains => true,
    :report_uri => '//example.com/uri-directive',
    :pins => [
      {:sha256 => 'abc'},
      {:sha256 => '123'}
    ]
  }
end

# Append value to the source list, override 'none' values
# Produces: default-src 'self'; script-src 'self' s3.amazaonaws.com; object-src 'self' youtube.com
append_content_security_policy_source(script_src: "s3.amazaonaws.com", object_src: %w('self' youtube.com))

# Overrides the previously set source list, override 'none' values
# Produces: default-src 'self'; script-src s3.amazaonaws.com; object-src 'self'
override_content_security_policy_directive(script_src: "s3.amazaonaws.com", object_src: %w('self'))

override_x_frame_options(SAMEORIGIN)

# Disable the header if set as a global config setting
unless made_up_method_use_hpkp?
  override_hpkp(false)
end

# Or define the hpkp inline (if the global config is nil/false)
if made_up_method_use_hpkp?
  override_hpkp {
    :max_age => 60.days.to_i,
    :include_subdomains => true,
    :report_uri => '//example.com/uri-directive',
    :pins => [
      {:sha256 => 'abc'},
      {:sha256 => '123'}
    ]
  }
end

/cc @gregose @mastahyeti @ptoomey3

@btoews
Copy link
Contributor

btoews commented Oct 7, 2015

I like the approach a lot. Does SecureHeaders store it's config as a Hash? If so, could we just deep-dup the config and put it straight in the ENV? If would simplify the API a bit if we could just do something like:

class ApplicationController
  def secure_headers_config
    env["SECURE_HEADERS_CONFIG"]
  end
end

class MyCoolController
  def my_cool_action
    secure_headers_config["CSP"]["SCRIPT_SRC"] << "somedomain.com"
    secure_headers_config["HPKP"] = {
      :max_age => 60.days.to_i,
      :include_subdomains => true,
      :report_uri => '//example.com/uri-directive',
      :pins => [
        {:sha256 => 'abc'},
        {:sha256 => '123'}
      ]
    }
  end
end

One advantage of sticking with methods for updating the headers is that we could pre-compute the default header strings at boot time and only regenerate them if the override_* method was called. This could even be taken a step further by defining the overrides and compiling the necessary headers at boot time and then only telling the middleware to use the overridden config in the controller:

::SecureHeaders::Configuration.configure do |config|
   config.csp = {
     default_src: %w('self'),
     script_src: %w('self')
   }
end

class MyCoolController
  SecureHeaders::Configuration.override("script_from_otherdomain_com") do |config|
    config.csp["script_src"] << "otherdomain.com"
  end

  def my_cool_action
    secure_headers_use_override("script_from_otherdomain_com")
  end
end

I'm not sure how costly regenerating the headers for each request is though, or how much cheaper a static approach is.

@ptoomey3
Copy link
Member

ptoomey3 commented Oct 7, 2015

I'm not sure how costly regenerating the headers for each request is though, or how much cheaper a static approach is.

Yeah, I'm not sure if matters much either way. I bet most smaller applications won't use a dynamic policy. But, those smaller applications probably won't care about the small performance hit of a per-request approach either. It seems like it would be pretty simple to support the "static by default" approach, but I don't think it needs to be a must have if we notice anything vaguely tricky/annoying about implementing it.

@oreoshake
Copy link
Contributor Author

Every policy served with secure_headers is generated dynamically, even if it's static throughout the site. It also uses about 8 before_filters. And it supports procs for CSP config values. All of this is horribly inefficient of course. I don't recall anyone mentioning it as a bottleneck.

This approach however, wouldn't even need a before_filter. I think the "static by default" strategy makes sense. Sites with highly dynamic policies will pay some price, but still less per request than a single database query :trollface:. I'm not sure if it's worth further optimization.

@btoews
Copy link
Contributor

btoews commented Oct 7, 2015

I don't recall anyone mentioning it as a bottleneck.

Do you know if Twitter or any other mega Rails apps are using this Gem? I don't imagine it would be noticeable on smaller apps, but a ton of procs and before filters could add up for us.

Sites with highly dynamic policies will pay some price, but still less per request than a single database query :trollface:. I'm not sure if it's worth further optimization.

Makes sense.

@oreoshake
Copy link
Contributor Author

Heh twitter was mostly scala by the time this came out. I don't know what the largest site using it was, but yeah, that's really not important here since we have a better approach sans procs and filters.

@oreoshake
Copy link
Contributor Author

I'm beginning to realllly like this, so much so I'm considering removing support for all other methods. It would drastically clean up the code and should work for everyone.

@oreoshake
Copy link
Contributor Author

This has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants