Skip to content
This repository has been archived by the owner on Apr 24, 2023. It is now read-only.
/ jdk20 Public archive

8299891: JMX ObjectInputFilter additional classes needed #97

Closed
wants to merge 9 commits into from

Conversation

kevinjwalls
Copy link
Contributor

@kevinjwalls kevinjwalls commented Jan 11, 2023

The default setting for the ObjectInputFilter for JMX, introduced in jdk20, is too restrictive.
javax.management.Attribute and AttributeList classes are also needed, and Query related classes.

There are a number of Query-related classes, so adding javax.management.* is appropriate otherwise the list becomes hard to manage. This is a * and not a ** which would mean all subpackages, so the openmean subpackage stays in the list.


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
  • Change requires CSR request JDK-8300581 to be approved

Issues

  • JDK-8299891: JMX ObjectInputFilter additional classes needed
  • JDK-8300581: JMX ObjectInputFilter additional classes needed (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 97

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk20/pull/97.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 11, 2023

👋 Welcome back kevinw! 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.

@kevinjwalls kevinjwalls marked this pull request as ready for review January 11, 2023 09:42
@openjdk
Copy link

openjdk bot commented Jan 11, 2023

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

  • jmx
  • serviceability

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 serviceability serviceability-dev@openjdk.org jmx jmx-dev@openjdk.org labels Jan 11, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 11, 2023
@mlbridge
Copy link

mlbridge bot commented Jan 11, 2023

Webrevs

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

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

I believe it would be good to add a test method that registers for notifications using a notification filter. Especially attribute change notification, possibly MBean registration too.

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

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

Thanks for adding the new testcases, especially WRT to notifications.
Your filter might be a little wider than strictly required but it looks like a good first step, and is obviously better than no filter. I am approving on the condition that all JMX (and JCK?) tests are stable passing.
Please obtain approval from a maintainer of this area before pushing.

@openjdk
Copy link

openjdk bot commented Jan 18, 2023

@kevinjwalls 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:

8299891: JMX ObjectInputFilter additional classes needed

Reviewed-by: dfuchs, sspitsyn, cjplummer

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 39 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 ready Pull request is ready to be integrated csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated csr Pull request needs approved CSR before integration labels Jan 18, 2023
@kevinjwalls
Copy link
Contributor Author

Thanks Daniel. Yes, one problem is that tests were passing with the more restrictive filter. With the test update here, at least we have a failure with the too-restrictive filter and a pass with this change.
JCK: api/javax_management/ also looks good.

java.rmi.MarshalledObject;\
java.rmi.dgc.*;\
java.rmi.server.*;\
javax.security.auth.Subject;!*
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the ! at the end indicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's "not everything else".

https://docs.oracle.com/en/java/javase/19/core/serialization-filtering1.html

"If a class name doesn’t match any filter, then it is allowed. If you want to allow only certain class names, then your filter must reject everything that doesn’t match. To reject all class names other than those specified, include !* as the last pattern in a class filter."

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. It would be good to clarify that in the comment above this filter. Also, maybe put it on a new line. Otherwise at first glance it appears to have a relationship to the class immediately before it.

Does this mean that this filter list would serve no purpose if the !* was omitted? I'm just curious as to why the !* is needed rather than it just being default behavior that a class has to match a filter in the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a class is not matched, it is "undecided", mentioned at the end of the long comment. That is not a rejection, which that existing long comment does not state. For an actual rejection, we need the ! to match, so patterns generally end in !*

It's the same or very similar comment as in conf/security/java.security

I added a note about the !* at the end to clarify, as it is new to use the filter in this area, and yes put it on a new line.

Copy link

@sspitsyn sspitsyn left a comment

Choose a reason for hiding this comment

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

This fix looks okay to me.
One nit was inlined.
Thanks,
Serguei

Copy link

@sspitsyn sspitsyn left a comment

Choose a reason for hiding this comment

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

Thank you for the update, Kevin!
Thanks,
Serguei

@kevinjwalls
Copy link
Contributor Author

Thanks for the suggestions and reviews, will integrate.

@kevinjwalls
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Feb 1, 2023

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

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Feb 1, 2023

@kevinjwalls Pushed as commit 225f805.

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
integrated Pull request has been integrated jmx jmx-dev@openjdk.org serviceability serviceability-dev@openjdk.org
4 participants