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

JDK-8291974 PrivateCredentialPermission should not use local variable to enable debugging #10206

Closed
wants to merge 7 commits into from

Conversation

mcpowers
Copy link
Contributor

@mcpowers mcpowers commented Sep 7, 2022

https://bugs.openjdk.org/browse/JDK-8291974


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 a CSR request to be approved

Issues

  • JDK-8291974: PrivateCredentialPermission should not use local variable to enable debugging
  • JDK-8293653: PrivateCredentialPermission should not use local variable to enable debugging (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10206

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

Using diff file

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

Sorry, something went wrong.

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 7, 2022

👋 Welcome back mpowers! 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 Sep 7, 2022

@mcpowers The following label will be automatically applied to this pull request:

  • security

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

@openjdk openjdk bot added the security security-dev@openjdk.org label Sep 7, 2022
@wangweij
Copy link
Contributor

wangweij commented Sep 9, 2022

Code change looks good to me. Since a serialVersionUID is already defined and testing is always false, removing it is not a compatibility issue.

However, now that the serialized form is modified (see https://download.java.net/java/early_access/jdk20/docs/api/serialized-form.html#javax.security.auth.PrivateCredentialPermission), I wonder if we need a small CSR.

@mcpowers
Copy link
Contributor Author

mcpowers commented Sep 9, 2022

What happens when deserialization encounters a missing field like testing? Does it ignore it?

@wangweij
Copy link
Contributor

wangweij commented Sep 9, 2022

Assign it a default value, false.

@mcpowers mcpowers marked this pull request as ready for review September 12, 2022 16:30
@openjdk openjdk bot added csr Pull request needs approved CSR before integration rfr Pull request is ready for review labels Sep 12, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 12, 2022

Webrevs

@seanjmullan
Copy link
Member

I would write a test which serializes the data (before your change) and deserializes it after. There should be some regression tests that already do that.

@wangweij
Copy link
Contributor

In the problem section part of the CSR, add a sentence on what's wrong with the field.

In the spec section, please only keep the removal of the testing field. All others are implementation changes and not part of spec.

In the compatibility reason, add a sentence on why removing the field has no actual effect on serialization and deserialization.

@mcpowers
Copy link
Contributor Author

@seanjmullan There is an existing test which serializes and deserializes a PrivateCredentialPermission object. It only checks that the deserialized object is same as the original. There is no testing for new or deleted fields. The Java Serialization Spec indicates that the testing variable can be safely removed. This is only possible because the variable is boolean and is initialized to false. The CSR has more details. I have verified this behavior with a manual test of my own.

Are you suggesting that I write and integrate a new test that basically verifies that the Java Serialization Spec works as documented?

@wangweij
Copy link
Contributor

wangweij commented Sep 15, 2022

I've added my name as a reviewer of the CSR.

As for the test, I think it makes sense to serialize the object in an old JDK, hardcode the bytes in a new test and try to deserialize it, and make sure it equals to an object that is freshly constructed.

For the opposite direction, one can write a test in the JDK 17 repo that deserializes a byte array "from the future". This sounds fascinating but probably not worth doing.

@valeriepeng
Copy link
Contributor

valeriepeng commented Sep 15, 2022

What happens when deserialization encounters a missing field like testing? Does it ignore it?

Have we tried this to know for sure that it is ignored? I think you should verify this by serializing w/o this "testing" field and de-serialize it with older releases.
Never mind the above comment, I just saw your other comment that this has been verified.

@mcpowers
Copy link
Contributor Author

I wrote a test to deserialize an object created and serialized before the fix. I compare the old object with the current object after the fix. What is important is not the comparison, but that deserialization doesn't throw an exception.

@valeriepeng
Copy link
Contributor

I wrote a test to deserialize an object created and serialized before the fix. I compare the old object with the current object after the fix. What is important is not the comparison, but that deserialization doesn't throw an exception.

Does both de-serialization pass, i.e. deserialize the new bytes using old release and deserializing the old bytes using new release?

@mcpowers
Copy link
Contributor Author

I tested old bytes on new release. It passes. I didn't test new bytes on an old release because of Max's "from the future" comment. Honestly, I'm not sure what any of this testing proves. Maybe that the Java Object Serialization Specification isn't broken?

@wangweij
Copy link
Contributor

wangweij commented Sep 19, 2022

It's still worth manually testing the opposite direction at least once on your own machine, I just meant it's not worth adding this test into our code repository since it must run with a different JDK.

I agree the test should pass if the Java Object Serialization Specification isn't broken, but sometimes our tests are able to expose problems in other areas.

@mcpowers
Copy link
Contributor Author

Fair enough. I'll run a test in the opposite direction.

@valeriepeng
Copy link
Contributor

I tested old bytes on new release. It passes. I didn't test new bytes on an old release because of Max's "from the future" comment. Honestly, I'm not sure what any of this testing proves. Maybe that the Java Object Serialization Specification isn't broken?

Supposedly if the newer bytes doesn't work with the older releases, it is a compatibility format change and you should change the serialVersionID value to indicate it. In this case, some may wonder if it's worthwhile to remove this field.

@RogerRiggs
Copy link
Contributor

As mentioned earlier in the comments...
This is a completely compatible change. Removing a field is well specified as a compatible change.
From old version to new version the value in the stream is discarded.
From a new version to the old version, the value in the stream is missing so the field is initialized to its default value (false for boolean).
There's no reason to change the serialVersionUID. Changing it would be an incompatible change and cause all kinds of other problems.

In the current case, since the value of the field is false, it will be hard to observe any change in behavior.

@mcpowers
Copy link
Contributor Author

I ran the test in the reverse direction. No difference in behavior was observed.

@valeriepeng
Copy link
Contributor

I ran the test in the reverse direction. No difference in behavior was observed.

Great, thanks for confirming.

@valeriepeng
Copy link
Contributor

As mentioned earlier in the comments... This is a completely compatible change. Removing a field is well specified as a compatible change. From old version to new version the value in the stream is discarded. From a new version to the old version, the value in the stream is missing so the field is initialized to its default value (false for boolean). There's no reason to change the serialVersionUID. Changing it would be an incompatible change and cause all kinds of other problems.

In the current case, since the value of the field is false, it will be hard to observe any change in behavior.

Thanks for the comments, just want to be sure the old version handles the missing field, that's all.

* Base64 encoding of Serialized PrivateCredentialPermission object
* before bug fix for JDK-8291974.
*/
static String before = "rO0ABXNyAC9qYXZheC5zZWN1cml0eS5hdXRoLlByaXZhdGVDcmVkZW50aWFsUGVybWlzc2lvbklV3Hd7UH9MAgADWgAHdGVzdGluZ0wAD2NyZWRlbnRpYWxDbGFzc3QAEkxqYXZhL2xhbmcvU3RyaW5nO0wACnByaW5jaXBhbHN0AA9MamF2YS91dGlsL1NldDt4cgAYamF2YS5zZWN1cml0eS5QZXJtaXNzaW9uscbhPyhXUX4CAAFMAARuYW1lcQB+AAF4cHQAGWNyZWQxIHBjMSAicG4xIiBwYzIgInBuMiIAdAAFY3JlZDFw";
Copy link
Contributor

Choose a reason for hiding this comment

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

The line is a little too long. You can break it into multiple lines and use Base64.getMimeDecoder() to decode it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or use string concatenation, e.g. "xxx" + "yyy".

Copy link
Member

Choose a reason for hiding this comment

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

This looks like a good candidate for Text Blocks: https://openjdk.org/jeps/378

Copy link
Contributor Author

Choose a reason for hiding this comment

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

        String a = """
            xxx
            yyy
        """;

is not the same as

        String a =
            "xxx" +
            "yyy"
        ;

a contains an embedded newline; b does not.

Copy link

Choose a reason for hiding this comment

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

@mcpowers the idea would be to use

String a = """
        xxx\
        yyy\
    """;

Copy link
Contributor

Choose a reason for hiding this comment

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

Either is OK. That's why I proposed getMimeDecoder that will skip the newline characters.

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'll go with Text Blocks and Florent's suggestion, but thanks for mentioning getMimeDecoder. I'm sure I'll find a use for it in the future.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Oct 4, 2022
ObjectInputStream ois = new ObjectInputStream(is);
PrivateCredentialPermission pcp2 =
(PrivateCredentialPermission)ois.readObject();
is.close();
Copy link
Member

Choose a reason for hiding this comment

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

Not necessary as BAIS.close() is a no-op.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

InputStream is = new ByteArrayInputStream(decoded);

// Deserialize input stream and create a new object.
ObjectInputStream ois = new ObjectInputStream(is);
Copy link
Member

Choose a reason for hiding this comment

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

Use try-with resources here so the input stream is automatically closed even if there are exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using try-with resources

System.out.println("Serial2 test succeeded");
} catch (Exception e) {
e.printStackTrace();
throw new SecurityException("Serial test failed");
Copy link
Member

Choose a reason for hiding this comment

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

If you include e as the cause (2nd argument) of SecurityException then you don't need to print the stack trace on line 78.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

* @bug 8291974
* @summary PrivateCredentialPermission should not use local variable to enable debugging
* implementation-dependent class
* @run main/othervm/policy=Serial.policy Serial2
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove main/othervm/policy=Serial.policy (actually you can remove the whole @run line then).
The policy argument causes the test to run with a SecurityManager enabled, and there isn't any reason that this test needs to do that AFAICT. Also that policy file is for other tests in this directory for accessing the file system or JAAS credentials, which you are not accessing in this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

import java.io.*;
import java.util.*;

public class Serial2 implements java.io.Serializable {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see why this needs to implement Serializable (I see you probably used the other Serial.java test in this directory as a template. I don't think that needs it either, but you can leave that as is for now).

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 read your sentence as "leave both for now". Sorry. You mean fix in this PR and leave the other alone.

@seanjmullan
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Oct 31, 2022

@seanjmullan The change author (@mcpowers) must issue an integrate command before the integration can be sponsored.

@seanjmullan
Copy link
Member

Looks good. Go ahead an integrate and I will sponsor.

@mcpowers
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Oct 31, 2022

@mcpowers This pull request has not yet been marked as ready for integration.

@seanjmullan
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Oct 31, 2022

@seanjmullan The change author (@mcpowers) must issue an integrate command before the integration can be sponsored.

@openjdk
Copy link

openjdk bot commented Oct 31, 2022

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

8291974: PrivateCredentialPermission should not use local variable to enable debugging

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 23 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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@seanjmullan) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

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

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 31, 2022
@openjdk
Copy link

openjdk bot commented Oct 31, 2022

@mcpowers
Your change (at version ce27a08) is now ready to be sponsored by a Committer.

@seanjmullan
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Oct 31, 2022

Going to push as commit 8480f87.
Since your change was applied there have been 23 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 Oct 31, 2022
@openjdk openjdk bot closed this Oct 31, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Oct 31, 2022
@openjdk
Copy link

openjdk bot commented Oct 31, 2022

@seanjmullan @mcpowers Pushed as commit 8480f87.

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

@mlbridge
Copy link

mlbridge bot commented Nov 1, 2022

Mailing list message from Sean Mullan on security-dev:

On 10/31/22 11:02 AM, Mark Powers wrote:

On Mon, 10 Oct 2022 21:04:46 GMT, Sean Mullan <mullan at openjdk.org> wrote:

Mark Powers has updated the pull request incrementally with one additional commit since the last revision:

Sean comments

test/jdk/javax/security/auth/PrivateCredentialPermission/Serial2.java line 35:

33: import java.util.*;
34:
35: public class Serial2 implements java.io.Serializable {

I don't see why this needs to implement `Serializable` (I see you probably used the other `Serial.java` test in this directory as a template. I don't think that needs it either, but you can leave that as is for now).

I read your sentence as "leave both for now". Sorry. You mean fix in this PR and leave the other alone.

Yes, I meant remove it from this test if it isn't needed. I think you
can fix the other one, if you like, but since it hasn't caused any
issues, feel free to leave it for now.

--Sean

@mcpowers mcpowers deleted the JDK-8291974 branch November 1, 2022 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated security security-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.

None yet

6 participants