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

ZOOKEEPER-3229: [TLS] add AES-256 ciphers to default cipher list #744

Closed
wants to merge 1 commit into from

Conversation

ivmaykov
Copy link
Contributor

  • Add AES-256 cipher suites
  • Add AES-128-...-SHA cipher suites (for compatibility with Netty OpenSSL transport)
  • code cleanup: split up the suites into CBC and GCM ciphers and put them in different order when constructing java8 / java9 defaults.

@ivmaykov ivmaykov closed this Dec 20, 2018
@ivmaykov ivmaykov reopened this Dec 20, 2018
Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

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

+1 lgtm

};
}

private static String[] concatArrays(String[] left, String[] right) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ArrayUtils.addAll() in Apache Commons lib?

Copy link

@dineshappavoo dineshappavoo Dec 21, 2018

Choose a reason for hiding this comment

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

I think apache commons lang or lang3 is not included for all the targets in ivy dependencies ( https://github.com/apache/zookeeper/blob/master/ivy.xml#L93 ). At least I couldn't use that library and compile the code. You might want to remove the conf and use the default conf like
<dependency org="commons-lang" name="commons-lang" rev="${commons-lang.version}"/>
to make it working.

@dineshappavoo
Copy link

LGTM 👍

Copy link
Contributor

@lvfangmin lvfangmin left a comment

Choose a reason for hiding this comment

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

+1 LGTM.

@anmolnar
Copy link
Contributor

anmolnar commented Jan 2, 2019

retest this please

@ivmaykov ivmaykov closed this Jan 3, 2019
@ivmaykov ivmaykov reopened this Jan 3, 2019
@anmolnar
Copy link
Contributor

anmolnar commented Jan 3, 2019

retest this please

@anmolnar
Copy link
Contributor

anmolnar commented Jan 3, 2019

@ivmaykov QuorumSSLTest.testProtocolVersion is failing, because TLSv1.1 participant is able to join TLSv1.2-only quorum. Not sure how is it related to this change, but could you please take a look?

@ivmaykov
Copy link
Contributor Author

@anmolnar I will take a look. That seems pretty weird, and I don't think we had that failure on our internal fork (otherwise, our continuous build would catch it and fail). Sorry for not responding sooner, was working on other things - should have time to work up on my open PRs again over the next few days.

@ivmaykov
Copy link
Contributor Author

@anmolnar it doesn't fail when rebased on top of #681. I don't want to spend too much time figuring out why, since I wrote #681 a few months ago and the relevant knowledge is no longer in my cache :) I do remember that #681 fixes several issues with configuring SSL sockets. This PR can wait until after #681 lands.

@ivmaykov
Copy link
Contributor Author

@anmolnar tests are passing now that this is rebased on top of master which includes #681

@asfgit asfgit closed this in 3583bbf Jan 25, 2019
asfgit pushed a commit that referenced this pull request Jan 25, 2019
- Add AES-256 cipher suites
- Add AES-128-...-SHA cipher suites (for compatibility with Netty OpenSSL transport)
- code cleanup: split up the suites into CBC and GCM ciphers and put them in different order when constructing java8 / java9 defaults.

Author: Ilya Maykov <[email protected]>

Reviewers: [email protected]

Closes #744 from ivmaykov/ZOOKEEPER-3229

(cherry picked from commit 3583bbf)
Signed-off-by: Andor Molnar <[email protected]>
@anmolnar
Copy link
Contributor

Committed to master and 3.5 branches.
Thanks @ivmaykov !

RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Sep 3, 2022
- Add AES-256 cipher suites
- Add AES-128-...-SHA cipher suites (for compatibility with Netty OpenSSL transport)
- code cleanup: split up the suites into CBC and GCM ciphers and put them in different order when constructing java8 / java9 defaults.

Author: Ilya Maykov <[email protected]>

Reviewers: [email protected]

Closes apache#744 from ivmaykov/ZOOKEEPER-3229
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants