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

8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 #7796

Closed
wants to merge 14 commits into from

Conversation

bradfordwetmore
Copy link
Contributor

@bradfordwetmore bradfordwetmore commented Mar 12, 2022

JDK-8253368 changed the behavior of SSLSocket to no longer throw a fatal internal_error (80) and invalidate existing sessions (either completed or under construction) as described in (RFC 4346/TLSv1.1+) if a connection was closed without receiving a close_notify alert from the peer.

This change introduces similar behavior to SSLEngine.

The unit test checks that closing the read(input) sides of the SSLSocket/SSLEngine throws an SSLException, but doesn't invalidate their respective sessions.

Tier1/2 mach5 tests have been successfully run.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7796/head:pull/7796
$ git checkout pull/7796

Update a local copy of the PR:
$ git checkout pull/7796
$ git pull https://git.openjdk.java.net/jdk pull/7796/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7796

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7796.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 12, 2022

👋 Welcome back wetmore! 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 Mar 12, 2022

@bradfordwetmore 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 Mar 12, 2022
@bradfordwetmore bradfordwetmore marked this pull request as ready for review Mar 18, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 18, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 18, 2022

Webrevs

Copy link
Member

@XueleiFan XueleiFan left a comment

Looks good to me. Thanks!

@openjdk
Copy link

openjdk bot commented Mar 21, 2022

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

8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368

Reviewed-by: xuelei, rhalade, coffeys

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 Mar 21, 2022
/*
* The following is to set up the keystores/trust material.
*/
private static final String pathToStores = "../../../../javax/net/ssl/etc";
Copy link
Contributor

@coffeys coffeys Mar 21, 2022

Choose a reason for hiding this comment

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

I think the advise is to move away from binary style keystores. i.e. to use test/jdk/javax/net/ssl/templates/SSLContextTemplate.java for a replacement. Is that possible here ?

Copy link
Contributor Author

@bradfordwetmore bradfordwetmore Mar 22, 2022

Choose a reason for hiding this comment

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

Sigh...this is a whole can of worms I wasn't expecting. Looks like one person did the SSLContextTemplate and updated with SSLEngineTemplate, then another person took a completely different takes with SSLSocketTemplate, and then SSLSocketSSLEngineTemplate didn't get touched at all.

This should really be harmonized. :(

Copy link
Contributor

@coffeys coffeys Mar 22, 2022

Choose a reason for hiding this comment

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

ouch - maybe this should be fixed up in a separate bug then. Don't think it should be a blocker for this fix

Copy link
Contributor Author

@bradfordwetmore bradfordwetmore Mar 23, 2022

Choose a reason for hiding this comment

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

Assessing how much work it will be.

Copy link
Contributor Author

@bradfordwetmore bradfordwetmore Apr 2, 2022

Choose a reason for hiding this comment

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

@coffeys suggested using the flat text keystore (non-binary) that has been developed for SSLSocketTemplate/SSLEngineTemplate. The SSLSocketTemplate, SSLEngineTemplate and SSLSocketSSLEngineTemplate testbeds have diverged broadly (as noted in JDK-8284047), so this would not be an easy fix. In the interest of time, I'll use the existing template, and hope someone can address JDK-8284047 for future issues.

It's not ideal, but as many existing tests still use the binary keystores, so this will just be one more.

@@ -0,0 +1,491 @@
/*
* Copyright (c) 2011, 2022, Oracle and/or its affiliates. All rights reserved.
Copy link
Member

@rhalade rhalade Mar 21, 2022

Choose a reason for hiding this comment

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

should this be updated to only list 2022?

Copy link
Contributor Author

@bradfordwetmore bradfordwetmore Mar 22, 2022

Choose a reason for hiding this comment

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

In previous days, we used to include the dates from the templates, which this test was derived from. I could go either way.

Copy link
Member

@rhalade rhalade Mar 22, 2022

Choose a reason for hiding this comment

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

I am not knowledgable one in this area and thought was merely a typo. Fine either way.

Copy link
Contributor Author

@bradfordwetmore bradfordwetmore Mar 23, 2022

Choose a reason for hiding this comment

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

I'll leave for now.

} finally {
engineLock.unlock();
conContext.closeInbound();
engineLock.unlock();
Copy link
Member

@dfuch dfuch Mar 23, 2022

Choose a reason for hiding this comment

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

I see that onContext.closeInbound() might throw, which would leave the engineLock locked and could cause deadlocks down the road. So maybe you should have a nested try { } finally { } here to make sure the lock is properly unlocked.

Copy link
Member

@XueleiFan XueleiFan Mar 23, 2022

Choose a reason for hiding this comment

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

+1.

Copy link
Contributor Author

@bradfordwetmore bradfordwetmore Mar 23, 2022

Choose a reason for hiding this comment

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

Good catch. Thanks.

conContext.closeInbound();
} finally {
engineLock.unlock();
}
Copy link
Member

@XueleiFan XueleiFan Mar 23, 2022

Choose a reason for hiding this comment

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

I looked the update further. It would be nice if the try-statement could support more than one finally blocks in the future so that the code could be more clear.

try {
    ...
} finally {
   // the 1st final block
} finally {
   // the 2nd final block
}

For the block from 781 to 787, it may be more effective if moving the block out of the synchronization lock (that's, moving 781-787 to line 779.)

Copy link
Contributor Author

@bradfordwetmore bradfordwetmore Mar 24, 2022

Choose a reason for hiding this comment

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

@XueleiFan, I'm not following your first comment. AFAIK, double finally blocks aren't currently an option, unless I'm missing a new language feature. Or is this just a general "it would be nice if the Java language was able to do..." comment?

        try {
            ...
            throw new SSLException(...);
        } finally {
            conContext.closeInbound();
        } finally {
            engineLock.unlock();
        }

As to your second question, the model of:

method() {
    engineLock.lock();
    try {
        action();
    } finally {
        engineUnlock.unlock();
    }
}

is very simple to understand and thus maintain, and is used quite consistently throughout SSLEngineImpl and catches any problems. IIUC, you are suggesting:

    public void closeInbound() throws SSLException {
        if (isInboundDone()) {
            return;
        }

        if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
            SSLLogger.finest("Closing inbound of SSLEngine");
        }

        engineLock.lock();
        try {
            if (!conContext.isInputCloseNotified && (conContext.isNegotiated
                    || conContext.handshakeContext != null)) {
                throw new SSLException(
                        "closing inbound before receiving peer's close_notify");
            }
        } finally {
            try {
                conContext.closeInbound();
            } finally {
                engineLock.unlock();
            }
        }
    }

What is the advantage to changing to this style, other than a very small performance win by not locking in the already closed case? Isn't the locking code pretty optimized? SSLLogger might throw a NPE (unlikely), and isInboundDone() just checks a variable, but the code could change down the road. I'm just not seeing a reason to change the maintainability of this code.

Copy link
Member

@XueleiFan XueleiFan Mar 24, 2022

Choose a reason for hiding this comment

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

@XueleiFan, I'm not following your first comment.

Sorry for that. I was wondering something new in the future, which is not doable in the current Java language. Please just leave it alone.

Copy link
Contributor Author

@bradfordwetmore bradfordwetmore Mar 24, 2022

Choose a reason for hiding this comment

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

Ah! Thanks.

Copy link
Member

@XueleiFan XueleiFan Mar 24, 2022

Choose a reason for hiding this comment

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

IIUC, you are suggesting:
... snipped ...

Yes.

What is the advantage to changing to this style, other than a very small performance win by not locking in the already closed case?

The advantage is mainly about the performance, as I/O of SSLLogger could be slow. The SSLSocketImpl code does not put the similar block locked. But it may be not the purpose of this update. I'm fine if you want to leave it alone.

Copy link
Contributor Author

@bradfordwetmore bradfordwetmore Apr 2, 2022

Choose a reason for hiding this comment

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

I can see your point for these two methods (close*bound()), but we are using SSLLogger behind the engineLock throughout the implementation. I think I will leave as is, rather than try to squeak this little bit out when logging is on and active.

@bradfordwetmore
Copy link
Contributor Author

bradfordwetmore commented Apr 2, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Apr 2, 2022

Going to push as commit 0b09f70.

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

openjdk bot commented Apr 2, 2022

@bradfordwetmore Pushed as commit 0b09f70.

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

@bradfordwetmore bradfordwetmore deleted the JDK-8273553 branch Apr 7, 2022
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
5 participants