appsec: add Start() and Stop() to API #2507
Conversation
eliottness
changed the title
eliottness
force-pushed
the
eliott.bouhana/public-appsec-start
branch
from
6ad9c86
to
1dd09f2
Compare
github-actions
bot
added
the
apm:ecosystem
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
eliottness
force-pushed
the
eliott.bouhana/public-appsec-start
branch
from
06cc2fd
to
33d8249
Compare
BenchmarksBenchmark execution time: 2024-01-25 16:50:37 Comparing candidate commit 33d8249 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 39 metrics, 2 unstable metrics. |
There was a problem hiding this comment.
I think we should align with the new "Go style" used in v2 for options - I think we decided to go with a simple public config struct
| // WithCodeActivation enable or disable Appsec. Can also be set using `DD_APPSEC_ENABLED`. | ||
| func WithCodeActivation(enable bool) StartOption { | ||
| return func(c *internalConfig.Config) { | ||
| c.CodeActivation = &enable | ||
| } | ||
| } |
There was a problem hiding this comment.
I propose to hide this detail and hard-code c.CodeAction = true in public_appsec.Start() when called.
| // WithCodeActivation enable or disable Appsec. Can also be set using `DD_APPSEC_ENABLED`. | |
| func WithCodeActivation(enable bool) StartOption { | |
| return func(c *internalConfig.Config) { | |
| c.CodeActivation = &enable | |
| } | |
| } |
A user should just write appsec.Start(cfg) to be able to start ASM with this option explicitly set.
| // WithRCActive enable or disable Remote Configuration for Appsec Datadog feature. | ||
| func WithRCActive(enable bool) StartOption { | ||
| return func(c *internalConfig.Config) { | ||
| c.RCEnabled = enable | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't see any usefulness for this. ASM without RC doesn't make sense, as RC brings all the protection features, and protection features are our main ASM value.
| if !tracer.IsUpAndRunning() { | ||
| tracerNotStarted.Do(func() { log.Error("appsec: tracer not started. appsec won't start. Please use tracer.Start() first") }) | ||
| return | ||
| } |
There was a problem hiding this comment.
WDYT of doing it the other way around: asking public_appsec.Start users to do it before tracer.Start so that we can just accept the first config being started with. Today's implementation restarts when private_appsec.Start is called multiple times.
appsec.Start(cfg1)
tracer.Start(cfg2)
or even simpler for this first iteration:
tracer.Start(tracer.WithAppSecConfig(cfg))
| // and enforces to have AppSec disabled. | ||
| enabled, set, err := config.IsEnabled() |
There was a problem hiding this comment.
I haven't read the full logic, but here is what's important: DD_APPSEC_ENABLED=false must always disable AppSec, no matter what else is trying to enable it. So code activation + DD_APPSEC_ENABLED=false => appsec disabled.
|
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
What does this PR do?
This PR adds
StartandStopand options to it on the appsec public API for a programmatic access to enabling ASM Threats.Motivation
We would like to have a way to enable and configure ASM directly in consul internally in Datadog. A Version 1 of this works already but requires to redeploy everything which can be slow and cannot be done by us. A way to dynamically activate ASM but more granularly than Remote Activation is required to make this happen.
Reviewer's Checklist
For Datadog employees:
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!