[JBWS-4179] WS integration with WildFly Elytron - AuthenticationClien… #108

Skyllarr · 2019-07-24T18:57:42Z

…t for Authentication / SSL

Requires update to following repositories:
jbossws-spi : jbossws/jbossws-spi#5
jbossws-common: jbossws/jbossws-common#18
wildfly-elytron: wildfly-security/wildfly-elytron#1309

jbliznak · 2019-07-25T08:31:37Z

modules/client/pom.xml

+
+    <dependency>
+      <groupId>org.jboss.ws</groupId>
+      <artifactId>jbossws-spi</artifactId>


Duplicate depedency? Already at https://github.com/jbossws/jbossws-cxf/blob/master/modules/client/pom.xml#L37

jbliznak · 2019-07-25T09:08:19Z

modules/client/pom.xml

+    <dependency>
+      <groupId>xerces</groupId>
+      <artifactId>xercesImpl</artifactId>
+      <version>2.7.1</version>


xercesImpl version is managed, not needed to specify it here, see https://github.com/jbossws/jbossws-cxf/blob/master/pom.xml#L773

Fixed. Thanks!

jbliznak · 2019-07-25T11:00:22Z

...s/client/src/main/java/org/jboss/wsf/stack/cxf/client/configuration/ElytronClientConfig.java

+      try {
+         endpointURI = new URI(endpointAddress);
+      } catch (URISyntaxException e) {
+         e.printStackTrace();    // TODO throw some uri parsing exception


What about this TODO? I guess client won't be used only as standalone application but can run at container environment so we need exception here, right?

jbliznak · 2019-08-07T07:53:59Z

modules/testsuite/cxf-tests/pom.xml

+    <dependency>
+        <groupId>org.wildfly.security</groupId>
+        <artifactId>wildfly-elytron-client</artifactId>
+        <version>1.10.0.CR5</version>


Please parametrize the version via property.

jbliznak

Changes seem reasonable to me, lets wait for feedback from @jimma.
I would just appreciate if we could get rid of unnecessary whitespace changes.

Skyllarr · 2019-08-07T13:38:53Z

@jbliznak @jimma All test cases have been added now.

Skyllarr · 2019-08-13T13:57:05Z

Changes seem reasonable to me, lets wait for feedback from @jimma.
I would just appreciate if we could get rid of unnecessary whitespace changes.

@jbliznak Thanks! Unnecessary whitespace changes are now gone.

jbliznak

I found some minor issues, please review.
Also could you please add some javadoc for tests that these are meant for JBWS-4179 (at least for the whole new testcases)?

jbliznak · 2019-08-13T17:12:11Z

...a/org/jboss/test/ws/jaxws/cxf/clientConfig/CXFClientConfigElytronNotOnClasspathTestCase.java

+                 .addClass(org.jboss.test.ws.jaxws.cxf.httpauth.ObjectFactory.class)
+                 .addAsResource(new File(JBossWSTestHelper.getTestResourcesDir() + "/jaxws/cxf/clientConfig/META-INF/jaxws-client-config.xml"), "META-INF/jaxws-client-config.xml")
+                 .addAsResource(new File(JBossWSTestHelper.getTestResourcesDir() + "/jaxws/cxf/httpauth/WEB-INF/wsdl/hello.wsdl"), "META-INF/wsdl/hello.wsdl")
+                 .addAsResource(new File("/home/skylar/work/projects/integrations/ws/jbossws-cxf/modules/testsuite/cxf-tests/src/test/resources/jaxws/cxf/clientConfig/META-INF/wildfly-config-http-basic-auth.xml"), "META-INF/wildfly-config.xml")


please replace with dynamically created path

jbliznak · 2019-08-13T17:12:35Z

.../cxf-tests/src/test/resources/jaxws/cxf/clientConfig/META-INF/wildfly-config-ssl-context.xml

+    <authentication-client xmlns="urn:elytron:client:1.4">
+        <key-stores>
+            <key-store name="truststore" type="PKCS12">
+                <file name="/home/skylar/work/projects/integrations/ws/jbossws-cxf/modules/testsuite/cxf-tests/src/test/etc/client.truststore"/>


please replace with dynamically created path or relative path if possible

jbliznak · 2019-08-13T17:12:40Z

.../cxf-tests/src/test/resources/jaxws/cxf/clientConfig/META-INF/wildfly-config-ssl-context.xml

+                <key-store-clear-password password="secret"/>
+            </key-store>
+            <key-store name="keystore" type="JKS">
+                <file name="/home/skylar/work/projects/integrations/ws/jbossws-cxf/modules/testsuite/cxf-tests/src/test/etc/client.keystore"/>


please replace with dynamically created path or relative path if possible

jbliznak · 2019-08-13T17:14:22Z

...a/org/jboss/test/ws/jaxws/cxf/clientConfig/CXFClientConfigElytronNotOnClasspathTestCase.java

+   @ArquillianResource
+   private URL baseURL;
+
+   public static final String CLIENT_JAR = JBossWSTestHelper.writeToFile(new JBossWSTestHelper.JarDeployment("jaxws-cxf-jbws3713-client.jar") {


could we use other name for JAR, eg. 'jaxws-cxf-jbws4179-client.jar'? It is already used by other tests

jbliznak · 2019-08-13T17:15:24Z

...a/org/jboss/test/ws/jaxws/cxf/clientConfig/CXFClientConfigElytronNotOnClasspathTestCase.java

+                 .addAsResource(new File(JBossWSTestHelper.getTestResourcesDir() + "/jaxws/cxf/clientConfig/META-INF/jaxws-client-config.xml"), "META-INF/jaxws-client-config.xml")
+                 .addAsResource(new File(JBossWSTestHelper.getTestResourcesDir() + "/jaxws/cxf/httpauth/WEB-INF/wsdl/hello.wsdl"), "META-INF/wsdl/hello.wsdl")
+                 .addAsResource(new File("/home/skylar/work/projects/integrations/ws/jbossws-cxf/modules/testsuite/cxf-tests/src/test/resources/jaxws/cxf/clientConfig/META-INF/wildfly-config-http-basic-auth.xml"), "META-INF/wildfly-config.xml")
+                 .addAsManifestResource(new File(JBossWSTestHelper.getTestResourcesDir() + "/jaxws/cxf/jbws3713/WEB-INF/client-permissions.xml"), "permissions.xml")


could we use own copy of that file here? so that we don't have to worry about future changes

jbliznak · 2019-08-13T17:16:15Z

...a/org/jboss/test/ws/jaxws/cxf/clientConfig/CXFClientConfigElytronNotOnClasspathTestCase.java

+      // our client jar is an input param to jboss-module
+      final File f = new File(JBossWSTestHelper.getTestArchiveDir(), CLIENT_JAR);
+      sbuf.append(" -jar ").append(f.getAbsolutePath());
+      sbuf.append(" -agentlib:jdwp=transport=dt_socket,address=8787,server=y,suspend=y ").append(f.getAbsolutePath());


why these debug args here?

jbliznak · 2019-08-13T17:38:14Z

...est/java/org/jboss/test/ws/jaxws/cxf/clientConfig/SSLContextElytronClientConfigTestCase.java

+         URL wsdlURL = getResourceURL("jaxws/cxf/httpauth/WEB-INF/wsdl/hello.wsdl");
+         Service service = Service.create(wsdlURL, serviceName);
+         Hello proxy = service.getPort(Hello.class);
+         ((BindingProvider) proxy).getRequestContext().put(BindingProvider.ENDPOINT_ADDRESS_PROPERTY, "https://localhost:13443/ssl-mutual-auth/HelloService");


could you please parametrize 13443 so that it takes port offset for tested container into account? Eg. https://github.com/jbossws/jbossws-cxf/blob/master/modules/testsuite/cxf-tests/src/test/java/org/jboss/test/ws/jaxws/samples/wsse/policy/oasis/WSSecurityPolicyExamples23xTestCase.java#L68-L69

jimma · 2019-08-15T08:56:48Z

@Skyllarr This is great stuff to have in jbossws. And thanks @jbliznak for review.
There might be something we can improve/refactor after the first look. Correct me if I missed thing.

ClientConfig is extensible to put all information which Elytron client needs. We can put all information to ClientConfig's property. This possibly doesn't need ElytronClientConfig to store these infomation.
Change SPI WildFlyClientConfigProvider to ClientCongProvider to make it more generic and in WFLY eltyron we create the implement class WildFlyClientConfigProvider to actually add security info to ClientConfig like :

public class WildFlyClientConfigProvider implmentens ClientConfigProvider {
        public void provides(ClientConfig config) {
           // add all Elytron client config to jbossws clientConfig object 
           config.setProperty("sslContext", context")
           ....
       }
}

We can name ElytronConduitSelector with another general name because it doesn't handle Elytron specific thing. It can retrieve info from ClientConfig and enforce them to CXF's conduit etc.

Your thoughts ?

Skyllarr · 2019-08-15T11:30:00Z

ClientConfig is extensible to put all information which Elytron client needs. We can put all information to ClientConfig's property. This possibly doesn't need ElytronClientConfig to store these infomation.

@jimma Please correct me if I misunderstand. ClientConfig has only String properties: Map<String, String> properties and I need to store javax.net.ssl.SSLContext instance in the property. Do you mean to extend ClientConfig class so it can have properties of other type than String?So in that case it would be something like:

public class WildflyClientConfig extends ClientConfig {
   Map<String, Object> securityProperties; 
...
}

and then it could be:

public class WildFlyClientConfigProvider implmentens ClientConfigProvider {
        public void provides(**WildflyClientConfig** config) {
           // add all Elytron client config to jbossws clientConfig object 
          config.setSecurityProperty("sslContext", contextInstance)
           ....
       }
}

Did I get that?

jimma · 2019-08-15T13:53:02Z

@Skyllarr I think you can add this field directly to ClientConfig like :

public SSLContext  getSslContext()
   {
      ...
   }
public void setSSLContext(SSLContext context) {
    ... 
}

And we directly pass this ClientConfig to WildFlyClientConfigProvider as you pasted above. And we can still create other ClientConfigProvider to add more properties to ClientConfig in the future. WDYT ?

Skyllarr · 2019-08-15T14:06:07Z

@jimma Alright now I think I know what you mean. Yes, it sounds nicer. I'll try it that way now. Thanks

Skyllarr · 2019-08-21T12:08:55Z

@jimma I have updated the PRs accordingly.

Skyllarr · 2019-08-26T18:13:07Z

@jbliznak I have added documentation to new test classes. @jimma please review

Skyllarr · 2019-08-28T16:20:26Z

some refactoring and nullcheck for nonexistent config file added

Skyllarr · 2019-08-29T13:20:35Z

rebased and unused import deleted

Skyllarr · 2019-11-12T08:49:57Z

@jimma please review

...a/org/jboss/test/ws/jaxws/cxf/clientConfig/CXFClientConfigElytronNotOnClasspathTestCase.java

jimma · 2019-11-28T14:46:38Z

...a/org/jboss/test/ws/jaxws/cxf/clientConfig/CXFClientConfigElytronNotOnClasspathTestCase.java

+   @RunAsClient
+   public void testClientWithoutElytronOnClasspath() throws Exception {
+      List<String> list = runJBossModulesClient();
+      assertTrue(true);


Should this line be removed ?

@jimma yes i will remove

...a/org/jboss/test/ws/jaxws/cxf/clientConfig/CXFClientConfigElytronNotOnClasspathTestCase.java

…t for Authentication / SSL

jbliznak reviewed Jul 25, 2019

View reviewed changes

Skyllarr force-pushed the WS_client_Elytron_integration branch from 3f1f3bd to ab7d385 Compare July 25, 2019 09:23

jbliznak reviewed Jul 25, 2019

View reviewed changes

Skyllarr force-pushed the WS_client_Elytron_integration branch 3 times, most recently from 99d5ef5 to 52dd1c9 Compare July 26, 2019 07:19

Skyllarr changed the title ~~[EAP7-1097] WS integration with WildFly Elytron - AuthenticationClien…~~ [JBWS-4179] WS integration with WildFly Elytron - AuthenticationClien… Jul 26, 2019

Skyllarr force-pushed the WS_client_Elytron_integration branch 5 times, most recently from fa8a455 to d089b8d Compare August 6, 2019 10:56

jbliznak reviewed Aug 7, 2019

View reviewed changes

Skyllarr force-pushed the WS_client_Elytron_integration branch from d089b8d to b1c390a Compare August 7, 2019 13:37

Skyllarr force-pushed the WS_client_Elytron_integration branch 8 times, most recently from 4862b97 to 7f88c61 Compare August 13, 2019 13:54

Skyllarr force-pushed the WS_client_Elytron_integration branch 2 times, most recently from 3693527 to ebc24d2 Compare August 13, 2019 16:43

jbliznak suggested changes Aug 13, 2019

View reviewed changes

jbliznak reviewed Aug 13, 2019

View reviewed changes

Skyllarr force-pushed the WS_client_Elytron_integration branch 4 times, most recently from 7e42071 to 7b8ee02 Compare August 13, 2019 21:16

Skyllarr force-pushed the WS_client_Elytron_integration branch from 7b8ee02 to b57bcc3 Compare August 21, 2019 11:52

Skyllarr force-pushed the WS_client_Elytron_integration branch from b57bcc3 to 527d7c6 Compare August 26, 2019 15:59

jbliznak approved these changes Aug 27, 2019

View reviewed changes

Skyllarr force-pushed the WS_client_Elytron_integration branch 2 times, most recently from b13fc26 to d19d22c Compare August 28, 2019 16:02

Skyllarr force-pushed the WS_client_Elytron_integration branch 2 times, most recently from e75d245 to 40ec143 Compare August 29, 2019 13:17

Skyllarr force-pushed the WS_client_Elytron_integration branch 2 times, most recently from 07e2de8 to 5b6ca72 Compare October 11, 2019 20:03

Skyllarr force-pushed the WS_client_Elytron_integration branch 3 times, most recently from 26d1376 to 11758e3 Compare November 25, 2019 14:25

jimma reviewed Nov 28, 2019

View reviewed changes

...a/org/jboss/test/ws/jaxws/cxf/clientConfig/CXFClientConfigElytronNotOnClasspathTestCase.java Show resolved Hide resolved

jimma reviewed Nov 28, 2019

View reviewed changes

[JBWS-4179] WS integration with WildFly Elytron - AuthenticationClien…

72cea9d

…t for Authentication / SSL

Skyllarr force-pushed the WS_client_Elytron_integration branch from 11758e3 to 72cea9d Compare November 28, 2019 14:51

jimma merged commit 3119ac6 into jbossws:master Nov 29, 2019

[JBWS-4179] WS integration with WildFly Elytron - AuthenticationClien… #108

[JBWS-4179] WS integration with WildFly Elytron - AuthenticationClien… #108

Conversation

Skyllarr commented Jul 24, 2019 • edited Loading

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

Choose a reason for hiding this comment

jbliznak left a comment • edited Loading

Choose a reason for hiding this comment

Skyllarr commented Aug 7, 2019

Skyllarr commented Aug 13, 2019

jbliznak left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimma commented Aug 15, 2019

Skyllarr commented Aug 15, 2019 • edited Loading

jimma commented Aug 15, 2019

Skyllarr commented Aug 15, 2019

Skyllarr commented Aug 21, 2019

Skyllarr commented Aug 26, 2019

Skyllarr commented Aug 28, 2019

Skyllarr commented Aug 29, 2019

Skyllarr commented Nov 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skyllarr commented Jul 24, 2019 •

edited

Loading

jbliznak left a comment •

edited

Loading

Skyllarr commented Aug 15, 2019 •

edited

Loading