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

Insight and CLI remove "anon" from disabled algorithms. #5943

Closed
wants to merge 3 commits into from

Conversation

mtbc
Copy link
Member

@mtbc mtbc commented Jan 18, 2019

What this PR does

On startup has Insight and command-line importer remove "anon" from among the value of the jdk.tls.disabledAlgorithms security property.

Testing this PR

Insight should be able to connect to servers even with current Java installed: version 8u201 or later.

bin/omero import ... should work similarly.

Related reading

https://trello.com/c/q7we5yYn/68-insight-ssl-algorithms

@mtbc mtbc added the develop label Jan 18, 2019
JDK-8211883

Code reuse to be refactored once decoupling is complete.
@mtbc
Copy link
Member Author

mtbc commented Jan 18, 2019

Best to copy-and-paste code until after the dust from #5934 settles.

@mtbc mtbc changed the title Insight removes "anon" from disabled algorithms. Insight and CLI remove "anon" from disabled algorithms. Jan 18, 2019
@mtbc
Copy link
Member Author

mtbc commented Jan 20, 2019

For setProperty could have caught SecurityException but:

  • Possible that this isn't something we're likely to run into. (Maybe would have been an issue for webstart.)
  • Not easy to handle in any useful way.
  • Seemed best to keep changes as simple as possible for an emergency fix.

@dominikl
Copy link
Member

It's tricky to test. Tested so far:

  • Windows 10, Java 8 201: Cannot connect to nightshade with 5.4.9. Can connect with OMERO-DEV-merge build to eel though, so fix seems to work (unforunatenly can't test nightshade with the 5.5.0 OMERO_DEV merge build).

@jburel
Copy link
Member

jburel commented Jan 21, 2019

Connecting to eel is enough I would think

@pwalczysko
Copy link
Member

Connecting to eel is enough I would think

But I thought we want to do 5.4.10 insight -> we need to build it anyway ?

@pwalczysko
Copy link
Member

pwalczysko commented Jan 21, 2019

Tested insight

  • Windows 7 laptop to eel with the java 8 update 201, went fine
  • Windows 7 laptop to eel with java 8 update 60, went fine (to test working with older java updates)
  • Mac OS to eel with the java 8 update 201, went fine

@jburel
Copy link
Member

jburel commented Jan 21, 2019

enough to test the fix I mean

@dominikl
Copy link
Member

Tested Insight on Linux (Debian) was well 👍 Unfortunately can't test CLI, no idea how to get Ice 3.6 for Debian.

@jburel
Copy link
Member

jburel commented Jan 21, 2019

@pwalczysko
Copy link
Member

pwalczysko commented Jan 21, 2019

Tested:

  • Mac OS CLI import to eel using java 8 update 201, went fine
  • Mac OS CLI import to eel using java 8 update 65, went fine

@dominikl
Copy link
Member

dominikl commented Jan 21, 2019

Ok, finally managed to get the right Java (11.0.2) installed on debian. The fix doesn't seem to work for the CLI:
screen shot 2019-01-21 at 14 47 31

@pwalczysko
Copy link
Member

@dominikl Isn't that maybe because you are running the linux in VB ? I would guess @mtbc had tested his fix on Linux in the first place ?

@jburel
Copy link
Member

jburel commented Jan 21, 2019

@dominikl
Copy link
Member

Thanks. With the debian OpenSSL patch applied it works (5.4.9 fails, 5.5.0 dev build works) 👍

@pwalczysko
Copy link
Member

I think we have expected results on 3 OS-es, Insight and CLI.

Ready to merge FMPOV

@joshmoore
Copy link
Member

Sorry for the late comment, but would this have been better placed directly in omero.client or was your goal to hit main methods to make it only happen once?

@mtbc
Copy link
Member Author

mtbc commented Jan 23, 2019

I tend to be leery of touching that, especially as it's complex and mirrored in Python and C++; I also wasn't sure if it would always be wanted by every client instantiation (could be a utility method called from main methods?). Time's short and this is tested so probably okay for a 5.4.10? Also happy to refactor as you like for 5.5.0: was going to solicit suggestions on that.

@joshmoore
Copy link
Member

Understood. The downside is that other consumers will not get the fix, but that may be acceptable. Fine to see refactoring in 5.5.

@mtbc
Copy link
Member Author

mtbc commented Jan 24, 2019

Superseded by #5947.

@mtbc mtbc closed this Jan 24, 2019
@mtbc mtbc deleted the Insight-SSL branch January 24, 2019 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants