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

8254711: Add java.security.Provider.getService JFR Event #9657

Closed
wants to merge 10 commits into from

Conversation

coffeys
Copy link
Contributor

@coffeys coffeys commented Jul 27, 2022

Add a JFR Event for java.security.Provider.getService(String type, String algorithm) calls.


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

  • JDK-8254711: Add java.security.Provider.getService JFR Event

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9657

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 27, 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
Copy link

openjdk bot commented Jul 27, 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 Jul 27, 2022
@coffeys coffeys changed the title 8254711: Add JFR events for security crypto algorithms 8254711: Add java.security.Provider.getService JFR Event Sep 14, 2022
@coffeys coffeys marked this pull request as ready for review September 14, 2022 17:01
@coffeys
Copy link
Contributor Author

coffeys commented Sep 14, 2022

Reviewer request: @seanjmullan @egahlin

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

mlbridge bot commented Sep 14, 2022

Webrevs

Copy link
Member

@seanjmullan seanjmullan left a comment

Choose a reason for hiding this comment

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

Looks good.

@openjdk
Copy link

openjdk bot commented Sep 19, 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:

8254711: Add java.security.Provider.getService JFR Event

Reviewed-by: mullan, valeriep, jpai

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 118 new commits pushed to the master branch:

  • e195897: 8294068: Unconditional and eager load of nio library since JDK-8264744
  • 84d7ff6: 8288129: Shenandoah: Skynet test crashed with iu + aggressive
  • 07afa3f: 8294110: compiler/uncommontrap/Decompile.java fails after JDK-8293798
  • 0746bcb: 8294083: RISC-V: Minimal build failed with --disable-precompiled-headers
  • 95ec2ea: 8293897: Synthetic final modifier is part of the AST for a try-with-resource resource
  • d14e96d: 8293493: Signal Handlers printout should show signal block state
  • da4fdfb: 8293659: Improve UnsatisfiedLinkError error message to include dlopen error details
  • cd1cdcd: 8293116: Incremental JDK build could be sped up
  • e9401e6: 8293364: IGV: Refactor Action in EditorTopComponent and fix minor bugs
  • 844a95b: 8292892: Javadoc index descriptions are not deterministic
  • ... and 108 more: https://git.openjdk.org/jdk/compare/9ef6c0925ae5a0ca774b23f6318551417a53e6c6...master

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 Sep 19, 2022
import jdk.jfr.events.VirtualThreadSubmitFailedEvent;
import jdk.jfr.events.X509CertificateEvent;
import jdk.jfr.events.X509ValidationEvent;
import jdk.jfr.events.*;
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, rest of the changes look fine to me, except this one. Was this an intentional change to use * import instead of the explicit ones?

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 was formatted via IDE. Seems like you've a preference to use the more verbose import style. I'll revert this change.

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 for pointing out that test issue also. I missed it. I'll push a minor edit to correct.

@jaikiran
Copy link
Member

The failures reported in the GitHub Actions job look related to this change - jdk.jfr.event.metadata.TestDefaultConfigurations test appears to be failing:

java.lang.Exception: Setting 'threshold' in event 'jdk.SecurityProviderService' was not configured in the configuration 'default'
Setting 'threshold' in event 'jdk.SecurityProviderService' was not configured in the configuration 'profile'

	at jdk.jfr.event.metadata.TestDefaultConfigurations.throwExceptionWithErrors(TestDefaultConfigurations.java:115)
	at jdk.jfr.event.metadata.TestDefaultConfigurations.main(TestDefaultConfigurations.java:76)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
	at java.base/java.lang.reflect.Method.invoke(Method.java:578)
	at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:125)
	at java.base/java.lang.Thread.run(Thread.java:1589)

JavaTest Message: Test threw exception: java.lang.Exception: Setting 'threshold' in event 'jdk.SecurityProviderService' was not configured in the configuration 'default'
Setting 'threshold' in event 'jdk.SecurityProviderService' was not configured in the configuration 'profile'

@egahlin
Copy link
Member

egahlin commented Sep 19, 2022

I noticed that the event is disabled by default and in profile settings.

Is it because of concerns of too many events, or too much overhead? Or something else?

I think we should strive to have events always on. Few users will take the time to learn about the event and enable it. Usage will probably drop by 99%, if not enabled by default.

If concerns are performance related, there might be other event designs that could be used, for example, a periodic event that is always enabled.

legacyMap.remove(key, s);
} else {
return s;
if (s != null && SecurityProviderServiceEvent.isTurnedOn()) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be useful to generate an event even for the cases where a call to this method was made but no service was available and null was returned? The event perhaps could capture that there was no service found for such type/algorithm combination? That would help identify usages in applications where there might be fallbacks being used when this method returns null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had this as the original design actually. I'm not sure how interesting it would be to record such "no-service" type events. It would probably add 2-4 times the number of events for this event type to a typical recording, given that the framework iterates over the providers in preferential order.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think this would generate too much noise and detract from the main motivation for these events, which is to help users analyze the security of algorithms that are being used by their applications at the JCE layer.

Choose a reason for hiding this comment

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

Plus one regarding "too much noise". This event is at the Provider.getService() level and would reports all calls regardless the type and algorithm. Crypto services which supports the delayed provider selection may call Provider.getService() to query but may not use all available ones. So, even if the service is returned, it may not be actually used. Just saying.
Does JFR events support filtering? Or is the expectation of this being a collection of usages and analyze is done separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's stick to just recording events where a match on service and provider is made then.

The expectation is that analysis would be done post recording. Probably best to reduce the runtime performance impact.

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 for noting the reason, Sean. It's reasonable to only generate an event for a match.

@coffeys
Copy link
Contributor Author

coffeys commented Sep 19, 2022

This new event is disabled by default just like the other crypto related events that were added some time back (e.g. TLSHandshakeEvent). My assumption is that these events will be enabled for audit mode when one is interested in finding out what their crypto operations looks like. Periodic events are good but for such operations I think it's critical that we capture all such crypto calls to ensure that admins get a good picture of what's been used/called.

Perhaps we can have a discussion about whether such JDK events should be on by default and work the issue via another JBS issue. jdk.Deserialization is another example of a JDK level event that's off by default.

@egahlin
Copy link
Member

egahlin commented Sep 19, 2022

This new event is disabled by default just like the other crypto related events that were added some time back (e.g. TLSHandshakeEvent). My assumption is that these events will be enabled for audit mode when one is interested in finding out what their crypto operations looks like. Periodic events are good but for such operations I think it's critical that we capture all such crypto calls to ensure that admins get a good picture of what's been used/called.

I remember we had this discussion a few years back and maybe we need both? Some events for audits and some that can be detected by the JMC rules engine in a normal recording?

Perhaps it's time to introduce a parameterized setting, similar to what we have for GC and compiler, where user can specify levels. From JDK 17, they can be specified on command line, i.e.

$ java -XX:StartFlightRecording:security=off/normal/debug/audit

This would lower the bar and make the events useful to more people.

We can do this in another JBS issue.

Copy link

@valeriepeng valeriepeng left a comment

Choose a reason for hiding this comment

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

The java.base part of changes look good.

@coffeys
Copy link
Contributor Author

coffeys commented Sep 22, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Sep 22, 2022

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

  • d781ab0: 8294003: Don't handle si_addr == 0 && si_code == SI_KERNEL SIGSEGVs
  • a216960: 8294087: RISC-V: RVC: Fix a potential alignment issue and add more alignment assertions for the patchable calls/nops
  • 3fa6778: 8292296: Use multiple threads to process ParallelGC deferred updates
  • 800e68d: 8292044: HttpClient doesn't handle 102 or 103 properly
  • 83abfa5: 8255670: Improve C2's detection of modified nodes
  • 5652030: 8292376: A few Swing methods use inheritDoc on exceptions which are not inherited
  • 03f287d: 8293995: Problem list sun/tools/jstatd/TestJstatdRmiPort.java on all platforms because of 8293577
  • d5bee4a: 8294086: RISC-V: Cleanup InstructionMark usages in the backend
  • 47f233a: 8292202: modules_do is called without Module_lock
  • 742bc04: 8294100: RISC-V: Move rt_call and xxx_move from SharedRuntime to MacroAssembler
  • ... and 123 more: https://git.openjdk.org/jdk/compare/9ef6c0925ae5a0ca774b23f6318551417a53e6c6...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Sep 22, 2022

@coffeys Pushed as commit bc2af47.

💡 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
5 participants