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

8259707: LDAP channel binding does not work with StartTLS extension #2085

Closed
wants to merge 4 commits into from

Conversation

@alexeybakhtin
Copy link

@alexeybakhtin alexeybakhtin commented Jan 14, 2021

Please review a small patch to enable LDAP TLS Channel Binding with StartTLS Extension.
Test from the bug report and jtreg javax/naming tests are passed.


Progress

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

Issue

  • JDK-8259707: LDAP channel binding does not work with StartTLS extension

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2085/head:pull/2085
$ git checkout pull/2085

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 14, 2021

👋 Welcome back abakhtin! 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 label Jan 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 14, 2021

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

  • core-libs

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 core-libs label Jan 14, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 14, 2021

@seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Jan 19, 2021

Can you add a test for this or is it too hard? There are existing tests for StartTLS in the security/infra area -- could they be enhanced to test this case?

@alexeybakhtin
Copy link
Author

@alexeybakhtin alexeybakhtin commented Jan 20, 2021

Unfortunately, I can not find any LDAP StartTLS Extended Operation regression tests. security/infra area contains RevocationChecker tests. They can not be used for this scenario.

@seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Jan 20, 2021

Unfortunately, I can not find any LDAP StartTLS Extended Operation regression tests. security/infra area contains RevocationChecker tests. They can not be used for this scenario.

Ok, please add a noreg-hard label to the bug.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 20, 2021

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

8259707: LDAP channel binding does not work with StartTLS extension

Reviewed-by: mullan, dfuchs, aefimov

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

  • c5ad713: 8260250: Duplicate check in DebugInformationRecorder::recorders_frozen
  • bf5e801: 8259922: MethodHandles.collectArguments does not throw IAE if pos is outside the arity range
  • 0ea5862: 8260053: Optimize Tokens' use of Names
  • 18eb6d9: 8255348: NPE in PKIXCertPathValidator event logging code
  • a97f3c1: 8258853: Support separate function declaration and definition with ENABLE_IF-based SFINAE
  • 154e1d6: 8259009: G1 heap summary should be shown in "Heap Parameters" window on HSDB
  • acbcde8: 8256111: Create implementation for NSAccessibilityStaticText protocol
  • f928265: 8260009: InstanceKlass::has_as_permitted_subclass() fails if subclass was redefined
  • 7ed8ba1: 8256814: WeakProcessorPhases may be redundant
  • bfac3fb: 8260212: Shenandoah: resolve-only UpdateRefsMode is not used
  • ... and 102 more: https://git.openjdk.java.net/jdk/compare/a6b2162f5400b4596afa5b88219c9cb55dcdb729...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.

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, @dfuch, @AlekseiEfimov) 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 label Jan 20, 2021
@alexeybakhtin
Copy link
Author

@alexeybakhtin alexeybakhtin commented Jan 20, 2021

Sean, Thank you for review

@alexeybakhtin
Copy link
Author

@alexeybakhtin alexeybakhtin commented Jan 20, 2021

/integrate

@openjdk openjdk bot added the sponsor label Jan 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 20, 2021

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

Copy link
Member

@dfuch dfuch left a comment

That look reasonable to me. But what would happen if at some point after performing some LDAP operations, you called StartTLSResponse::close and then after some more time you tried to again create a StartTLSRequest on the same context? Would that work - or would you be using a possibly obsolete channel binding obtained from the first upgrade?

@AlekseiEfimov
Copy link
Member

@AlekseiEfimov AlekseiEfimov commented Jan 20, 2021

The change looks valid to me too.
I believe Daniel question could be illustrated with the following change to CBwithTLS reproducer attached to the bug report:

--- CBwithTLS_Original.java	2021-01-20 14:56:09.620844903 +0000
+++ CBwithTLS.java	2021-01-20 15:01:47.253982227 +0000
@@ -45,7 +45,7 @@
         System.out.println(ctxt.getAttributes("", new String[]{"defaultNamingContext"}).get("defaultNamingContext").get());
 
         // Switch to TLS
-
+        for (int i=0; i<2; i++) {
         StartTlsResponse tls = (StartTlsResponse) ctxt.extendedOperation(new StartTlsRequest());
         tls.negotiate();
 
@@ -64,6 +64,9 @@
         lc.login();
 
         Subject.doAs(lc.getSubject(), (PrivilegedAction<Void>) CBwithTLS::run);
+            lc.logout();
+            tls.close();
+        }
     }
 
     private static Void run() {
@alexeybakhtin
Copy link
Author

@alexeybakhtin alexeybakhtin commented Jan 20, 2021

New ChannelBinding Data will be recreated for every TLS connection and provided to SASL Client in the new environment properties set (cloned from the original).
LdapSasl.java lines 133 - 136:

                        TlsChannelBinding tlsCB =
                                TlsChannelBinding.create(cert);
                        envProps = (Hashtable<String, Object>) env.clone();
                        envProps.put(TlsChannelBinding.CHANNEL_BINDING, tlsCB.getData());

@dfuch
Copy link
Member

@dfuch dfuch commented Jan 20, 2021

New ChannelBinding Data will be recreated for every TLS connection and provided to SASL Client in the new environment properties set (cloned from the original).
LdapSasl.java lines 133 - 136:

                        TlsChannelBinding tlsCB =
                                TlsChannelBinding.create(cert);
                        envProps = (Hashtable<String, Object>) env.clone();

Hi Alexey,

Aleksei and I have concern because this code uses a cert that is obtained from a CompletableFuture, and the completable future can be completed only once. The second time around - you will therefore find the same cert that was set when the first StartTLSResponse was negotiated. This may - or may not matter - depending on whether the cert certificate returned by the server the second time around should be the same - or not.
Could you test this scenario?
It may be that it's a niche scenario that makes no sense or that we don't want to support - I'm not sure how STARTTLS is used in the wild. Do you have any insights on this?

best regards,

-- daniel

@openjdk openjdk bot removed the sponsor label Jan 21, 2021
@alexeybakhtin
Copy link
Author

@alexeybakhtin alexeybakhtin commented Jan 21, 2021

Hi Daniel, Aleksei,

You are right, the second StartTLS request works incorrectly because of a single CompletableFuture. I do not know if several StartTLS sessions are used in the wild, but there are no such restrictions in the spec. I have updated the review to create new CompletableFuture for each TLS session. Updated test, suggested by Aleksei is passed. I have verified that the Channel Binding data is created on the base of a new cert object every TLS session.

}

public boolean isTlsConnection() {
return sock instanceof SSLSocket;
private HandshakeListener tlsHandshakeListener;

This comment has been minimized.

@AlekseiEfimov

AlekseiEfimov Jan 21, 2021
Member

I believe that volatile modifier should be added here. And it could be beneficial for future maintainers to have here a comment block with a brief description of when tlsHandshakeListener is used.

This comment has been minimized.

@alexeybakhtin

alexeybakhtin Jan 21, 2021
Author

Agree. Added comments and volatile modifier.


private class HandshakeListener implements HandshakeCompletedListener {

private CompletableFuture<X509Certificate> tlsHandshakeCompleted =

This comment has been minimized.

@AlekseiEfimov

AlekseiEfimov Jan 21, 2021
Member

tlsHandshakeCompleted could be made final

This comment has been minimized.

@alexeybakhtin

alexeybakhtin Jan 21, 2021
Author

Thank you. Made it final

Copy link
Member

@AlekseiEfimov AlekseiEfimov left a comment

Hi Alexey,
The latest changes look good to me.
Thanks for handling a case of sequential StartTLS requests on one LDAP context and running the modified test. I've also checked that existing LDAP tests shows no failures with the proposed changes.
You might also want to update last modification years to 2021 in both files.

@alexeybakhtin
Copy link
Author

@alexeybakhtin alexeybakhtin commented Jan 22, 2021

/integrate

@openjdk openjdk bot added the sponsor label Jan 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 22, 2021

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

@dfuch
dfuch approved these changes Jan 22, 2021
Copy link
Member

@dfuch dfuch left a comment

LGTM. Thanks for taking this on!

@dfuch
Copy link
Member

@dfuch dfuch commented Jan 22, 2021

I will sponsor this!

@dfuch
Copy link
Member

@dfuch dfuch commented Jan 22, 2021

/sponsor

@openjdk openjdk bot closed this Jan 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 22, 2021

@dfuch @alexeybakhtin Since your change was applied there have been 112 commits pushed to the master branch:

  • c5ad713: 8260250: Duplicate check in DebugInformationRecorder::recorders_frozen
  • bf5e801: 8259922: MethodHandles.collectArguments does not throw IAE if pos is outside the arity range
  • 0ea5862: 8260053: Optimize Tokens' use of Names
  • 18eb6d9: 8255348: NPE in PKIXCertPathValidator event logging code
  • a97f3c1: 8258853: Support separate function declaration and definition with ENABLE_IF-based SFINAE
  • 154e1d6: 8259009: G1 heap summary should be shown in "Heap Parameters" window on HSDB
  • acbcde8: 8256111: Create implementation for NSAccessibilityStaticText protocol
  • f928265: 8260009: InstanceKlass::has_as_permitted_subclass() fails if subclass was redefined
  • 7ed8ba1: 8256814: WeakProcessorPhases may be redundant
  • bfac3fb: 8260212: Shenandoah: resolve-only UpdateRefsMode is not used
  • ... and 102 more: https://git.openjdk.java.net/jdk/compare/a6b2162f5400b4596afa5b88219c9cb55dcdb729...master

Your commit was automatically rebased without conflicts.

Pushed as commit 874aef4.

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

@openjdk openjdk bot removed the rfr label Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants