-
Notifications
You must be signed in to change notification settings - Fork 66
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
feat(agent): add support for csec php agent #918
base: dev
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
#include "csec_metadata.h" | ||
#include "util_strings.h" | ||
#include "php_hash.h" | ||
#include "php_api_internal.h" | ||
|
||
static void nr_csec_php_add_assoc_string_const(zval* arr, | ||
const char* key, | ||
const char* value) { | ||
char* val = NULL; | ||
|
||
if (NULL == arr || NULL == key || NULL == value) { | ||
return; | ||
} | ||
|
||
val = nr_strdup(value); | ||
nr_php_add_assoc_string(arr, key, val); | ||
nr_free(val); | ||
} | ||
|
||
#ifdef TAGS | ||
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) { | ||
|
||
NR_UNUSED_RETURN_VALUE; | ||
NR_UNUSED_RETURN_VALUE_PTR; | ||
NR_UNUSED_RETURN_VALUE_USED; | ||
NR_UNUSED_THIS_PTR; | ||
NR_UNUSED_EXECUTE_DATA; | ||
|
||
array_init(return_value); | ||
|
||
nr_csec_php_add_assoc_string_const(return_value, KEY_ENTITY_NAME, nr_app_get_entity_name(NRPRG(app))); | ||
nr_csec_php_add_assoc_string_const(return_value, KEY_ENTITY_TYPE, nr_app_get_entity_type(NRPRG(app))); | ||
nr_csec_php_add_assoc_string_const(return_value, KEY_ENTITY_GUID, nr_app_get_entity_guid(NRPRG(app))); | ||
nr_csec_php_add_assoc_string_const(return_value, KEY_HOSTNAME, nr_app_get_host_name(NRPRG(app))); | ||
nr_csec_php_add_assoc_string_const(return_value, KEY_LICENSE, NRPRG(license).value); | ||
|
||
if (NRPRG(app)) { | ||
nr_csec_php_add_assoc_string_const(return_value, KEY_AGENT_RUN_ID, NRPRG(app)->agent_run_id); | ||
nr_csec_php_add_assoc_string_const(return_value, KEY_ACCOUNT_ID, NRPRG(app)->account_id); | ||
nr_csec_php_add_assoc_string_const(return_value, KEY_PLICENSE, NRPRG(app)->plicense); | ||
int high_security = NRPRG(app)->info.high_security; | ||
add_assoc_long(return_value, KEY_HIGH_SECURITY, (long)high_security); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
#include "php_agent.h" | ||
#include "util_hashmap.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" | ||
Comment on lines
+4
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I really doubt that backend will use
I do not understand your answer. If csec-agent is going to retrieve this information before There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And without entity.guid, it does not connect to the backend |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -515,6 +515,58 @@ static PHP_INI_MH(nr_high_security_mh) { | |
return SUCCESS; | ||
} | ||
|
||
static PHP_INI_MH(nr_security_enabled_mh) { | ||
int val; | ||
|
||
(void)entry; | ||
(void)NEW_VALUE_LEN; | ||
(void)mh_arg1; | ||
(void)mh_arg2; | ||
(void)mh_arg3; | ||
(void)stage; | ||
NR_UNUSED_TSRMLS; | ||
|
||
val = nr_bool_from_str(NEW_VALUE); | ||
|
||
if (-1 == val) { | ||
return FAILURE; | ||
} | ||
|
||
if (val) { | ||
NR_PHP_PROCESS_GLOBALS(nr_security_enabled) = true; | ||
} else { | ||
NR_PHP_PROCESS_GLOBALS(nr_security_enabled) = false; | ||
} | ||
|
||
return SUCCESS; | ||
} | ||
|
||
static PHP_INI_MH(nr_security_agent_enabled_mh) { | ||
int val; | ||
|
||
(void)entry; | ||
(void)NEW_VALUE_LEN; | ||
(void)mh_arg1; | ||
(void)mh_arg2; | ||
(void)mh_arg3; | ||
(void)stage; | ||
NR_UNUSED_TSRMLS; | ||
|
||
val = nr_bool_from_str(NEW_VALUE); | ||
|
||
if (-1 == val) { | ||
return FAILURE; | ||
} | ||
|
||
if (val) { | ||
NR_PHP_PROCESS_GLOBALS(nr_security_agent_enabled) = true; | ||
} else { | ||
NR_PHP_PROCESS_GLOBALS(nr_security_agent_enabled) = false; | ||
} | ||
|
||
return SUCCESS; | ||
} | ||
|
||
static PHP_INI_MH(nr_preload_framework_library_detection_mh) { | ||
int val; | ||
|
||
|
@@ -2009,6 +2061,18 @@ PHP_INI_ENTRY_EX("newrelic.high_security", | |
nr_high_security_mh, | ||
0) | ||
|
||
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) | ||
|
||
Comment on lines
+2064
to
+2075
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the newrelic-php-agent need these settings? They don't seem to be used anywhere... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will the values of these settings be used by the security agent as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, these values are to be used by the security agent as well as mentioned here in comment. |
||
/* | ||
* Feature flag handling. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1332,3 +1332,86 @@ newrelic.daemon.logfile = "/var/log/newrelic/newrelic-daemon.log" | |
; for vulnerability management. | ||
; | ||
;newrelic.vulnerability_management.package_detection.enabled = true | ||
|
||
; 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 | ||
anmol-ap marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
; 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 = "" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why these settings are added? newrelic-php-agent is neither parsing nor using any of these settings. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What is the meaning of these two flags? What is the difference between them?
Since newrelic-php-agent is not parsing any of those, I don't think they belong to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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. |
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.
Does this function need to be called from PHP?
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.
No but this function will be called by the Security Agent
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.
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.
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.
newrelic-php-agent already does similar thing - see here.
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.
Thanks Michal, I will update the code and not expose this as a PHP user function. Thanks for the reference too.
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.
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, butNRPRG
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.
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.
@anmol-ap Please take a look at #922 - it should have what's needed.
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.
Yes, the special purpose value for handle worked. Thanks.