feat(agent): add support for csec php agent #918

anmol-ap · 2024-06-14T07:54:23Z

Changes required for security agent :
Extract account id from the connection info
Add public method to get entity guid, account id, and other metadata required by security agent to connect to SE
Handling to read the security.agent.enabled and security.enabled flags and log the same NR-276914
Add additional config required for security agent

newrelic-php-agent-bot · 2024-06-14T07:54:51Z

Test Suite	Status	Result
Multiverse	❌	2/9 passing
SOAK	❌	54/56 passing

codecov-commenter · 2024-06-14T08:19:10Z

Codecov Report

Attention: Patch coverage is 50.00000% with 23 lines in your changes missing coverage. Please review.

Project coverage is 78.52%. Comparing base (ddc5230) to head (c2f0349).
Report is 11 commits behind head on dev.

Files	Patch %	Lines
agent/csec_metadata.c	0.00%	19 Missing ⚠️
agent/php_nrini.c	75.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #918      +/-   ##
==========================================
- Coverage   78.79%   78.52%   -0.28%     
==========================================
  Files         193      194       +1     
  Lines       27214    27339     +125     
==========================================
+ Hits        21443    21467      +24     
- Misses       5771     5872     +101

Flag	Coverage Δ
agent-for-php-7.0	`77.44% <50.00%> (-0.09%)`	⬇️
agent-for-php-7.1	`77.18% <45.23%> (-0.10%)`	⬇️
agent-for-php-7.2	`78.11% <45.23%> (-0.10%)`	⬇️
agent-for-php-7.3	`78.13% <45.23%> (-0.10%)`	⬇️
agent-for-php-7.4	`77.83% <45.23%> (-0.10%)`	⬇️
agent-for-php-8.0	`77.86% <45.23%> (-0.14%)`	⬇️
agent-for-php-8.1	`77.84% <45.23%> (-0.14%)`	⬇️
agent-for-php-8.2	`77.44% <45.23%> (-0.14%)`	⬇️
agent-for-php-8.3	`77.44% <45.23%> (-0.14%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lavarou · 2024-06-14T19:01:55Z

agent/csec_metadata.c

+void zif_newrelic_get_security_metadata(void); /* ctags landing pad only */
+void newrelic_get_security_metadata(void);     /* ctags landing pad only */
+#endif
+PHP_FUNCTION(newrelic_get_security_metadata) {


Does this function need to be called from PHP?

No but this function will be called by the Security Agent

If that's the case then this shouldn't be a PHP user function but a function exported by newrelic-php-agent shared object, callable from a program by the means of dynamic binding - see https://linux.die.net/man/3/dlsym.

newrelic-php-agent already does similar thing - see here.

Thanks Michal, I will update the code and not expose this as a PHP user function. Thanks for the reference too.

I tried the dynamic call using dlopen and dlsym, but seems like the values may not be accessible.
nr_app_get_entity_name(NRPRG(app)) is one of the calls which is required, but NRPRG is the zend module's address space which does not seem accessible directly.

fprintf(stderr, "%p : newrelic_globals.app\n", NRPRG(app));

gives (nil) when called from dlopen'ed binary while from inside a valid address is printed.

Lets connect over call to finalise a solution.

@anmol-ap Please take a look at #922 - it should have what's needed.

Yes, the special purpose value for handle worked. Thanks.

lavarou · 2024-06-14T19:28:21Z

agent/scripts/newrelic.ini.template

+
+; Setting: newrelic.security.enabled
+; Type   : boolean
+; Scope  : system
+; Default: false
+; Info   : Indicates if attack detection security module is to be enabled
+;
+newrelic.security.enabled = true
+
+; Setting: newrelic.security.mode
+; Type   : string
+; Scope  : system
+; Default: "IAST"
+; Info   : Security module provides two modes "IAST" or "RASP"
+;          See documentation for more details
+;
+;newrelic.security.mode = "IAST"
+
+; Setting: newrelic.security.validator_service_endpoint_url
+; Type   : string
+; Scope  : system
+; Default: "wss://csec.nr-data.net"
+; Info   : New Relic<E2><80><99>s security module SaaS connection URLs
+;
+;newrelic.security.validator_service_url = "wss://csec.nr-data.net"
+
+; Setting: newrelic.security.agent.enabled
+; Type   : boolean
+; Scope  : system
+; Default: false
+; Info   : Used to enable security module, default false is equivalent to
+;           security module not even loaded. If this setting is set to true,
+;           then only security module is loaded and to enable it, a restart
+;           of application is required. This is different than
+;           newrelic.security.enabled, in terms that security.enabled decides
+;           runtime behavior of security module but security.agent.enabled
+;           would not even load the security module when set to false
+;
+;newrelic.security.agent.enabled = false
+
+; Setting: newrelic.security.detection.rci.enabled
+; Type   : boolean
+; Scope  : system
+; Default: true
+; Info   : Indicates if detection of remote code injection attack category is to be enabled
+;
+;newrelic.security.detection.rci.enabled = true
+
+; Setting: newrelic.security.detection.rxss.enabled
+; Type   : boolean
+; Scope  : system
+; Default: true
+; Info   : Indicates if detection of reflected xss attack category is to be enabled
+;
+;newrelic.security.detection.rxss.enabled = true
+
+; Setting: newrelic.security.detection.deserialization.enabled
+; Type   : boolean
+; Scope  : system
+; Default: true
+; Info   : Indicates if detection of deserialization attack category is to be enabled
+;
+;newrelic.security.detection.deserialization.enabled = true
+
+; Setting: newrelic.security.request.body_limit
+; Type   : unsigned integer
+; Scope  : system
+; Default: 300
+; Info   : Sets the maximum limit of the request body to be read in kb.
+;
+;newrelic.security.request.body_limit = 300
+
+; Setting: newrelic.security.validator.client_ssl_cert_filepath
+; Type   : string
+; Scope  : system
+; Default: none
+; Info   : Sets the full path of the client certificate in PEM
+;          format. When set, this certificate will be used to
+;          authenticate the New Relic IAST Security Engine's url. If
+;          not set, the default certificate will be used. Make
+;          sure the file permissions are correct.
+;
+;newrelic.security.validator.client_ssl_cert_filepath = ""


Why these settings are added? newrelic-php-agent is neither parsing nor using any of these settings.

newrelic-php-agent will be parsing newrelic.security.agent.enabled and newrelic.security.enabled settings and logging in the log file if these settings are disabled. That is consistent across other language agents also. And the other settings will be read by the Security agent and we do not want to introduce another config file.

newrelic-php-agent will be parsing newrelic.security.agent.enabled and newrelic.security.enabled settings and logging in the log file if these settings are disabled. That is consistent across other language agents also.

What is the meaning of these two flags? What is the difference between them?

And the other settings will be read by the Security agent and we do not want to introduce another config file.

Since newrelic-php-agent is not parsing any of those, I don't think they belong to the agent/scripts/newrelic.ini.template, which serves as a quick-access documentation of newrelic-php-agent settings. These settings must be properly documented in official documentation available on docs.newrelic.com. If Security agent wants to also provide a quick-access documentation for ini settings controlling its behavior, it should include its own ini template in the distribution.

What is the meaning of these two flags? What is the difference between them?

The newrelic.security.agent.enabled is a flag for not even loading the agent. But since php security agent is not a dependency like in other language agents, this flag tells the security agent to not perform instrumentation. A complete bypass of the agent. But this has been kept to keep the config consistent across all language agents.

The newrelic.security.enabled flag tells the php security agent to perform instrumentation and all the tasks for the security agent.

Since newrelic-php-agent is not parsing any of those, I don't think they belong to the

I understand this but IAST security agent is not a different agent for the customer. To the customer, there is a single agent which supports both APM and IAST. Therefore, we do not want a separate ini file for the security agent. Plus 2 settings mentioned above are already read and logged in APM agent.

lavarou · 2024-06-14T19:55:37Z

agent/php_nrini.c

+PHP_INI_ENTRY_EX("newrelic.security.agent.enabled",
+                 "0",
+                 NR_PHP_SYSTEM,
+                 nr_security_agent_enabled_mh,
+                 0)
+
+PHP_INI_ENTRY_EX("newrelic.security.enabled",
+                 "0",
+                 NR_PHP_SYSTEM,
+                 nr_security_enabled_mh,
+                 0)
+


Does the newrelic-php-agent need these settings? They don't seem to be used anywhere...

newrelic.security.agent.enabled and newrelic.security.enabled settings are logged in the APM log file if these settings are disabled in the minit function

Will the values of these settings be used by the security agent as well?

Yes, these values are to be used by the security agent as well as mentioned here in comment.

agent/scripts/newrelic.ini.template

anmol-ap · 2024-06-19T12:30:34Z

Updated the readme file and the third party notices as per the legal review : Legal Review

agent/scripts/newrelic.ini.template

lavarou · 2024-06-26T14:15:25Z

agent/csec_metadata.h

+#define KEY_ENTITY_NAME "entity.name"
+#define KEY_ENTITY_TYPE "entity.type"
+#define KEY_ENTITY_GUID "entity.guid"
+#define KEY_HOSTNAME "hostname"
+#define KEY_AGENT_RUN_ID "agent.run.id"
+#define KEY_ACCOUNT_ID "account.id"
+#define KEY_LICENSE "license"
+#define KEY_PLICENSE "plicense"
+#define KEY_HIGH_SECURITY "high_security"


Are all these values really required by csec-agent? Why? When does the csec-agent going to use these values? Some of these values can change within a request if PHP code calls newrelic_set_appname

Yes, the backend (Security Engine) uses these values to correctly map all the information for application, incidents, etc. csec-agent sends this information with the events.
If the value changes, as mentioned in the documentation for given method, same impact will be there. But we will not be missing any vulnerabilities due to this.

Yes, the backend (Security Engine) uses these values to correctly map all the information for application, incidents, etc. csec-agent sends this information with the events.

I really doubt that backend will use plicense to map anything. Why is this particular value needed?

If the value changes, as mentioned in the documentation for given method, same impact will be there. But we will not be missing any vulnerabilities due to this.

I do not understand your answer. If csec-agent is going to retrieve this information before newrelic_set_appname is called in PHP code, and send the 'event' to Security Engine after newrelic_set_appname is called, then the 'event' will not be correctly mapped. When does csec-agent retrieve the metadata and when does csec-agent generate the events sent to the backend? This is very important design consideration that needs to be taken into account.

We are logging some of these values, and that includes the license as well. Logging for license in particular has to be masked, and hence the reason for plicense. Unmasked license logging has previously being flagged at some agent.

At any point of time, when the apm agent is connected, entity.name is going to have a value along with entity.guid. When the newrelic_set_appname is called, csec-agent will be using that value on next reconnect cycle to the server. So, yes as soon as newrelic_set_appname is called, entity.name and entity.guid will not change for the csec-agent but on the next reconnect (this is planned at SE, the backend), the events will be mapped to new entity.name and entity.guid, this would reset the values for all the running and connected processes.

csec-agent tries to retrieve the metadata at the beginning of each request until it has data. It will wait for next reconnect cycle with server to see if anything has changed.

csec-agent generates the events based on the hooks and send the data immediately to the backend.

As has been detailed the intent of using newrelic_set_appname as early as possible in code, taking runtime values from APM agent would not affect the app's data at backend.

csec-agent tries to retrieve the metadata at the beginning of each request until it has data.

This is exactly why php-agent team has a problem with this. Not only there's potential for incorrect mapping of an event to an app, but also this depends on wether php-agent has been initialized before csec-agent, which we cannot see how it can be guaranteed. Why doesn't csec-agent retrieve the metadata only at the time it is needed, in the 'hook' you mention here:

csec-agent generates the events based on the hooks and send the data immediately to the backend.

csec-agent takes the connection information from the apm agent only. It does not generate any data until the apm agent has been connected. csec-agent is completely reliant on APM agent for entity.guid.

And without entity.guid, it does not connect to the backend

add support for csec php agent

091c2fb

anmol-ap requested review from ZNeumann and lavarou June 14, 2024 07:54

anmol-ap self-assigned this Jun 14, 2024

lavarou reviewed Jun 14, 2024

View reviewed changes

anmol-ap requested a review from lavarou June 17, 2024 12:05

lavarou reviewed Jun 17, 2024

View reviewed changes

agent/scripts/newrelic.ini.template Outdated Show resolved Hide resolved

update readme and third party notices as per legal review

430d665

newrelic.security.enabled should be false by default

19739b6

lavarou reviewed Jun 21, 2024

View reviewed changes

agent/scripts/newrelic.ini.template Show resolved Hide resolved

lavarou changed the title ~~add support for csec php agent~~ feat(agent): add support for csec php agent Jun 21, 2024

add extension loading to ini file

c2f0349

lavarou reviewed Jun 26, 2024

View reviewed changes

lavarou added the csec-agent label Jul 11, 2024

bduranleau-nr added the on hold This issue or pull request is necessary, but better suited for the future label Nov 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(agent): add support for csec php agent #918

feat(agent): add support for csec php agent #918

anmol-ap commented Jun 14, 2024 •

edited by jira bot

Loading

newrelic-php-agent-bot commented Jun 14, 2024 •

edited

Loading

codecov-commenter commented Jun 14, 2024 •

edited

Loading

lavarou Jun 14, 2024

anmol-ap Jun 17, 2024

lavarou Jun 18, 2024

lavarou Jun 18, 2024

anmol-ap Jun 19, 2024

anmol-ap Jun 21, 2024

lavarou Jun 21, 2024

anmol-ap Jun 24, 2024

lavarou Jun 14, 2024

anmol-ap Jun 17, 2024

lavarou Jun 17, 2024

anmol-ap Jun 19, 2024

lavarou Jun 14, 2024

anmol-ap Jun 17, 2024

lavarou Jun 17, 2024

anmol-ap Jun 19, 2024

anmol-ap commented Jun 19, 2024

lavarou Jun 26, 2024 •

edited

Loading

anmol-ap Jun 27, 2024

lavarou Jun 27, 2024 •

edited

Loading

anmol-ap Jun 27, 2024 •

edited

Loading

lavarou Jun 27, 2024

anmol-ap Jun 27, 2024 •

edited

Loading

anmol-ap Jun 27, 2024

feat(agent): add support for csec php agent #918

Are you sure you want to change the base?

feat(agent): add support for csec php agent #918

Conversation

anmol-ap commented Jun 14, 2024 • edited by jira bot Loading

newrelic-php-agent-bot commented Jun 14, 2024 • edited Loading

codecov-commenter commented Jun 14, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmol-ap commented Jun 19, 2024

lavarou Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavarou Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

anmol-ap Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmol-ap Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmol-ap commented Jun 14, 2024 •

edited by jira bot

Loading

newrelic-php-agent-bot commented Jun 14, 2024 •

edited

Loading

codecov-commenter commented Jun 14, 2024 •

edited

Loading

lavarou Jun 26, 2024 •

edited

Loading

lavarou Jun 27, 2024 •

edited

Loading

anmol-ap Jun 27, 2024 •

edited

Loading

anmol-ap Jun 27, 2024 •

edited

Loading