Add SCA enablement state in telemetry messages #3563
Conversation
vpellan
added
core
vpellan
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3563 +/- ##
=======================================
Coverage 98.11% 98.11%
=======================================
Files 1225 1225
Lines 72743 72783 +40
Branches 3479 3482 +3
=======================================
+ Hits 71369 71409 +40
Misses 1374 1374 ☔ View full report in Codecov by Sentry. |
vpellan
force-pushed
the
vpellan/sca-enablement
branch
3 times, most recently
from
c819632
to
ddcb89d
Compare
|
Waiting for 2.0 to be released |
| list << conf_value('appsec.enabled', config.dig('appsec', 'enabled')) if config.respond_to?('appsec') | ||
| if config.respond_to?('appsec') | ||
| list << conf_value('appsec.enabled', config.dig('appsec', 'enabled')) | ||
| list << conf_value('appsec.sca_enabled', config.dig('appsec', 'sca_enabled')) |
There was a problem hiding this comment.
Looking at the other options (lines 110 and 113 in this file), the pattern appears to be component.function.enabled, i.e. in this case it would be appsec.sca.enabled. Is the x_enabled pattern used elsewhere?
There was a problem hiding this comment.
I agree that it would be better, but the corresponding RFC (https://docs.google.com/document/d/1xTLC3UEGNooZS0YOYp3swMlAhtvVn1aa639TGxHHYvg) specifies appsec.sca_enabled in the telemetry paragraph
| @@ -13,7 +13,6 @@ module Datadog | |||
| module AppSec | |||
| module Contrib | |||
| module Rack | |||
| # Create an array of lowercased headers | |||
There was a problem hiding this comment.
Did you remove this comment because it was incorrect?
There was a problem hiding this comment.
No it is not incorrect but it doesn't add anything (it was originally saying more than that but I factorised the corresponding code, and the comment with it, which led to this not very useful comment that I forgot to delete) (#3528 (comment))
| def payload(seq_id) | ||
| { | ||
| configuration: @changes.map do |name, value| | ||
| { | ||
| name: name, | ||
| value: value, | ||
| origin: @origin, | ||
| } | ||
| end | ||
| } | ||
| { configuration: configuration(seq_id) } | ||
| end | ||
|
|
||
| def configuration(seq_id) | ||
| config = Datadog.configuration | ||
|
|
||
| res = @changes.map do |name, value| | ||
| { | ||
| name: name, | ||
| value: value, | ||
| origin: @origin, | ||
| } | ||
| end | ||
|
|
||
| unless config.dig('appsec', 'sca_enabled').nil? | ||
| res << { | ||
| name: 'appsec.sca_enabled', | ||
| value: config.appsec.sca_enabled, | ||
| origin: 'code', | ||
| seq_id: seq_id, | ||
| } | ||
| end | ||
|
|
||
| res |
There was a problem hiding this comment.
This payload is only used for remote configuration changes today.
Is DD_APPSEC_SCA_ENABLED remotely configurable?
I asked because this method should have no product specific code, everything should come through the changes argument to AppClientConfigurationChange.new(changes, origin).
There was a problem hiding this comment.
According to this document (https://docs.google.com/document/d/12NBx-nD-IoQEMiCRnJXneq4Be7cbtSc6pJLOFUWTpNE), DD_APPSEC_SCA_ENABLED is remotely configurable
vpellan
force-pushed
the
vpellan/sca-enablement
branch
from
cb6159c
to
63c8cde
Compare
vpellan
force-pushed
the
vpellan/sca-enablement
branch
from
edef787
to
467b49d
Compare
What does this PR do?
This PR adds SCA enablement state in configuration payloads that are sent in app-started and app-client-configuration-change Telemetry messages
Motivation:
Customers need a way to enable SCA via an environment variable. If set, this variable must be sent to the backend through Telemetry messages
How to test the change?
The tests are included in the corresponding rspec files
Link to the corresponding PR in system-tests repo
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!