update waf rules to 1.5.0 version #2598
Conversation
lib/datadog/appsec/processor.rb
Outdated
| @ruleset = case ruleset_setting | ||
| when :recommended, :risky, :strict | ||
| when :recommended, :strict | ||
| JSON.parse(Datadog::AppSec::Assets.waf_rules(ruleset_setting)) | ||
| when String | ||
| JSON.parse(File.read(ruleset_setting)) |
There was a problem hiding this comment.
Done in this way, this change seems like it can cause problems when upgrading -- a customer that previously used the risky ruleset will start getting an error when they upgrade between dd-trace-rb 1.9.0 and a later version.
Should we perhaps do something along the lines of: when customers pick risky, we instead choose the closest one for them (is that recommended or strict?), and also print a warning (maybe similar to this) saying that risky is now a no-op.
This way the upgrade experience is always smooth.
There was a problem hiding this comment.
That is a great point @ivoanjo
I'll ask the security folks for best closest alternative to risky and print a warning 😄
Thank for the feedback
There was a problem hiding this comment.
That is a concern of me too.
@Taiki-San I think the removal of risky is a breaking change, which appears to run against the ruleset release SemVer 2.0 contract, WDYT?
lib/datadog/appsec/processor.rb
Outdated
| Datadog.logger.warn( | ||
| 'Ignoring user-supplied ruleset `:risky`. That configuration value has been deprecated,'\ | ||
| 'selecting `:recommended`.' | ||
| ) |
There was a problem hiding this comment.
Putting my user hat on, I think this message could be improved.
Imagine working on a rails app with a dozen other people. When you boot the app, there's a bunch of output scrolling: puma this, database that.
And then in between is this message:
WARN -- ddtrace: [ddtrace] Ignoring user-supplied ruleset
:risky. That configuration value has been deprecated,selecting:recommended.
Would you know what to do, other than go read the ddtrace sources? Or maybe grepping by risky?
Here's a suggestion:
The
:riskyApplication Security Management ruleset has been deprecated and no longer available. The:recommendedruleset will be used instead. Please remove theappsec.ruleset = :riskysetting from yourDatadog.configureblock.
I think this message improves on the current one by:
- Identifying which component this is coming from. Helps googling, talking to support, etc.
- Documenting the next steps that customers should take to make this warning go away.
There was a problem hiding this comment.
has been deprecated
I feel this is incorrect, as usually "deprecation" would mean that it went through a grace period where it was still available.
An option would be to actually deprecate risky while keeping things at 1.4.2 for at least one ddtrace release. before we remove risky and move to 1.5.0 in a subsequent version.
This way there is no risk of breakage and users of risky can move towards either recommended or strict at their own discretion with due warning.
There was a problem hiding this comment.
I'm of two minds here, but I think I'd rather have a release with the deprecation warning about risky being removed in the next version, then update to 1.5.0 + remove risky and have that released.
So essentially:
- open another PR that adds the deprecation warning
- keep this PR up as "do not merge" until a release with the warning has been published
WDYT @ivoanjo @GustavoCaso ?
|
I think this is one case where the final decision is on your side. My input is that I think these kinds of decisions should be done on a case-by-case basis, and on this specific case I think it's fine to go ahead with this PR. I think it's a plus that this PR gets customers on our current recommended configuration faster, vs having them stay for longer on an outdated version of the ruleset. |
…how a warning message
GustavoCaso
force-pushed
the
update-waf-rules-to-1.5.0
branch
from
b3465c9
to
201f82d
Compare
|
@ivoanjo Thanks so much for the feedback on the error message. Now it is much better and actionable from the point of view of a customer 👍 @lloeki I'm on the same side as Ivo. I think we should be good to ship this PR. Is less work for us to maintain and the fact that customer might be using |
What does this PR do?
Update appsec waf rules to 1.5.0 version
:recommendedand show a warning message