Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 2 commits into from
Oct 26, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66b8d2e
Add logic to remove default class_transformer.excludes when security …
jasonjkeller Aug 16, 2023
1ab94a6
Merge branch 'main' into 1447-security-agent-excludes
jasonjkeller Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions newrelic-agent/src/main/java/com/newrelic/agent/config/SecurityAgentConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,13 @@

package com.newrelic.agent.config;

import com.google.common.collect.Sets;
import com.newrelic.api.agent.Config;
import com.newrelic.api.agent.NewRelic;

import java.util.Collections;
import java.util.Set;

/* Default config should look like:
*
* security:
Expand Down Expand Up @@ -48,6 +52,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.*"));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Create supportability metrics showing the enabled status of the security agent.
Expand Down
59 changes: 45 additions & 14 deletions newrelic-agent/src/main/java/com/newrelic/agent/instrumentation/ClassNameFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
import java.util.Set;
import java.util.regex.Pattern;

import static com.newrelic.agent.config.SecurityAgentConfig.SECURITY_AGENT_CLASS_TRANSFORMER_EXCLUDES_TO_IGNORE;
import static com.newrelic.agent.config.SecurityAgentConfig.shouldInitializeSecurityAgent;

/**
* This filter is used to skip certain classes during the classloading transform callback based on the class name.
*/
Expand All @@ -38,8 +41,13 @@ public ClassNameFilter(IAgentLogger logger) {
}

/**
* Is the class excluded.
*
* Check if a class should be excluded from class transformation based on an excludes rule.
* <p>
* excludePatterns is a list of exclude rules merged together from user config and the default META-INF/excludes file.
* Exclude rules are evaluated when determining whether to transform weaved classes in the InstrumentationContextManager
* or pointcuts in the PointCutClassTransformer.
*
* @param className name of the class to check exclusion rule for
* @return <code>true</code> if this is an excluded class, <code>false</code> if not
*/
public boolean isExcluded(String className) {
Expand All @@ -53,7 +61,7 @@ public boolean isExcluded(String className) {

/**
* Is the class included.
*
*
* @return <code>true</code> if this is an included class, <code>false</code> if not
*/
public boolean isIncluded(String className) {
Expand All @@ -79,6 +87,10 @@ public void addConfigClassFilters(AgentConfig config) {
for (String exclude : excludes) {
sb.append("\n").append(exclude);
addExclude(exclude);
if (SECURITY_AGENT_CLASS_TRANSFORMER_EXCLUDES_TO_IGNORE.contains(exclude)) {
logger.finer(exclude + " 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.");
}
}
logger.finer(sb.toString());
}
Expand All @@ -89,7 +101,7 @@ public void addConfigClassFilters(AgentConfig config) {
}

/**
* Add excluded classes in the excludes file.
* Add excluded classes in the META-INF/excludes file.
*/
public void addExcludeFileClassFilters() {
InputStream iStream = this.getClass().getResourceAsStream(EXCLUDES_FILE);
Expand All @@ -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();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


for (String exclude : excludeList) {
addExclude(exclude);
ignoreExcludeForSecurityAgent = shouldInitializeSecurityAgent && SECURITY_AGENT_CLASS_TRANSFORMER_EXCLUDES_TO_IGNORE.contains(exclude);
if (ignoreExcludeForSecurityAgent) {
logger.finer("Ignored " + exclude + " 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=" + formattedIgnoredExcludes +
" or NEW_RELIC_CLASS_TRANSFORMER_EXCLUDES=" + formattedIgnoredExcludes);
} else {
addExclude(exclude);
}
}

if (ignoreExcludeForSecurityAgent) {
for (String exclude : SECURITY_AGENT_CLASS_TRANSFORMER_EXCLUDES_TO_IGNORE) {
// Remove the default exclude rule if the security agent is enabled. If a user explicitly adds one of these exclude rules via agent config
// then it will be added back to the list via ClassNameFilter.addConfigClassFilters, effectively taking precedence over this removal logic.
excludeList.remove(exclude);
}
}
logger.finer("Excludes initialized: " + excludeList);
}

/**
* Include matching classes.
*
*
* @param include either a class name or a regular expression
*/
public void addInclude(String include) {
Expand All @@ -133,7 +164,7 @@ public void addInclude(String include) {

/**
* Include the given class.
*
*
* @param className the name of the class to include
*/
public void addIncludeClass(String className) {
Expand All @@ -143,7 +174,7 @@ public void addIncludeClass(String className) {

/**
* Include classes matching the regular expression.
*
*
* @param regex a regular expression matching classes to include
*/
public void addIncludeRegex(String regex) {
Expand All @@ -155,7 +186,7 @@ public void addIncludeRegex(String regex) {

/**
* Exclude matching classes.
*
*
* @param exclude either a class name or a regular expression
*/
public void addExclude(String exclude) {
Expand All @@ -168,7 +199,7 @@ public void addExclude(String exclude) {

/**
* Exclude the given class.
*
*
* @param className the name of the class to exclude
*/
public void addExcludeClass(String className) {
Expand All @@ -178,7 +209,7 @@ public void addExcludeClass(String className) {

/**
* Exclude classes matching the regular expression.
*
*
* @param regex a regular expression matching classes to include
*/
public void addExcludeRegex(String regex) {
Expand All @@ -190,7 +221,7 @@ public void addExcludeRegex(String regex) {

/**
* Create a regular expression for a class.
*
*
* @return a regular expression that matches only the given class
*/
private String classNameToRegex(String className) {
Expand All @@ -199,7 +230,7 @@ private String classNameToRegex(String className) {

/**
* Compile a regular expression.
*
*
* @return a pattern or null if the regular expression could not be compiled
*/
private Pattern regexToPattern(String regex) {
Expand All @@ -214,7 +245,7 @@ private Pattern regexToPattern(String regex) {

/**
* Is the argument a regular expression (rather than a class name).
*
*
* @param value
* @return <code>true</code> if this is a regular expression, <code>false</code> if not
*/
Expand Down
6 changes: 0 additions & 6 deletions ...c/main/java/com/newrelic/agent/instrumentation/context/InstrumentationContextManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@

import java.lang.instrument.ClassFileTransformer;
import java.lang.instrument.Instrumentation;
import java.text.MessageFormat;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
Expand Down Expand Up @@ -220,11 +219,6 @@ public boolean shouldTransform(final String internalClassName, final ClassLoader
Agent.LOG.log(Level.FINEST, "Skipping transform of {0}. Classloader {1} is excluded.", internalClassName, classloader);
return false;
}
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;
}
Comment on lines -223 to -227
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (classNameFilter.isIncluded(internalClassName)) {
Agent.LOG.log(Level.FINEST, "Class {0} is explicitly included", internalClassName);
return true;
Expand Down
11 changes: 7 additions & 4 deletions newrelic-agent/src/main/resources/META-INF/excludes
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,6 @@
# adding exclusions for obfuscated jar - see ticket 197106
^asposewobfuscated/.*
^com/aspose/words/.*
# saxon xlst classes - see ticket 195949
^net/sf/saxon.*
# exclude java.security and sun.reflect classes from transformation
^java/security/.*
^sun/reflect/.*
# exclude lambdas by excluding java.lang.invoke.* classes
^java/lang/invoke/.*
Expand Down Expand Up @@ -94,3 +90,10 @@
^java/util/stream/MatchOps\$MatchOp
^java/util/stream/MatchOps\$BooleanTerminalSink
^javax/security/auth/Subject\$SecureSet\$1
### 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.*
Loading
Loading