Skip to content

Fix security manager issues #2871

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

Jump to bottom
Merged
merged 19 commits into from
Nov 23, 2022
Merged

Fix security manager issues #2871

merged 19 commits into from
Nov 23, 2022

Conversation

@SylvainJuge
Copy link
Member

@SylvainJuge SylvainJuge commented Nov 16, 2022 •
edited
Loading

What does this PR do?

Fixes #2520

  • fixes NoClassDefFound errors triggered in classloading due to URL validation in JDK outside of a doPrivileged wrapping.
  • fixes few missing doPrivileged calls to make the agent able to start properly (reading environment variables, get classloader reference of an existing class and resolve method handles).
  • surprisingly, some of the calls to System.getenv trigger exception when not called in a privileged action, whereas the System.getProperty or System.getProperties do not, as a result I added a PrivilegedActionUtils class to properly wrap those calls with minimal code duplication and verbosity.

Checklist

  • This is a bugfix
SylvainJuge
SylvainJuge commented Nov 16, 2022
View reviewed changes
elastic-apm-agent/src/main/java/co/elastic/apm/agent/premain/ShadedClassLoader.java Outdated
Comment on lines 231 to 243
public URL findResource(final String name) {
URL result = null;
if (!locallyNonAvailableResources.get().contains(name)) {

// while most of the body of default 'findResource' in JDK implementation is in a privileged action
// an extra URL check is performed just after it, hence we have to wrap the whole method call in a privileged
// action otherwise the security manager will complain about lack of proper read privileges on the agent jar
result = AccessController.doPrivileged(new PrivilegedAction<URL>() {
@Override
public URL run() {
return findResourceInternal(getShadedResourceName(name));
}
});
Copy link
Member Author

@SylvainJuge SylvainJuge Nov 16, 2022

Choose a reason for hiding this comment

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

[for reviewer] this fixes the NoClassDefFound exceptions, the JDK implementation of findResource that we call through the super.findResource(name) performs an extra URL check outside of a privileged action.
@ghost
Copy link

ghost commented Nov 16, 2022 •
edited by ghost
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-11-23T17:14:30.580+0000

  • Duration: 57 min 24 sec

Test stats 🧪

Test Results
Failed 0
Passed 3209
Skipped 36
Total 3245

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark tests.

  • run jdk compatibility tests : Run the JDK Compatibility tests.

  • run integration tests : Run the Agent Integration tests.

  • run end-to-end tests : Run the APM-ITs.

  • run windows tests : Build & tests on windows.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)
@SylvainJuge SylvainJuge marked this pull request as ready for review November 17, 2022 08:39
@github-actions
Copy link

github-actions bot commented Nov 17, 2022

/test
eyalkoren
eyalkoren reviewed Nov 17, 2022
View reviewed changes
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

Awesome fix!! 👏
See comments
@SylvainJuge
Copy link
Member Author

SylvainJuge commented Nov 17, 2022

@eyalkoren avoiding allocation here might be something quite challenging, especially for the cases when there are multiple parameters, do you think that would be required ?

Alternatively, maybe a simpler approach here would be to simply shortcut this extra wrapping when System.getSecurityManager() == null and avoid this extra allocation when security manager is disabled. My assumption here is that users of Security Manager do not have strong expectations in memory allocation as their application is very likely to have to perform similar wrapping with privileged actions. I will update this PR to this approach and we can further discuss the final implementation.
@SylvainJuge
Copy link
Member Author

SylvainJuge commented Nov 18, 2022

Status:

  • I have used a rather simple but tedious approach to delegate to a utility class in order to wrap with AccessController.doPrivileged
  • doing this means we have to review and modify where the agent might call the security manager through multiple code paths.
  • this PR does not yet cover all aspects, for example all the file access by the agent or the logger plugins will trigger the security manager issues.
  • there might be a better and more generic approach that we need to discuss further.
@eyalkoren
Copy link
Contributor

eyalkoren commented Nov 20, 2022

Alternatively, maybe a simpler approach here would be to simply shortcut this extra wrapping when System.getSecurityManager() == null and avoid this extra allocation when security manager is disabled.

Definitely, this sounds very reasonable. Just let's not have this overhead penalty to the vast majority of users that don't use the security manager.
@SylvainJuge SylvainJuge mentioned this pull request Nov 21, 2022
Merged
3 tasks
@eyalkoren
Copy link
Contributor

eyalkoren commented Nov 22, 2022

Closes #390
@eyalkoren eyalkoren mentioned this pull request Nov 22, 2022
Merged
@SylvainJuge SylvainJuge added the await-release Mark issues that depend on next release, or PRs that are planned to be included label Nov 23, 2022
eyalkoren
eyalkoren reviewed Nov 23, 2022
View reviewed changes
Copy link
Contributor

@eyalkoren eyalkoren left a comment •
edited
Loading

Choose a reason for hiding this comment

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

What a magnificent refactoring! 👏
Looks really good and I recognized lots of stuff I saw failing in #2883, which is a good indication that we are in the right direction.

Some minor comments still.
eyalkoren
eyalkoren approved these changes Nov 23, 2022
View reviewed changes
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

Only one more exception to avoid automatic casting in AbstractJulEcsReformattingHelper
@SylvainJuge SylvainJuge enabled auto-merge (squash) November 23, 2022 16:52
@SylvainJuge SylvainJuge merged commit 965c656 into elastic:main Nov 23, 2022
@SylvainJuge SylvainJuge deleted the security-manager-tomcat branch November 24, 2022 07:49
@eyalkoren eyalkoren mentioned this pull request Nov 27, 2022
Closed
@JonasKunz JonasKunz removed the await-release Mark issues that depend on next release, or PRs that are planned to be included label Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-java

3 participants

@SylvainJuge @eyalkoren @JonasKunz