Allow SDK to run in environments prohibiting use of sun.misc.Unsafe #4902

rrva · 2022-10-31T21:39:57Z

Some applications run under strict java.security permissions which do not allow access to sun.misc.Unsafe.

BatchSpanProcessor uses Unsafe via jctools, but has a fallback to ArrayBlockingQueue. Extending that fallback rule to cover java security exceptions as well.

Since the entire java security manager is marked for deprecation in future java versions, I went with string-matching on the root cause message, which removes deprecation warnings when building with newer java but still does the job in those versions which use java.security policies.

linux-foundation-easycla · 2022-10-31T21:40:02Z

The committers listed above are authorized under a signed CLA.

✅ login: rrva / name: Ragnar Rova (5c283d9)

rrva · 2022-10-31T21:46:13Z

The Individual CLA option is not enabled for this project.

trask

thx @rrva! do you have any ideas on how to run some(?) tests under the security manager?

trask · 2022-10-31T23:00:00Z

The Individual CLA option is not enabled for this project.

hi @rrva, it should be, can you attach a screenshot of what you see?

rrva · 2022-11-01T19:45:40Z

The Individual CLA option is not enabled for this project.

hi @rrva, it should be, can you attach a screenshot of what you see?

Signed. Yesterday I got an error saying the individual option was not enabled and I should contact the project admin.

rrva · 2022-11-01T19:59:21Z

thx @rrva! do you have any ideas on how to run some(?) tests under the security manager?

I have added a test and squashed all commits

codecov · 2022-11-01T20:41:45Z

Codecov Report

Base: 90.93% // Head: 90.96% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (511a3ac) compared to base (ef076c2).
Patch coverage: 33.33% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4902      +/-   ##
============================================
+ Coverage     90.93%   90.96%   +0.03%     
- Complexity     4817     4820       +3     
============================================
  Files           545      545              
  Lines         14383    14388       +5     
  Branches       1383     1384       +1     
============================================
+ Hits          13079    13088       +9     
+ Misses          901      897       -4     
  Partials        403      403

Impacted Files	Coverage Δ
...a/io/opentelemetry/sdk/trace/internal/JcTools.java	`31.57% <33.33%> (+3.00%)`	⬆️
...metry/sdk/logs/export/BatchLogRecordProcessor.java	`89.70% <0.00%> (+0.73%)`	⬆️
.../io/opentelemetry/api/internal/PercentEscaper.java	`84.21% <0.00%> (+4.51%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

rrva · 2022-11-01T21:39:25Z

I get higher code coverage of JcTools.java than codecov when checking with "Run with coverage" in IntelliJ. What could be wrong?

rrva · 2022-11-01T23:56:50Z

@trask Please have a look at what I could do to the coverage issue of JcTools.java, it's a bit tricky.

I think what is happening is that MpscArrayQueueProducerIndexField.P_INDEX_OFFSET is sometimes sucessfully initialized before the SunMiscProhibitedSecurityManager is installed. Since it is a static field it is only initialized once per JVM invocation, it stays that way. This initialization is needed to fail if JcToolsSecurityManagerTest is to be allowed to cover the code paths I have changed.

I'm a bit unsure if this theory holds but thats what I think is happening when running jacoco.

I think we either need to find a way that jacoco runs JcToolsSecurityManagerTest in an isolated jvm or excluding the changed code from coverage checks.

When I run JcToolsSecurityManagerTest and JcToolsTest test separately in IntelliJ I do get coverage on all paths. When I run JcToolsSecurityManagerTest without the changes in this PR it fails as expected.

...haded-deps/src/test/java/io/opentelemetry/sdk/trace/internal/JcToolsSecurityManagerTest.java

jkwatson · 2022-11-02T03:28:54Z

codecov is claiming quite a bit of uncovered new code in this PR. Can you add more test cases to cover what it says isn't covered?

sdk/trace-shaded-deps/build.gradle.kts

rrva · 2022-11-02T19:35:23Z

@jsuereth

I simplified the actual change to the code down to a single line, the rest is test code (which jacoco has trouble recognizing coverage of when running the entire projects' tests).

It also needs this ugly thing (only in the build.gradle.kts of the sdk/trace-shaded-deps module which today only contains a single other test)

tasks.withType<Test>().configureEach {
  // JcToolsSecurityManagerTest interferes with JcToolsTest
  setForkEvery(1)
}

If you're fine with that, thats OK by me. OR, I drop the entire test since:

Jacoco did not recognize coverage properly
Tests that interfere with each other and leave the test JVM in some unclean state (yuck)
The old code had a clause for catching NoClassDefFoundError, with a reworked simplified impl I am actually just adding a case to that, and that functionality did not have any test coverage either ;)

rrva · 2022-11-02T19:41:25Z

sorry the tests probably do not leave the test JVM in an unclean state, but if other tests are run concurrently with the JcToolsSecurityManagerTest then it will get ArrayBlockingQueue instead of JcTools queue (a bit randomly since test execution parallelism/ordering is not deterministic)

jack-berg

Just a couple of small comments but looks good otherwise

...haded-deps/src/test/java/io/opentelemetry/sdk/trace/internal/JcToolsSecurityManagerTest.java

.../src/test/java/io/opentelemetry/sdk/trace/internal/SunMiscProhibitedSecurityManagerTest.java

rrva · 2022-11-08T16:54:06Z

is this mergeable?

rrva · 2022-11-08T16:55:54Z

the only thing I am afraid of is that it will degrade performance in these environments without any warning so perhaps a warning should be logged

jack-berg · 2022-11-08T19:34:31Z

the only thing I am afraid of is that it will degrade performance in these environments without any warning so perhaps a warning should be logged

I'm in favor of logging a warning though. Should make sure its only logged once, rather than each time a queue is created.

rrva · 2022-11-08T20:23:44Z

the only thing I am afraid of is that it will degrade performance in these environments without any warning so perhaps a warning should be logged

I'm in favor of logging a warning though. Should make sure its only logged once, rather than each time a queue is created.

Warning added and duplicates mostly avoided (could still happen when creating many queues concurrently)

Rebased.

Good to merge?

sdk/trace-shaded-deps/src/main/java/io/opentelemetry/sdk/trace/internal/JcTools.java

...haded-deps/src/test/java/io/opentelemetry/sdk/trace/internal/JcToolsSecurityManagerTest.java

Some applications run under strict java.security permissions which do not allow access to sun.misc.Unsafe. BatchSpanProcessor uses Unsafe via jctools, but has a fallback to ArrayBlockingQueue. Extending that fallback rule to cover java security exceptions as well.

…y-java into security

…pen-telemetry#4902) Some applications run under strict java.security permissions which do not allow access to sun.misc.Unsafe. BatchSpanProcessor uses Unsafe via jctools, but has a fallback to ArrayBlockingQueue. Extending that fallback rule to cover java security exceptions as well. Co-authored-by: Jack Berg <jberg@newrelic.com>

rrva requested a review from a team October 31, 2022 21:39

trask reviewed Oct 31, 2022

View reviewed changes

rrva force-pushed the security branch from 5c283d9 to 80276dd Compare November 1, 2022 19:57

rrva force-pushed the security branch from a3c79b3 to d5fdd43 Compare November 1, 2022 20:30

rrva force-pushed the security branch 3 times, most recently from da4d961 to 919d292 Compare November 1, 2022 21:32

jkwatson reviewed Nov 2, 2022

View reviewed changes

...haded-deps/src/test/java/io/opentelemetry/sdk/trace/internal/JcToolsSecurityManagerTest.java Outdated Show resolved Hide resolved

jsuereth approved these changes Nov 2, 2022

View reviewed changes

sdk/trace-shaded-deps/build.gradle.kts Outdated Show resolved Hide resolved

rrva force-pushed the security branch from 2f0adb7 to 1e9f265 Compare November 2, 2022 19:22

jack-berg reviewed Nov 3, 2022

View reviewed changes

rrva force-pushed the security branch from bd9807c to a6c2f4f Compare November 3, 2022 23:04

jack-berg approved these changes Nov 3, 2022

View reviewed changes

.../src/test/java/io/opentelemetry/sdk/trace/internal/SunMiscProhibitedSecurityManagerTest.java Outdated Show resolved Hide resolved

rrva force-pushed the security branch from a6c2f4f to 578774e Compare November 4, 2022 20:33

jkwatson approved these changes Nov 8, 2022

View reviewed changes

rrva force-pushed the security branch from 578774e to 593ba2f Compare November 8, 2022 20:22

rrva force-pushed the security branch 2 times, most recently from e07af3e to ed5c0d8 Compare November 8, 2022 20:30

jack-berg reviewed Nov 8, 2022

View reviewed changes

sdk/trace-shaded-deps/src/main/java/io/opentelemetry/sdk/trace/internal/JcTools.java Outdated Show resolved Hide resolved

...haded-deps/src/test/java/io/opentelemetry/sdk/trace/internal/JcToolsSecurityManagerTest.java Show resolved Hide resolved

rrva force-pushed the security branch from 5724b4f to 865f3bd Compare November 9, 2022 21:29

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

511a3ac

…y-java into security

jack-berg merged commit 36adb27 into open-telemetry:main Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow SDK to run in environments prohibiting use of sun.misc.Unsafe #4902

Allow SDK to run in environments prohibiting use of sun.misc.Unsafe #4902

rrva commented Oct 31, 2022 •

edited

Loading

linux-foundation-easycla bot commented Oct 31, 2022 •

edited

Loading

rrva commented Oct 31, 2022

trask left a comment

trask commented Oct 31, 2022

rrva commented Nov 1, 2022

rrva commented Nov 1, 2022

codecov bot commented Nov 1, 2022 •

edited

Loading

rrva commented Nov 1, 2022

rrva commented Nov 1, 2022

jkwatson commented Nov 2, 2022

rrva commented Nov 2, 2022

rrva commented Nov 2, 2022

jack-berg left a comment

rrva commented Nov 8, 2022

rrva commented Nov 8, 2022

jack-berg commented Nov 8, 2022

rrva commented Nov 8, 2022

Allow SDK to run in environments prohibiting use of sun.misc.Unsafe #4902

Allow SDK to run in environments prohibiting use of sun.misc.Unsafe #4902

Conversation

rrva commented Oct 31, 2022 • edited Loading

linux-foundation-easycla bot commented Oct 31, 2022 • edited Loading

rrva commented Oct 31, 2022

trask left a comment

Choose a reason for hiding this comment

trask commented Oct 31, 2022

rrva commented Nov 1, 2022

rrva commented Nov 1, 2022

codecov bot commented Nov 1, 2022 • edited Loading

Codecov Report

rrva commented Nov 1, 2022

rrva commented Nov 1, 2022

jkwatson commented Nov 2, 2022

rrva commented Nov 2, 2022

rrva commented Nov 2, 2022

jack-berg left a comment

Choose a reason for hiding this comment

rrva commented Nov 8, 2022

rrva commented Nov 8, 2022

jack-berg commented Nov 8, 2022

rrva commented Nov 8, 2022

rrva commented Oct 31, 2022 •

edited

Loading

linux-foundation-easycla bot commented Oct 31, 2022 •

edited

Loading

codecov bot commented Nov 1, 2022 •

edited

Loading