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

8245527: LDAP Channel Binding support for Java GSS/Kerberos #278

Closed
wants to merge 2 commits into from

Conversation

@alexeybakhtin
Copy link

@alexeybakhtin alexeybakhtin commented Sep 21, 2020

Hi,

Plaese review JDK-8245527 fix which implements LDAP Channel Binding support for Java GSS/Kerberos.
Initial review is available at core-devs: https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/068197.html
This version removes "tls-unique" CB type from the list of possible channel binding types. The only supported type is "tls-server-end-point"

CSR is also updated : https://bugs.openjdk.java.net/browse/JDK-8247311

Thank you
Alexey


Progress

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

Issue

  • JDK-8245527: LDAP Channel Binding support for Java GSS/Kerberos

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 21, 2020

👋 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 Sep 21, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 21, 2020

@alexeybakhtin The following labels will be automatically applied to this pull request: core-libs 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 (add|remove) "label" command.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 21, 2020

Webrevs

@dfuch
Copy link
Member

@dfuch dfuch commented Sep 22, 2020

Thanks for the PR Alexey! Let me run a last round of testing - and if that comes back clean I'll approve. Please don't integrate until you get a reviewer from security-libs too.

best regards,
-- daniel

@@ -972,4 +990,46 @@ public void run() {
}
return buf;
}

private CompletableFuture<X509Certificate> tlsHandshakeCompleted =
Copy link
Member

@dfuch dfuch Sep 22, 2020

Should be final?

Copy link
Author

@alexeybakhtin alexeybakhtin Sep 22, 2020

Thank you. Agree. It should be final.

/**
* Channel binding on the basis of TLS Finished message
*/
TLS_UNIQUE("tls-unique"),
Copy link
Member

@dfuch dfuch Sep 22, 2020

Is that still used? If not maybe it should be removed?

Copy link
Author

@alexeybakhtin alexeybakhtin Sep 22, 2020

No, It is not used.
However, I'd like to leave it as is (it is mentioned in the documentation as unsupported value).
Otherwise, TlsChannelBindingType enum will have one element only and should be simplified/removed in all places. In this case, it would be double work to add TlsChannelBindingType enum back in the future if "tls-unique" required.
If required I can remove TLS_UNIQUE item, but not remove TlsChannelBindingType enum

Copy link
Member

@dfuch dfuch Sep 22, 2020

I was suggesting to keep TlsChannelBindingType but remove TLS_UNIQUE; However, I'm OK to keep things as is: this is an internal API. I wonder if it would deserve a comment:

        /**
         * Channel binding on the basis of TLS Finished message
         */
        // TLS_UNIQUE is defined by RFC 5929 but is not supported by the current LDAP stack.
        TLS_UNIQUE("tls-unique"),

Copy link
Author

@alexeybakhtin alexeybakhtin Sep 22, 2020

Thank you. Added suggested comment.

Copy link
Contributor

@wangweij wangweij left a comment

I'm mostly OK with the SASL/JGSS part, except for the small nits in this comment. I'm not an expert on HandshakeCompletedListener.

@@ -150,6 +151,14 @@
}
secCtx.requestMutualAuth(mutual);

if (props != null) {
// TLS Channel Binding
byte[] tlsCB = (byte[])props.get("jdk.internal.sasl.tlschannelbinding");
Copy link
Contributor

@wangweij wangweij Sep 22, 2020

You can say the name is defined in another class in another module. If we really want to rename it one day we will know where it's from.

Copy link
Author

@alexeybakhtin alexeybakhtin Sep 22, 2020

Thank you. Comment is added

CHANNEL_BINDING_AF_UNSPEC:CHANNEL_BINDING_AF_NULL_ADDR);
int acceptorAddressType = getAddrType(acceptorAddress,
(channelBinding instanceof TlsChannelBindingImpl)?
CHANNEL_BINDING_AF_UNSPEC:CHANNEL_BINDING_AF_NULL_ADDR);
Copy link
Contributor

@wangweij wangweij Sep 22, 2020

Normally we put a white space around "?" and ":".

Copy link
Author

@alexeybakhtin alexeybakhtin Sep 22, 2020

Thank you. Fixed.


/**
* Parse value of "com.sun.jndi.ldap.tls.cbtype" property
* @param cbType
Copy link
Contributor

@wangweij wangweij Sep 22, 2020

Please add a @return here, esp, about null.

Copy link
Author

@alexeybakhtin alexeybakhtin Sep 22, 2020

Added @return with comments


public TlsChannelBindingType getType() {
return cbType;
}
Copy link
Contributor

@wangweij wangweij Sep 22, 2020

Add a new line here.

Copy link
Author

@alexeybakhtin alexeybakhtin Sep 22, 2020

Fixed

dfuch
dfuch approved these changes Sep 23, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2020

@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 more details.

After integration, the commit message for the final commit will be:

8245527: LDAP Channel Binding support for Java GSS/Kerberos

Reviewed-by: dfuchs, aefimov, 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 79 new commits pushed to the master branch:

  • 37b7028: 8240556: Abort concurrent mark after effective eager reclamation of humongous objects
  • dcde95b: 8253586: C2: Clean up unused PhaseIterGVN::init_worklist()
  • 0b83fc0: 8235710: Remove the legacy elliptic curves
  • 8239b67: 8253322: Update the specification in the newly added constructors
  • 24a4248: 8253615: Change to Visual Studio 2019 16.7.2 for building on Windows at Oracle
  • 8b85c3a: 8251261: CDS dumping should not clear states in live classes
  • 9ac162e: 8251999: remove usage of PropertyResolvingWrapper in vmTestbase/nsk/aod
  • 527a309: 8252722: More Swing plaf APIs that rely on default constructors
  • 3495c19: 8250855: Address reliance on default constructors in the Java 2D APIs
  • a9d0440: 8250859: Address reliance on default constructors in the Accessibility APIs
  • ... and 69 more: https://git.openjdk.java.net/jdk/compare/52c28b869293c8639228ff32ec8f8615955fe39d...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 (@wangweij, @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 Sep 23, 2020
Copy link
Member

@AlekseiEfimov AlekseiEfimov left a comment

Hi Alexey, The latest changes looks good to me.

@dfuch
Copy link
Member

@dfuch dfuch commented Sep 23, 2020

/csr

@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2020

@dfuch the issue for this pull request, JDK-8245527, already has an approved CSR request: JDK-8247311

@alexeybakhtin
Copy link
Author

@alexeybakhtin alexeybakhtin commented Sep 24, 2020

/reviewer credit mullan
/reviewer credit weijun
/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 24, 2020

@alexeybakhtin
Reviewer mullan successfully credited.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 24, 2020

@alexeybakhtin Reviewer weijun has already made an authenticated review of this PR, and does not need to be credited manually.

@openjdk openjdk bot added the sponsor label Sep 24, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 24, 2020

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

@yan-too
Copy link

@yan-too yan-too commented Sep 25, 2020

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Sep 25, 2020

@yan-too @alexeybakhtin Since your change was applied there have been 79 commits pushed to the master branch:

  • 37b7028: 8240556: Abort concurrent mark after effective eager reclamation of humongous objects
  • dcde95b: 8253586: C2: Clean up unused PhaseIterGVN::init_worklist()
  • 0b83fc0: 8235710: Remove the legacy elliptic curves
  • 8239b67: 8253322: Update the specification in the newly added constructors
  • 24a4248: 8253615: Change to Visual Studio 2019 16.7.2 for building on Windows at Oracle
  • 8b85c3a: 8251261: CDS dumping should not clear states in live classes
  • 9ac162e: 8251999: remove usage of PropertyResolvingWrapper in vmTestbase/nsk/aod
  • 527a309: 8252722: More Swing plaf APIs that rely on default constructors
  • 3495c19: 8250855: Address reliance on default constructors in the Java 2D APIs
  • a9d0440: 8250859: Address reliance on default constructors in the Accessibility APIs
  • ... and 69 more: https://git.openjdk.java.net/jdk/compare/52c28b869293c8639228ff32ec8f8615955fe39d...master

Your commit was automatically rebased without conflicts.

Pushed as commit cfa3f74.

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