-
Notifications
You must be signed in to change notification settings - Fork 375
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
[APPSEC-9341] Allow blocking response template configuration via ENV variables #2975
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments for reviewers to peruse
@lloeki here is an alternative suggestion for configuration of this feature. settings :block do
# HTTP status code to block with
option :status do |o|
o.type :int
o.default 403
end
# only applies to redirect status codes
option :location do |o|
o.type :string
o.setter { |v| URI(v) unless v.nil? }
end
# only applies to non-redirect status codes with bodies
settings :templates do
option :html do |o|
o.env 'DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML'
o.type :string
o.setter do |value|
unless File.exist?(value)
raise(ArgumentError, "appsec.templates.html: file not found: #{value}")
end
end
end
option :json do |o|
o.env 'DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON'
o.type :string
o.setter do |value|
unless File.exist?(value)
raise(ArgumentError, "appsec.templates.json: file not found: #{value}")
end
end
end
option :html do |o|
o.env 'DD_APPSEC_HTTP_BLOCKED_TEMPLATE_TEXT'
o.type :string
o.setter do |value|
unless File.exist?(value)
raise(ArgumentError, "appsec.templates.text: file not found: #{value}")
end
end
end
end
end It separates each type into its own The code for the moment in which we need to retrieve either the default template or the configured one the code could look like this: def content(content_type)
content_format = content_type.split('/')[1]).to_sym
raise ArgumentError, "unexpected type: #{content_type.inspect}" unless valid_format?(content_format)
using_default = Datadog.configuration.appsec.block.templates.using_default?(content_format)
if using_default
Datadog::AppSec::Assets.blocked(format: content_format)
else
path = Datadog.configuration.appsec.block.templates.send(content_format).to_s
cache[path] ||= (File.open(path, 'rb', &:read) || '')
end
end This new code uses What do you think 😄 ? |
Actually, @lloeki looking at the documentation on the DD site https://docs.datadoghq.com/security/application_security/threats/library_configuration/#configure-a-custom-blocking-page-or-payload The documented way to configure the blocking response is |
848c156
to
400e62f
Compare
400e62f
to
cf2b968
Compare
settings :templates do | ||
option :html do |o| | ||
o.env 'DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML' | ||
o.type :string, nilable: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to provide nilable
because we use using_default?
when checking whether we need to use the default value.
Since using_default?
computes the option, we needed to provide something that would make it work. Another solution would be to give an empty string as default.
What does this PR do?
Allow blocking response configuration:- change status code- change response body- specify redirect target (location header)Motivation
Enable users to customize attack response.Add support for configuring the block response template over configuration and ENV variables.
The customer can tweak the response body of the blocking page or default to the one store on the library.
On the first implementation that Loic created, we used the class instance variable
@cache
to store the result from reading the custom templates. I moved it into the configuration since it already provides a cache by default.Additional Notes
- Initial implementation operates via static configuration.- Then remote configuration support can be added by altering configuration fetching inAppSec::Response
- There will be a bit of refactoring when #2970 is mergedHow to test the change?
specs