-
Notifications
You must be signed in to change notification settings - Fork 69
feat(agent): add support for csec php agent #918
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
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
- Loading branch information
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. | ||
| */ | ||
|
|
||
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, butNRPRGis 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.