FTP: Add for FTPS the ability to set KeyManager and TrustManager

[TheDeadOne]

This fix adds a couple of methods to FtpsSettings to set specific KeyManager and TrustManager.

[mdedetrich]

lgtm

[pjfanning]

It looks safe for v1.0.0 to me but I'd prefer if there was unit test coverage.

Maybe something a test can be crafted that uses custom Key and Trust managers that delegate to the default ones but that record the calls - so that they can be asserted on. Our use a mock framework like Mockito to achieve something similar.

[pjfanning]

needs a test

[TheDeadOne]

Adding an integration test was simple and natural, but mocking... KeyManager and TrustManager look unsuitable for unit tests. Besides, a full test of interaction with KeyManager will require reconfiguring the ftp server, as far as I understand, which means it will affect other tests. Perhaps we should stop at the integration test?

[mdedetrich]

@TheDeadOne Code needs to be formatted, run sbt scalafmtAll

[mdedetrich]

I would have preferred if the tests were written in Scala using the already established Mockito in Dependencies.scala (see https://github.com/apache/incubator-pekko-connectors/blob/79ec6fa65305dd933ef67063b88419bc98424f69/project/Dependencies.scala#L76-L79) rather than just adding a new Mockito dependency and writing the test in Java which is inconsistent with everything else in the project. https://github.com/apache/incubator-pekko-connectors/blob/c207c292ee9fecec2444637c84d63b9127280422/amqp/src/test/scala/org/apache/pekko/stream/connectors/amqp/scaladsl/AmqpFlowSpec.scala is such an example of a test that was written with scalatest/mockito.

@TheDeadOne Would this be something that you are able to do? Otherwise I can reimplement the test as suggested on your PR.

[TheDeadOne]

@mdedetrich I can try, but I'm afraid I won't do it well enough. I would appreciate help.

[mdedetrich]

@mdedetrich I can try, but I'm afraid I won't do it well enough. I would appreciate help.

No worries, I will do it on your PR later today. Can you just run sbt scalafmtAll so that your code is formatted/clean.

[TheDeadOne]

@mdedetrich I did it, but almost immediately got "[success] Total time: 9 s, completed 31.07.2023 15:13:20" and git status shows "nothing to commit, working tree clean".

[mdedetrich]

@mdedetrich I did it, but almost immediately got "[success] Total time: 9 s, completed 31.07.2023 15:13:20" and git status shows "nothing to commit, working tree clean".

Apologies, it should be sbt scalafmtSbt

[TheDeadOne]

@mdedetrich running the command only affected Dependencies.scala.

[mdedetrich]

@mdedetrich running the command only affected Dependencies.scala.

Yes this is correct, its what scalafmt was complaining about before. As you can see scalafmt is now passing https://github.com/apache/incubator-pekko-connectors/actions/runs/5711962119/job/15474563816?pr=205.

As mentioned before I will fix up the PR.

[mdedetrich]

FYI I haven't forgotten about this, just have a lot of other stuff going on. Will likely look at it next week

[pjfanning]

@mdedetrich is this ok to merge? We can always rewrite the Java test later. It would be good to get everything ready for the 1.0.0 release and to come back to lower priority work later.

[mdedetrich]

Im working on this now

changes made

[mdedetrich]

@pjfanning @TheDeadOne So I have just committed the changes I was talking about previously although there are some notable changes I had to make.

One of them is that rather than using a real X509ExtendedKeyManager/X509ExtendedTrustManager I instead ended up mocking it with mockito. Although it was originally working (both on @TheDeadOne 's version and when I did some initial work), at some point I was getting the following error

[info] org.apache.pekko.stream.connectors.ftp.FtpsWithTrustAndKeyManagersStageSpec *** ABORTED *** (1 millisecond)
[info]   org.mockito.exceptions.base.MockitoException: Cannot mock/spy class sun.security.ssl.X509TrustManagerImpl
[info] Mockito cannot mock/spy because :
[info]  - final class
[info]   at org.apache.pekko.stream.connectors.ftp.FtpsWithTrustAndKeyManagersStageSpec.setupTrustManager

I did look at the implementation of X509ExtendedKeyManager and I can confirm that it is final. I am not exactly sure why in some cases Mockito works fine when using spy and in other cases it complains that its final so I just opted to mock the classes along with the method, i.e.

doNothing().when(trustManager).checkServerTrusted(any(classOf[Array[X509Certificate]]), anyString,
  any(classOf[Socket]))

Note that this doesn't make the test any valid, i.e. if you remove the setup i.e. .withTrustManager(trustManager)/.withKeyManager(keyManager) then the test fails (as expected) since its expecting calls to checkServerTrusted. Likewise if you change verify(trustManager, atLeastOnce()).checkServerTrusted to verify(trustManager, never()).checkServerTrusted the tests also fail. One can also make an argument that this test is more "proper", i.e. the intention of the test is to make sure that setting the keyManager/trustManager works. Testing the actual implementation of trustManager.checkServerTrusted can be considered excessive since its an implementation detail and hence outside of the scope of our test.

Aside from that this version of the test is more comprehensive/accurate, i.e. it inherits BaseFtpsSpec and CommonFtpStageSpec which means it does the entire FTPS test suite and the Mockito verify is being injected directly into the pekko Source/Sink (where as with the original version .verify was being called at the end of the test which has a slight chance of making a test pass when it shouldn't)

I ended up doing changes on the PR itself so its not proper for me to review it

[pjfanning]

Lgtm