[#643] Add support for client certificate based authentication #744

Alfusainey · 2018-08-01T14:40:30Z

@sophokles73: this patch adds support for client certificate authentication. now the auth providers are known only to the SaslAuthenticator class. i plan to test feature in the ITs.

Signed-off-by: Alfusainey Jallow alf.jallow@gmail.com

Alfusainey · 2018-08-02T12:27:17Z

@sophokles73: can you please take a look at this?

sophokles73 · 2018-08-02T12:52:09Z

...p-vertx/src/main/java/org/eclipse/hono/adapter/amqp/AmqpAdapterSaslAuthenticatorFactory.java

            if (socket.isSsl()) {
-                LOG.debug("Client connected through a secured port. Ignoring peer certificates (not supported)");
+                LOG.debug("Client connected through a secured port");


sophokles73 · 2018-08-02T12:53:12Z

...p-vertx/src/main/java/org/eclipse/hono/adapter/amqp/AmqpAdapterSaslAuthenticatorFactory.java

+                try {
+                    peerCertificateChain = socket.sslSession().getPeerCertificates();
+                    clientCertAuthProvider = new X509AuthProvider(credentialsServiceClient, config);
+                    certValidator = new DeviceCertificateValidator();


do we need to create new instances for each device connecting?

sophokles73 · 2018-08-02T12:57:19Z

tests/src/test/java/org/eclipse/hono/tests/amqp/AmqpAdapterTestBase.java

@@ -358,6 +359,7 @@ public void testUploadingXnumberOfMessages(final TestContext context) throws Int

        final Future<ProtonConnection> result = Future.future();
        final ProtonClientOptions options = new ProtonClientOptions();
+        options.addEnabledSaslMechanism(ProtonSaslPlainImpl.MECH_NAME);


what about a test for a client using SASL_EXTERNAL?

i want to add that later for a device that wants to authenticate using a client certificate. the current test code base does not use any certificate..

SASL_PLAIN is added mainly so that the current ITs passes. WDYT?

I think that it would be good to have an integration test that uses SASL_EXTERNAL :-)

BTW you should rebase so that the Travis build works again ...

Alfusainey · 2018-08-02T15:33:46Z

I think that it would be good to have an integration test that uses SASL_EXTERNAL :-)

ok, i will include that in this PR

BTW you should rebase so that the Travis build works again ...

just rebased, thanks!

Alfusainey · 2018-08-11T18:57:25Z

@sophokles73: patch updated with 899010a. this commit also includes minor refactoring of the HTTP adapter code to reuse its trust options config.

sophokles73 · 2018-08-12T10:33:22Z

...p-vertx/src/main/java/org/eclipse/hono/adapter/http/vertx/VertxBasedHttpProtocolAdapter.java

-                        return null;
-                    }
-                });
+        return getTrustOptions();


I do not see why we need to introduce another level of indirection. Can't we simply pull up this method as is?

if you say "pull up", you mean having it as a default implementation for all protocol adapters?

Don't you think that would be a reasonable default?

yes, makes sense

sophokles73 · 2018-08-12T10:34:31Z

tests/src/test/java/org/eclipse/hono/tests/IntegrationTestSupport.java

@@ -101,6 +106,7 @@
    public static final int    MQTT_PORT = Integer.getInteger(PROPERTY_MQTT_PORT, DEFAULT_MQTT_PORT);
    public static final String AMQP_HOST = System.getProperty(PROPERTY_AMQP_HOST, DEFAULT_HOST);
    public static final int    AMQP_PORT = Integer.getInteger(PROPERTY_AMQP_PORT, DEFAULT_AMQP_PORT);
+    public static final int    AMQPS_PORT = Integer.getInteger(PROPERTY_AMQPS_PORT, 4041);


for consistency, can we use a constant for the default AMQPS port as well?

sophokles73 · 2018-08-12T10:35:36Z

tests/src/test/java/org/eclipse/hono/tests/IntegrationTestSupport.java

@@ -130,14 +136,14 @@

    private final Set<String> tenantsToDelete = new HashSet<>();
    private final Map<String, Set<String>> devicesToDelete = new HashMap<>();
-    private final Vertx vertx;
+    private static Vertx VERTX;


we use upper case naming for constants only

sophokles73 · 2018-08-12T10:41:21Z

@Alfusainey why doesn't the Travis build succeed?

Alfusainey · 2018-08-12T17:36:02Z

@Alfusainey why doesn't the Travis build succeed?

humm... looks like the vertx thread has been blocked for a long time... am suspecting that it does not directly relate to my changes:

at java.base@10.0.2/java.util.concurrent.CompletableFuture.waitingGet(CompletableFuture.java:1823)
at java.base@10.0.2/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1998)
at app//io.vertx.ext.unit.impl.CompletionImpl.await(CompletionImpl.java:56)
at app//org.eclipse.hono.tests.IntegrationTestSupport.lambda$deleteObjects$4(IntegrationTestSupport.java:206)
at app//org.eclipse.hono.tests.IntegrationTestSupport$$Lambda$245/1193841543.accept(Unknown Source)
at java.base@10.0.2/java.lang.Iterable.forEach(Iterable.java:75)
at app//org.eclipse.hono.tests.IntegrationTestSupport.deleteObjects(IntegrationTestSupport.java:203)
at app//org.eclipse.hono.tests.mqtt.MqttTestBase.lambda$postTest$1(MqttTestBase.java:121)

i think when deleting the temp devices, the call to deletion.await() does not return on time

Alfusainey · 2018-08-13T08:57:59Z

@sophokles73: patch updated with f4ca7ce. now the vertx instance in the IntegrationTestSupport class is not static. i suspect this was failing the java10 build (but am not entirely sure)

Alfusainey · 2018-08-13T11:10:52Z

@sophokles73: something very strange is happening with the jdk10 travis build. it takes longer to run and the MQTT adapter ITs is failing the build. i was thinking that my changes to the IntegrationTestSupport class fails the build but that is not the case

Alfusainey · 2018-08-13T12:25:38Z

@sophokles73: i rebased this patch with current master to include 008e20c. hopefully the fix passes the build 👍

sophokles73 · 2018-08-13T12:38:35Z

yeah, let's hope so ...

Alfusainey · 2018-08-13T13:25:51Z

@sophokles73: the build passes 🕺

sophokles73 · 2018-08-13T13:40:11Z

tests/src/test/java/org/eclipse/hono/tests/amqp/AmqpAdapterTestBase.java

-                connection.open();
-            }
-        });
+        if (!Strings.isNullOrEmpty(username) && !Strings.isNullOrEmpty(password)) {


please split this up into two methods: connectToAdapter(String, String) and connectToAdapter(SelfSignedCertificate) to make it more transparent in the client code which mechanism is used. There is basically no duplicated code so it shouldn't be an issue ...

@sophokles73 patch updated

sophokles73 · 2018-08-13T14:30:16Z

@Alfusainey can you squash and rebase?

…ation Signed-off-by: Alfusainey Jallow <alf.jallow@gmail.com>

Alfusainey · 2018-08-13T14:53:11Z

@sophokles73 : done!

Alfusainey force-pushed the amqp-adapter-certificate-auth branch from df0f093 to 058674f Compare August 1, 2018 14:43

sophokles73 reviewed Aug 2, 2018

View reviewed changes

Alfusainey force-pushed the amqp-adapter-certificate-auth branch from 058674f to 775d23f Compare August 2, 2018 15:33

Alfusainey force-pushed the amqp-adapter-certificate-auth branch from 775d23f to c340b99 Compare August 11, 2018 18:53

Alfusainey force-pushed the amqp-adapter-certificate-auth branch from c340b99 to 899010a Compare August 11, 2018 20:33

sophokles73 reviewed Aug 12, 2018

View reviewed changes

Alfusainey force-pushed the amqp-adapter-certificate-auth branch from f4ca7ce to 2a6561f Compare August 13, 2018 12:24

sophokles73 reviewed Aug 13, 2018

View reviewed changes

sophokles73 approved these changes Aug 13, 2018

View reviewed changes

[eclipse-hono#643] Add support for client certificate based authentic…

16a0b14

…ation Signed-off-by: Alfusainey Jallow <alf.jallow@gmail.com>

Alfusainey force-pushed the amqp-adapter-certificate-auth branch from 17d24e0 to 16a0b14 Compare August 13, 2018 14:52

sophokles73 added enhancement AMQP Adapter labels Aug 13, 2018

sophokles73 added this to To do in 0.7 via automation Aug 13, 2018

sophokles73 added this to the 0.7 milestone Aug 13, 2018

sophokles73 merged commit b293613 into eclipse-hono:master Aug 13, 2018

0.7 automation moved this from To do to Done Aug 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#643] Add support for client certificate based authentication #744

[#643] Add support for client certificate based authentication #744

Alfusainey commented Aug 1, 2018

Alfusainey commented Aug 2, 2018

sophokles73 Aug 2, 2018

sophokles73 Aug 2, 2018

sophokles73 Aug 2, 2018

Alfusainey Aug 2, 2018

sophokles73 Aug 2, 2018

sophokles73 Aug 2, 2018

Alfusainey commented Aug 2, 2018

Alfusainey commented Aug 11, 2018 •

edited

sophokles73 Aug 12, 2018

Alfusainey Aug 12, 2018

sophokles73 Aug 13, 2018

Alfusainey Aug 13, 2018

sophokles73 Aug 12, 2018

sophokles73 Aug 12, 2018

Alfusainey Aug 12, 2018

sophokles73 commented Aug 12, 2018

Alfusainey commented Aug 12, 2018

Alfusainey commented Aug 13, 2018

Alfusainey commented Aug 13, 2018

Alfusainey commented Aug 13, 2018

sophokles73 commented Aug 13, 2018

Alfusainey commented Aug 13, 2018

sophokles73 Aug 13, 2018

Alfusainey Aug 13, 2018

Alfusainey Aug 13, 2018

sophokles73 commented Aug 13, 2018

Alfusainey commented Aug 13, 2018

[#643] Add support for client certificate based authentication #744

[#643] Add support for client certificate based authentication #744

Conversation

Alfusainey commented Aug 1, 2018

Alfusainey commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alfusainey commented Aug 2, 2018

Alfusainey commented Aug 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophokles73 commented Aug 12, 2018

Alfusainey commented Aug 12, 2018

Alfusainey commented Aug 13, 2018

Alfusainey commented Aug 13, 2018

Alfusainey commented Aug 13, 2018

sophokles73 commented Aug 13, 2018

Alfusainey commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophokles73 commented Aug 13, 2018

Alfusainey commented Aug 13, 2018

Alfusainey commented Aug 11, 2018 •

edited