Add logic to remove default class_transformer.excludes when security … #1453

jasonjkeller · 2023-08-16T22:15:32Z

…agent is enabled

DO NOT MERGE!

By default, the following three class_transformer.exclude rules defined in META-INF/excludes will be applied to prevent any classes from being transformed in packages matching ^java/security/.*, ^javax/crypto/.*, or ^net/sf/saxon.*. Exclude rules are evaluated when determining whether to transform classes in the InstrumentationContextManager or the PointCutClassTransformer.

### All default excludes listed below get ignored when the security agent is enabled
# exclude java.security and sun.reflect classes from transformation
^java/security/.*
# crypto classes can cause class circularity errors if they get too far along in the class transformer
^javax/crypto/.*
# saxon xlst classes - see ticket 195949
^net/sf/saxon.*

Classes from these packages are excluded from transformation to prevent various issues such as long startup times (net/sf/saxon), ClassCircularityErrors (javax/crypto), and StackOverflowErrors (java/security).

However, preventing transformation of classes in these packages hinders functionality of the security agent.

This PR will remove the three listed default exclude rules when the security agent is enabled. The following will be logged indicating that this behavior is taking place.

2023-08-16T15:17:11,001-0700 [43845 1] com.newrelic.agent.instrumentation.PointCutClassTransformer FINER: Ignored ^java/security/.* class_transformer exclude defined in META-INF/excludes because the security agent is enabled. This can be overridden by explicitly setting newrelic.config.class_transformer.excludes=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.* or NEW_RELIC_CLASS_TRANSFORMER_EXCLUDES=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.*
2023-08-16T15:17:11,001-0700 [43845 1] com.newrelic.agent.instrumentation.PointCutClassTransformer FINER: Ignored ^javax/crypto/.* class_transformer exclude defined in META-INF/excludes because the security agent is enabled. This can be overridden by explicitly setting newrelic.config.class_transformer.excludes=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.* or NEW_RELIC_CLASS_TRANSFORMER_EXCLUDES=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.*
2023-08-16T15:17:11,001-0700 [43845 1] com.newrelic.agent.instrumentation.PointCutClassTransformer FINER: Ignored ^net/sf/saxon.* class_transformer exclude defined in META-INF/excludes because the security agent is enabled. This can be overridden by explicitly setting newrelic.config.class_transformer.excludes=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.* or NEW_RELIC_CLASS_TRANSFORMER_EXCLUDES=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.*
2023-08-16T15:17:11,012-0700 [43845 1] com.newrelic FINER: Ignored ^java/security/.* class_transformer exclude defined in META-INF/excludes because the security agent is enabled. This can be overridden by explicitly setting newrelic.config.class_transformer.excludes=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.* or NEW_RELIC_CLASS_TRANSFORMER_EXCLUDES=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.*
2023-08-16T15:17:11,012-0700 [43845 1] com.newrelic FINER: Ignored ^javax/crypto/.* class_transformer exclude defined in META-INF/excludes because the security agent is enabled. This can be overridden by explicitly setting newrelic.config.class_transformer.excludes=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.* or NEW_RELIC_CLASS_TRANSFORMER_EXCLUDES=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.*
2023-08-16T15:17:11,012-0700 [43845 1] com.newrelic FINER: Ignored ^net/sf/saxon.* class_transformer exclude defined in META-INF/excludes because the security agent is enabled. This can be overridden by explicitly setting newrelic.config.class_transformer.excludes=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.* or NEW_RELIC_CLASS_TRANSFORMER_EXCLUDES=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.*

Additionally, even if the security agent is enabled, these exclude rules can be explicitly added via one of the following agent config options, in which case the user configured exclude rule will take precedence and will not be removed due to the security agent being enabled.

YAML:

  class_transformer:
    excludes: ^javax/crypto/.*,^java/security/.*,^net/sf/saxon.*

System property:

-Dnewrelic.config.class_transformer.excludes=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.*

Environment variable:

NEW_RELIC_CLASS_TRANSFORMER_EXCLUDES=^javax/crypto/.*,^java/security/.*,^net/sf/saxon.*

When explicitly setting the excludes via user config the following will be logged:

2023-08-16T15:17:11,000-0700 [43845 1] com.newrelic.agent.instrumentation.PointCutClassTransformer FINER: ^java/security/.* class_transformer exclude explicitly added by user config. The user configured exclude rule will take precedence and will not be ignored due to the security agent being enabled.
2023-08-16T15:17:11,000-0700 [43845 1] com.newrelic.agent.instrumentation.PointCutClassTransformer FINER: ^javax/crypto/.* class_transformer exclude explicitly added by user config. The user configured exclude rule will take precedence and will not be ignored due to the security agent being enabled.
2023-08-16T15:17:11,000-0700 [43845 1] com.newrelic.agent.instrumentation.PointCutClassTransformer FINER: ^net/sf/saxon.* class_transformer exclude explicitly added by user config. The user configured exclude rule will take precedence and will not be ignored due to the security agent being enabled.
2023-08-16T15:17:11,011-0700 [43845 1] com.newrelic FINER: ^java/security/.* class_transformer exclude explicitly added by user config. The user configured exclude rule will take precedence and will not be ignored due to the security agent being enabled.
2023-08-16T15:17:11,011-0700 [43845 1] com.newrelic FINER: ^javax/crypto/.* class_transformer exclude explicitly added by user config. The user configured exclude rule will take precedence and will not be ignored due to the security agent being enabled.
2023-08-16T15:17:11,012-0700 [43845 1] com.newrelic FINER: ^net/sf/saxon.* class_transformer exclude explicitly added by user config. The user configured exclude rule will take precedence and will not be ignored due to the security agent being enabled.

…agent is enabled

jasonjkeller · 2023-08-16T22:45:13Z

newrelic-agent/src/main/java/com/newrelic/agent/config/SecurityAgentConfig.java

@@ -44,6 +48,8 @@ public class SecurityAgentConfig {
    private static final Config config = NewRelic.getAgent().getConfig();
    private static final String ENABLED = "enabled";
    private static final String DISABLED = "disabled";
+    public static final Set<String> SECURITY_AGENT_CLASS_TRANSFORMER_EXCLUDES_TO_IGNORE = Collections.unmodifiableSet(
+            Sets.newHashSet("^java/security/.*", "^javax/crypto/.*", "^net/sf/saxon.*"));


This is the full set of default class_transformer.exclude rules that need to be ignored when the security agent is enabled. Should further exclude rules need to be treated the same in the future they can simply be added here and no other logic should need to be changed elsewhere in the agent codebase. Only new tests would need to be added to IncludeExcludeTest.

jasonjkeller · 2023-08-16T22:47:15Z

.../src/main/java/com/newrelic/agent/instrumentation/context/InstrumentationContextManager.java

-        if (internalClassName.startsWith("javax/crypto/")) {
-            // crypto classes can cause class circularity errors if they get too far along in the class transformer
-            Agent.LOG.finest(MessageFormat.format("Instrumentation skipped by ''javax crypto'' rule: {0}", internalClassName));
-            return false;
-        }


This exclude rule was moved to the META-INF/excludes file to simplify the logic and keep all of the security agent related excludes in a central place.

jasonjkeller · 2023-08-16T22:47:57Z

newrelic-agent/src/main/resources/META-INF/excludes

+### All default excludes listed below get ignored when the security agent is enabled
+# exclude java.security and sun.reflect classes from transformation
+^java/security/.*
+# crypto classes can cause class circularity errors if they get too far along in the class transformer
+^javax/crypto/.*
+# saxon xlst classes - see ticket 195949
+^net/sf/saxon.*


These are the three default exclude rules that are effected by the security agent.

codecov-commenter · 2023-08-16T22:49:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.60%. Comparing base (7435d77) to head (1ab94a6).
Report is 1231 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #1453   +/-   ##
=========================================
  Coverage     70.60%   70.60%           
- Complexity     9794     9795    +1     
=========================================
  Files           817      817           
  Lines         39489    39500   +11     
  Branches       5995     5999    +4     
=========================================
+ Hits          27880    27888    +8     
  Misses         8902     8902           
- Partials       2707     2710    +3

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

jasonjkeller · 2023-08-16T22:50:04Z

newrelic-agent/src/main/java/com/newrelic/agent/instrumentation/ClassNameFilter.java

@@ -112,15 +124,34 @@ public void addExcludeFileClassFilters() {
                // ignore
            }
        }
+        boolean ignoreExcludeForSecurityAgent = false;
+        String formattedIgnoredExcludes = String.join(",", SECURITY_AGENT_CLASS_TRANSFORMER_EXCLUDES_TO_IGNORE);
+        boolean shouldInitializeSecurityAgent = shouldInitializeSecurityAgent();


Only consider removing one of the default exclude rules if the shouldInitializeSecurityAgent() check evaluates to true.

lovesh-ap · 2023-08-29T04:31:15Z

@jasonjkeller Hey there! 👋 Just wanted to let you know that we currently have an open issue (#154979) related to the changes in this PR. It might be good to keep an eye on that while reviewing this. Thanks!

Add logic to remove default class_transformer.excludes when security …

66b8d2e

…agent is enabled

jasonjkeller commented Aug 16, 2023

View reviewed changes

jtduffy approved these changes Aug 17, 2023

View reviewed changes

jasonjkeller mentioned this pull request Aug 17, 2023

Remove agent exclusions and run tests (IAST support) #1447

Closed

tbradellis approved these changes Aug 31, 2023

View reviewed changes

This was referenced Oct 2, 2023

NR-137989 Remove java/security/.* excludes #1445

Closed

Remove javax/crypto/.* excludes #1446

Closed

NR-137973 Remove net/sf/saxon.* excludes #1444

Closed

Merge branch 'main' into 1447-security-agent-excludes

1ab94a6

jasonjkeller merged commit 575b695 into main Oct 26, 2023
103 checks passed

jasonjkeller deleted the 1447-security-agent-excludes branch October 26, 2023 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logic to remove default class_transformer.excludes when security … #1453

Add logic to remove default class_transformer.excludes when security … #1453

jasonjkeller commented Aug 16, 2023 •

edited

Loading

jasonjkeller Aug 16, 2023

jasonjkeller Aug 16, 2023

jasonjkeller Aug 16, 2023

codecov-commenter commented Aug 16, 2023 •

edited

Loading

jasonjkeller Aug 16, 2023

lovesh-ap commented Aug 29, 2023

Add logic to remove default class_transformer.excludes when security … #1453

Add logic to remove default class_transformer.excludes when security … #1453

Conversation

jasonjkeller commented Aug 16, 2023 • edited Loading

jasonjkeller Aug 16, 2023

Choose a reason for hiding this comment

jasonjkeller Aug 16, 2023

Choose a reason for hiding this comment

jasonjkeller Aug 16, 2023

Choose a reason for hiding this comment

codecov-commenter commented Aug 16, 2023 • edited Loading

Codecov Report

jasonjkeller Aug 16, 2023

Choose a reason for hiding this comment

lovesh-ap commented Aug 29, 2023

jasonjkeller commented Aug 16, 2023 •

edited

Loading

codecov-commenter commented Aug 16, 2023 •

edited

Loading