You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "TheDeadOne (via GitHub)" <gi...@apache.org> on 2023/07/30 03:31:41 UTC
[GitHub] [incubator-pekko-connectors] TheDeadOne opened a new pull request, #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
TheDeadOne opened a new pull request, #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205
This fix adds a couple of methods to FtpsSettings to set specific KeyManager and TrustManager.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] mdedetrich commented on a diff in pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#discussion_r1278849645
##########
ftp/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker:
##########
@@ -0,0 +1 @@
+mock-maker-inline
Review Comment:
nit: newline
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] TheDeadOne commented on a diff in pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "TheDeadOne (via GitHub)" <gi...@apache.org>.
TheDeadOne commented on code in PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#discussion_r1278860090
##########
ftp/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker:
##########
@@ -0,0 +1 @@
+mock-maker-inline
Review Comment:
@mdedetrich fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] mdedetrich commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1674358691
FYI I haven't forgotten about this, just have a lot of other stuff going on. Will likely look at it next week
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] mdedetrich merged pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich merged PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] TheDeadOne commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "TheDeadOne (via GitHub)" <gi...@apache.org>.
TheDeadOne commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1657823432
@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".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] TheDeadOne commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "TheDeadOne (via GitHub)" <gi...@apache.org>.
TheDeadOne commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1657732000
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] mdedetrich commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1657772130
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. https://github.com/apache/incubator-pekko-connectors/blob/c207c292ee9fecec2444637c84d63b9127280422/amqp/src/test/scala/org/apache/pekko/stream/connectors/amqp/scaladsl/AmqpFlowSpec.scala is 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 redo the test on your PR.
.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] mdedetrich commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1657762067
@TheDeadOne Code needs to be formatted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] mdedetrich commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1657793888
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] pjfanning commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1657131197
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] pjfanning commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1686050097
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] mdedetrich commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1693063553
Im working on this now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] mdedetrich commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1693385667
@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 the 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.
```scala
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.
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] TheDeadOne commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "TheDeadOne (via GitHub)" <gi...@apache.org>.
TheDeadOne commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1657792624
@mdedetrich I can try, but I'm afraid I won't do it well enough. I would appreciate help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] mdedetrich commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1657825270
> @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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] TheDeadOne commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "TheDeadOne (via GitHub)" <gi...@apache.org>.
TheDeadOne commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1657837474
@mdedetrich running the command only affected Dependencies.scala.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] mdedetrich commented on pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#issuecomment-1657845782
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] pjfanning commented on a diff in pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#discussion_r1278958223
##########
ftp/src/test/java/org/apache/pekko/stream/connectors/ftp/FtpsWithTrustAndKeyManagersStageTest.java:
##########
@@ -0,0 +1,107 @@
+/*
Review Comment:
New files should use the standard ASF header. Only code written by Lightbend gets the header you've added here.
https://github.com/apache/incubator-pekko-connectors/blob/main/project/MetaInfLicenseNoticeCopy.scala#L1-L16
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-connectors] TheDeadOne commented on a diff in pull request #205: FTP: Add for FTPS the ability to set KeyManager and TrustManager
Posted by "TheDeadOne (via GitHub)" <gi...@apache.org>.
TheDeadOne commented on code in PR #205:
URL: https://github.com/apache/incubator-pekko-connectors/pull/205#discussion_r1279021811
##########
ftp/src/test/java/org/apache/pekko/stream/connectors/ftp/FtpsWithTrustAndKeyManagersStageTest.java:
##########
@@ -0,0 +1,107 @@
+/*
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org