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.

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

8292177: InitialSecurityProperty JFR event #10394

Closed
wants to merge 19 commits into from

Conversation

coffeys
Copy link
Contributor

@coffeys coffeys commented Sep 22, 2022

New JFR event to record state of initial security properties.

Debug output is also now added for these properties via -Djava.security.debug=properties


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10394/head:pull/10394
$ git checkout pull/10394

Update a local copy of the PR:
$ git checkout pull/10394
$ git pull https://git.openjdk.org/jdk pull/10394/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10394

View PR using the GUI difftool:
$ git pr show -t 10394

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10394.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 22, 2022

👋 Welcome back coffeys! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 22, 2022
@openjdk
Copy link

openjdk bot commented Sep 22, 2022

@coffeys The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot-jfr
  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org hotspot-jfr hotspot-jfr-dev@openjdk.org labels Sep 22, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 22, 2022

Webrevs

@@ -709,6 +709,11 @@
<setting name="stackTrace">true</setting>
</event>

<event name="jdk.InitialSecurityProperty">
<setting name="enabled">true</setting>
Copy link
Member

Choose a reason for hiding this comment

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

The other security related events are not enabled by default. Is this one enabled because it is only generated once? It seems it may still have some startup overhead because AFAIU it forces a load of security properties even if they are never accessed? Perhaps I don't fully understand how this event works though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - the thinking here is that since this is a one time event, we can have it enabled. It's similar to the InitialSystemProperty event.

We won't force loading of Security properties/class. We shouldn't at least. If no security properties are read in at time of JFR event commit, then we should have no InitialSecurityProperty events. See below/next comment.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, just so I understand, you want to make sure that if JFR is started after the security properties have already been read, then they are still recorded, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct - this type of event period (beginChunk) will fire once when the JFR recording is begun. It should capture Security Properties (if java.security.Security is loaded) for any recording, no matter when it might begin or end. Similar to how InitialSystemProperty is captured (but that's implemented at native/VM level)

Copy link
Member

Choose a reason for hiding this comment

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

How does it capture the event if JFR was started before the security properties were read? I would think you still need some additional code in Security.java to record the properties if the event is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per yesterday's stack trace, JFR triggers loading of the Security class - so your scenario won't arise with current state. We could include the new Event with period of endChunk instead of beingChunk setting. That should ensure the properties are only captured when the JFR recording is exiting.
@egahlin - would you have a preference on this ?

Copy link
Member

Choose a reason for hiding this comment

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

With event streaming, beginChunk is usually to prefer. Otherwise, a client that monitors the JVM must wait until the first chunk rotation to get the data.

That said, we want startup to be quick. There should probably be a common parameter, i.e. security=off/normal/audit/trace, that handles enablement for all security events. I don't know how expensive this event is and where it would fit among those categories?

If the event triggers class loading, it might make sense to check if the event is enabled first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @egahlin - maybe we can leave it at beginChunk setting then.

I've been doing some testing to satisfy myself that the impact of this event on performance is minimal, Running the new emitInitialSecurityProperties() is showing a cost of ~ 1.6ms (1602998 ns).

This new Event itself doesn't trigger extra class loading AFAICT. I went back to a jdk 20 binary without this patch and ran some tests.

ProtectionDomain is a very early class to initialize [1] (initPhase2)

Without JFR, java.security.Security will initialize in a default JDK with a JMX Agent.startLocalManagementAgent call in a simple HelloWorld test which prints "Hello" and then sleeps [2] - the JMX thread starts after about 3 seconds of runtime.

Without JFR and by using the -XX:+DisableAttachMechanism option, the Security class will not load in same test.

If JFR is on, then Security class is already being loaded, even without this patch [3]

[1]

	at java.base/java.security.ProtectionDomain.<clinit>(ProtectionDomain.java:64)
	at java.base/java.lang.ClassLoader.<init>(ClassLoader.java:316)
	at java.base/java.lang.ClassLoader.<init>(ClassLoader.java:431)
	at java.base/java.security.SecureClassLoader.<init>(SecureClassLoader.java:113)
	at java.base/jdk.internal.loader.BuiltinClassLoader.<init>(BuiltinClassLoader.java:194)
	at java.base/jdk.internal.loader.ClassLoaders$BootClassLoader.<init>(ClassLoaders.java:135)
	at java.base/jdk.internal.loader.ClassLoaders.<clinit>(ClassLoaders.java:79)
	at java.base/jdk.internal.loader.BootLoader.loadModule(BootLoader.java:120)
	at java.base/jdk.internal.module.ModuleBootstrap.boot2(ModuleBootstrap.java:266)
	at java.base/jdk.internal.module.ModuleBootstrap.boot(ModuleBootstrap.java:174)
	at java.base/java.lang.System.initPhase2(System.java:2214)

[2]

	at java.base/java.security.Security.<clinit>(Security.java:73)
	at java.base/sun.net.InetAddressCachePolicy$1.run(InetAddressCachePolicy.java:93)
	at java.base/sun.net.InetAddressCachePolicy$1.run(InetAddressCachePolicy.java:90)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:319)
	at java.base/sun.net.InetAddressCachePolicy.<clinit>(InetAddressCachePolicy.java:89)
	at java.base/java.net.InetAddress$NameServiceAddresses.get(InetAddress.java:1005)
	at java.base/java.net.InetAddress.getAllByName0(InetAddress.java:1658)
	at java.base/java.net.InetAddress.getAllByName(InetAddress.java:1524)
	at java.base/java.net.InetAddress.getByName(InetAddress.java:1413)
	at jdk.management.agent/sun.management.jmxremote.ConnectorBootstrap.startLocalConnectorServer(ConnectorBootstrap.java:531)
	at jdk.management.agent/jdk.internal.agent.Agent.startLocalManagementAgent(Agent.java:317)

[3]

	at java.base/java.security.Security.<clinit>(Security.java:73)
	at java.base/sun.security.util.SecurityProperties.getOverridableProperty(SecurityProperties.java:57)
	at java.base/sun.security.util.SecurityProperties.privilegedGetOverridable(SecurityProperties.java:48)
	at java.base/sun.security.util.SecurityProperties.includedInExceptions(SecurityProperties.java:72)
	at java.base/sun.security.util.SecurityProperties.<clinit>(SecurityProperties.java:36)
	at java.base/sun.security.util.FilePermCompat.<clinit>(FilePermCompat.java:43)
	at java.base/java.security.AccessControlContext.<init>(AccessControlContext.java:270)
	at java.base/java.security.AccessController.createWrapper(AccessController.java:649)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:461)
	at jdk.jfr/jdk.jfr.internal.SecuritySupport.doPrivilegedWithReturn(SecuritySupport.java:261)
	at jdk.jfr/jdk.jfr.internal.SecuritySupport.getPathInProperty(SecuritySupport.java:331)
	at jdk.jfr/jdk.jfr.internal.SecuritySupport.<clinit>(SecuritySupport.java:80)
	at jdk.jfr/jdk.jfr.internal.JVMSupport.checkAvailability(JVMSupport.java:46)
	at jdk.jfr/jdk.jfr.internal.JVMSupport.<clinit>(JVMSupport.java:41)
	at jdk.jfr/jdk.jfr.internal.Logger.<clinit>(Logger.java:41)
	at jdk.jfr/jdk.jfr.internal.dcmd.AbstractDCmd.execute(AbstractDCmd.java:75)

Copy link
Member

Choose a reason for hiding this comment

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

When support for the SM is removed, the dependency on AccessController.doPrivileged will be removed and there may no longer be a JFR dependency on loading the Security class. So, it is ok for now, but a solution that doesn't depend on this might be better in the long run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair point. Worse case scenario is that we don't record security properties on start up. Not the case for now though. I've added some extra code to check for this in the test.

@@ -300,4 +308,14 @@ private static void emitDirectBufferStatistics() {
DirectBufferStatisticsEvent e = new DirectBufferStatisticsEvent();
e.commit();
}

private static void emitInitialSecurityProperties() {
Copy link
Member

Choose a reason for hiding this comment

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

Is the Security class loaded and have the properties always been populated at this point? ProtectionDomaindoesn't reference theSecurity` class AFAICT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. JFR does load the Security class [1] via other dependencies but we shouldn't depend on that. I'll add a null check here. If Security class hasn't been loaded, then we shouldn't record any events.

[1]
java.lang.Throwable
        at java.base/java.security.Security.<clinit>(Security.java:73)
        at java.base/sun.security.util.SecurityProperties.getOverridableProperty(SecurityProperties.java:57)
        at java.base/sun.security.util.SecurityProperties.privilegedGetOverridable(SecurityProperties.java:48)
        at java.base/sun.security.util.SecurityProperties.includedInExceptions(SecurityProperties.java:72)
        at java.base/sun.security.util.SecurityProperties.<clinit>(SecurityProperties.java:36)
        at java.base/sun.security.util.FilePermCompat.<clinit>(FilePermCompat.java:43)
        at java.base/java.security.AccessControlContext.<init>(AccessControlContext.java:270)
        at java.base/java.security.AccessController.createWrapper(AccessController.java:649)
        at java.base/java.security.AccessController.doPrivileged(AccessController.java:461)
        at jdk.jfr/jdk.jfr.internal.SecuritySupport.doPrivilegedWithReturn(SecuritySupport.java:261)
        at jdk.jfr/jdk.jfr.internal.SecuritySupport.getPathInProperty(SecuritySupport.java:331)
        at jdk.jfr/jdk.jfr.internal.SecuritySupport.<clinit>(SecuritySupport.java:80)
        at jdk.jfr/jdk.jfr.internal.JVMSupport.checkAvailability(JVMSupport.java:46)
        at jdk.jfr/jdk.jfr.internal.JVMSupport.<clinit>(JVMSupport.java:41)
        at jdk.jfr/jdk.jfr.internal.Logger.<clinit>(Logger.java:41)
        at jdk.jfr/jdk.jfr.internal.dcmd.AbstractDCmd.execute(AbstractDCmd.java:75)

@jaikiran
Copy link
Member

Hello Sean,

Debug output is also now added for these properties via -Djava.security.debug=properties

Looking at the existing code upstream, it appears that the value properties is already recognized for java.security.debug system property. So it's not something this PR is introducing/proposing. However, the documentation for this system property doesn't seem to be listing that as a value https://docs.oracle.com/en/java/javase/19/security/troubleshooting-security.html. Do you think that documentation would need to be addressed (as a separate issue)?

@coffeys
Copy link
Contributor Author

coffeys commented Sep 28, 2022

Hello Sean,

Debug output is also now added for these properties via -Djava.security.debug=properties

Looking at the existing code upstream, it appears that the value properties is already recognized for java.security.debug system property. So it's not something this PR is introducing/proposing. However, the documentation for this system property doesn't seem to be listing that as a value https://docs.oracle.com/en/java/javase/19/security/troubleshooting-security.html. Do you think that documentation would need to be addressed (as a separate issue)?

Good point Jai. It's a rarely used property value I guess, but it would be exposed via the "all" debug value option as per test. I suspect there may be other such properties also. I'll log a JBS issue to track this and to have security/docs team discuss.

@openjdk
Copy link

openjdk bot commented Oct 6, 2022

@coffeys This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8292177: InitialSecurityProperty JFR event

Reviewed-by: mullan

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 127 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 6, 2022
package jdk.jfr.events;

import jdk.jfr.*;
import jdk.jfr.internal.MirrorEvent;
Copy link
Member

Choose a reason for hiding this comment

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

Hello Sean, this MirrorEvent appears to be an unused import. Furthermore, as far as I know, in core-libs the * wildcard imports aren't typically used. I don't know if it's OK to use it here in the jdk.jfr module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Jai - I removed the unused import and reverted to non-wildcard import use

for (Map.Entry<Object, Object> entry : p.entrySet()) {
InitialSecurityPropertyEvent e = new InitialSecurityPropertyEvent();
e.key = (String) entry.getKey();
e.value = (String) entry.getValue();
Copy link
Member

Choose a reason for hiding this comment

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

To avoid any (odd/unexpected) ClassCastException here, perhaps this loop can be changed to something like:

for (Set<String> name : p.stringPropertyNames()) {
    InitialSecurityPropertyEvent e = new InitialSecurityPropertyEvent();
    e.key = name;
    e.value = p.getProperty(name);

Since this code anyway wants to deal with string key/values, this wouldn't introduce any functional change and yet at the same time prevent any unexpected/theoretical ClassCastExceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice - wasn't aware of that method in Properties. Code updated.

private static class JavaSecurityAccessImpl implements JavaSecurityAccess {
static class JavaSecurityAccessImpl implements JavaSecurityAccess {
/* cache a copy for recording purposes */
static Properties initialSecurityProperties;
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look very clean. Could the Security class hold the initial security properties and provide an accessor method that JavaSecurityAccess:getIinitialProperties could use?

Copy link
Member

Choose a reason for hiding this comment

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

Agree, and alternatively, it seems cleaner to add a new SharedSecrets class for java.security.Security and remove the dependency on PD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modified code to have Security class hold the initial properties and provided an accessor method

Copy link
Member

Choose a reason for hiding this comment

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

What about creating a new JavaSecurityPropertiesAccess class and moving the accessor method there? It seems it would be cleaner to remove the dependency on PD in the long run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seanjmullan - I looked at that approach. The SharedSecrets.getJavaSecurityAccess().getInitialProperties(); call may trigger early initialization of the java.security.Security class - I'm not sure if we want that. ProtectionDomain class is currently loaded early in the JDK boot cycle.

In fact the change suggested by @AlanBateman yesterday also has possibility to trigger unnecessary loading of the Security class. I might revert to the original design where we store the cached Properties in ProtectionDomain ?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am missing something. If this JFR event is enabled, and the properties have not yet been accessed, then it seems ok for JFR to load the Security class when JFR is started since the user is interested in this event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My own thoughts here would be that the JFR event system should try and avoid side effects in such areas. If the Security class hasn't been loaded at time of recording, then I'd argue that the number InitialSecurityProperty events should be zero. (which is not possible at the moment since JFR does trigger Security class loading itself anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After further conversation with @seanjmullan , a sharedSecrets accessor for the Properties map should be ok. Edits pushed.


@Category({"Java Development Kit", "Security"})
@Label("Initial Security Property")
@Name("jdk.InitialSecurityProperty")
Copy link
Member

Choose a reason for hiding this comment

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

Should we name this to jdk.InitialSecurityProperties and the label to Initial Security Properties, to be more accurate?

Copy link
Member

Choose a reason for hiding this comment

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

There is one property per event, so it uses the same naming convention as jdk.InitialSystemProperty

Copy link
Member

Choose a reason for hiding this comment

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

You are right indeed - I overlooked the part where this PR loops over these properties and creates one event per property.

@Label("Initial Security Property")
@Name("jdk.InitialSecurityProperty")
@Description("Initial Security Properties")
public final class InitialSecurityPropertyEvent extends AbstractJDKEvent {
Copy link
Member

Choose a reason for hiding this comment

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

The event naming guidelines here https://docs.oracle.com/en/java/javase/17/jfapi/guidelines-naming-and-labeling-events.html recommend leaving out Event from the class name. So, maybe we should call this InitialSecurityProperties?

Copy link
Member

@egahlin egahlin Oct 10, 2022

Choose a reason for hiding this comment

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

The documentation is somewhat misleading. If the event has a name annotation, I think it's fine to call the class Event, because name will override the class name.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you Erik for that detail. Looks fine to me then.

oa.shouldContain("security.provider.2=SunRsaSign")
.shouldNotContain(EXPECTED_DEBUG_OUTPUT)
.shouldNotContain(UNEXPECTED_DEBUG_OUTPUT);
} else{
Copy link
Member

Choose a reason for hiding this comment

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

let's add one space after else

@@ -192,6 +192,7 @@ public class EventNames {
public final static String TLSHandshake = PREFIX + "TLSHandshake";
public final static String X509Certificate = PREFIX + "X509Certificate";
public final static String X509Validation = PREFIX + "X509Validation";
public final static String InitialSecurityProperty = PREFIX + "InitialSecurityProperty";
Copy link
Member

Choose a reason for hiding this comment

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

Let's unify order of modifiers in this file. Some fields use public final static and some public static final

@@ -63,6 +64,9 @@ public final class Security {
/* The java.security properties */
private static Properties props;

/* cache a copy for recording purposes */
static Properties initialSecurityProperties;
Copy link
Member

Choose a reason for hiding this comment

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

This can be private now.

@@ -162,6 +181,10 @@ private static boolean loadProps(File masterFile, String extraPropFile, boolean
}
}

static Properties getInitialSecurityProperties() {
Copy link
Member

Choose a reason for hiding this comment

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

Don't think we need this method anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - did another pass through the edits and caught these. Patch updated.

[Use blessed modifier order in EventNames]

[remove previous edit]

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Oct 13, 2022
@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 13, 2022
@coffeys
Copy link
Contributor Author

coffeys commented Oct 18, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Oct 18, 2022

Going to push as commit 8c40b7d.
Since your change was applied there have been 181 commits pushed to the master branch:

  • e7a964b: 8295268: Optimized builds are broken due to incorrect assert_is_rfp shortcuts
  • 0b7d811: 8294730: Add @throws and @implNote clauses to BigInteger::isProblablePrime and BigInteger::nextProblablePrime
  • 4434cbb: 8295264: Fix PaX check on RISC-V
  • a8c18eb: 8295257: Remove implicit noreg temp register arguments in aarch64 MacroAssembler
  • 6553065: 8295273: Remove unused argument in [load/store]_sized_value on aarch64 and riscv
  • b06f1b1: 8294594: Fix cast-function-type warnings in signal handling code
  • 71aa821: 8295176: some langtools test pollutes source tree
  • bca7ab3: 8295412: support latest VS2022 MSC_VER in abstract_vm_version.cpp
  • c33ca0c: 6229853: BasicTextAreaUI:create incompletely documents the possible returned View types
  • 358ac07: 8294254: [macOS] javax/swing/plaf/aqua/CustomComboBoxFocusTest.java failure
  • ... and 171 more: https://git.openjdk.org/jdk/compare/3644e26cef71c00e1a2638d2b8bed9c1bda965ca...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 18, 2022
@openjdk openjdk bot closed this Oct 18, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 18, 2022
@openjdk
Copy link

openjdk bot commented Oct 18, 2022

@coffeys Pushed as commit 8c40b7d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org hotspot-jfr hotspot-jfr-dev@openjdk.org integrated Pull request has been integrated security security-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.

6 participants