[WFLY-18476] helloworld-mutual-ssl Quickstart Common Enhancements CY2023Q3 #727

PrarthonaPaul · 2023-10-06T15:41:23Z

https://issues.redhat.com/browse/WFLY-18476

openshift-ci · 2023-10-06T15:41:33Z

Hi @PrarthonaPaul. Thanks for your PR.

I'm waiting for a wildfly member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

emmartins

Besides changes pointed in PR please change also the following:

quickstart/helloworld-mutual-ssl/pom.xml

Line 118 in 0cea65f

<version>${version.server.bom}</version>

this should use property named "version.bom.ee"

helloworld-mutual-ssl/README.adoc

helloworld-mutual-ssl/pom.xml

...-mutual-ssl/src/test/java/org/jboss/as/quickstarts/helloworld_mutual_ssl/BasicRuntimeIT.java

helloworld-mutual-ssl/pom.xml

emmartins · 2023-10-09T12:20:19Z

.github/workflows/quickstart_helloworld_mutual_ssl.yml

@@ -0,0 +1,15 @@
+name: WildFly helloworld-mutual-ssl Quickstart CI


The filename should be "quickstart_helloworld-mutual-ssl_ci.yml" and this Quickstart actually requires additional functionality in the abstract reused quickstart_ci.yml, which I will add later ( https://issues.redhat.com/browse/WFLY-18612 ), for now please use the following code:

name: WildFly helloworld-mutual-ssl Quickstart CI on: pull_request: types: [opened, synchronize, reopened, ready_for_review] paths: - helloworld-mutual-ssl/**' # Only run the latest job concurrency: group: '${{ github.workflow }} @ ${{ github.ref || github.run_id }}' cancel-in-progress: true jobs: Test-build-default-matrix: name: BUILD DEFAULT - JDK${{ matrix.jdk }} - ${{ matrix.os }} runs-on: ${{ matrix.os }} strategy: fail-fast: false matrix: jdk: [11, 17] os: [ubuntu-20.04, windows-latest] steps: - uses: actions/checkout@v4 with: path: quickstarts - name: Set up JDK ${{ matrix.jdk }} uses: actions/setup-java@v3 with: java-version: ${{ matrix.jdk }} distribution: 'temurin' cache: 'maven' - name: Build ${{ inputs.QUICKSTART_PATH }} Quickstart for Release run: | cd quickstarts cd helloworld-mutual-ssl mvn -U -B -fae clean install -Drelease shell: bash - name: Build, run & test helloworld-mutual-ssl Quickstart with provisioned-server profile run: | cd quickstarts cd helloworld-mutual-ssl echo "Building provisioned server..." mvn -U -B -fae clean package -Pprovisioned-server echo "Starting provisioned server..." mvn -U -B -fae wildfly:start -DjbossHome=target/server -Dstartup-timeout=120 # echo "Testing provisioned server..." # mvn -U -B -fae verify -Dserver.host=https://localhost:8080 -Pintegration-testing echo "Shutting down provisioned server..." mvn -U -B -fae wildfly:shutdown shell: bash - name: Build helloworld-mutual-ssl Quickstart with openshift profile run: | cd quickstarts cd helloworld-mutual-ssl mvn -U -B -fae clean package -Popenshift shell: bash - uses: actions/upload-artifact@v3 if: failure() with: name: surefire-reports-JDK${{ matrix.jdk }}-${{ matrix.os }} path: 'quickstarts/helloworld-mutual-ssl/**/surefire-reports/*.txt' Test-build-with-deps-matrix: name: BUILD WITH DEPS - JDK${{ matrix.jdk }} - ${{ matrix.os }} runs-on: ${{ matrix.os }} strategy: fail-fast: false matrix: jdk: [11, 17] os: [ubuntu-20.04, windows-latest] steps: - uses: actions/checkout@v4 with: repository: wildfly/wildfly ref: ${{ github.base_ref }} path: wildfly - uses: actions/checkout@v4 with: path: quickstarts - name: Set up JDK ${{ matrix.jdk }} uses: actions/setup-java@v3 with: java-version: ${{ matrix.jdk }} distribution: 'temurin' cache: 'maven' - name: Build Server run: | cd wildfly mvn -U -B -fae -DskipTests clean install shell: bash - name: Get Server Version run: | cd wildfly echo "VERSION_SERVER=$(mvn -N org.apache.maven.plugins:maven-help-plugin:3.2.0:evaluate -Dexpression=project.version -q -DforceStdout)" >> $GITHUB_ENV shell: bash - name: Build Quickstart for Release with built Server version run: | cd quickstarts cd helloworld-mutual-ssl mvn -U -B -fae clean package -Drelease -Dversion.server=${{ env.VERSION_SERVER }} shell: bash - name: Build, run & test helloworld-mutual-ssl Quickstart with provisioned-server profile, and built Server version run: | cd quickstarts cd helloworld-mutual-ssl echo "Building provisioned server..." mvn -U -B -fae clean package -Pprovisioned-server -Dversion.server=${{ env.VERSION_SERVER }} echo "Starting provisioned server..." mvn -U -B -fae wildfly:start -DjbossHome=target/server -Dstartup-timeout=120 # echo "Testing provisioned server..." # mvn -U -B -fae verify -Dserver.host=http://localhost:8080 -Pintegration-testing echo "Shutting down provisioned server..." mvn -U -B -fae wildfly:shutdown shell: bash - name: Build helloworld-mutual-ssl Quickstart with openshift profile, and built Server version run: | cd quickstarts cd helloworld-mutual-ssl mvn -U -B -fae clean package -Popenshift -Dversion.server=${{ env.VERSION_SERVER }} shell: bash - uses: actions/upload-artifact@v3 if: failure() with: name: surefire-reports-JDK${{ matrix.jdk }}-${{ matrix.os }} path: 'quickstarts/**/surefire-reports/*.txt'

all features this QS needs are now available, so please change this file to:

name: WildFly helloworld-mutual-ssl Quickstart CI on: pull_request: types: [opened, synchronize, reopened, ready_for_review] paths: - 'helloworld-mutual-ssl/**' - '.github/workflows/quickstart_ci.yml' jobs: call-quickstart_ci: uses: ./.github/workflows/quickstart_ci.yml with: QUICKSTART_PATH: helloworld-mutual-ssl SERVER_PROVISIONING_SERVER_HOST: https://localhost:8443/ TEST_PROVISIONED_SERVER: true TEST_OPENSHIFT: false

for the ci, we may need to add the path to server config directory using the -Djboss.server.config.dir=path/to/standalone/configuration param. Would this be part of this script?

emmartins · 2023-10-10T20:21:53Z

No, in such case you may skip the whole cloud enhancements (helm chart, openshift maven profile, the readme instructions), and remove the build of the openshift profile on your CI workflow yml.

…

On Tue, 10 Oct 2023 at 20:00, Prarthona Paul ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In helloworld-mutual-ssl/src/test/java/org/jboss/as/quickstarts/helloworld_mutual_ssl/BasicRuntimeIT.java <#727 (comment)>: > + */ +public class BasicRuntimeIT { + + private static final String DEFAULT_SERVER_HOST = "http://localhost:8080"; + + @test + public void testHTTPEndpointIsAvailable() throws IOException, InterruptedException, URISyntaxException { + String serverHost = System.getenv("SERVER_HOST"); + if (serverHost == null) { + serverHost = System.getProperty("server.host"); + } + if (serverHost == null) { + serverHost = DEFAULT_SERVER_HOST; + } + final HttpRequest request = HttpRequest.newBuilder() + .uri(new URI(serverHost+"/")) I have tested it with both .uri(new URI(serverHost+"/")) and .uri(new URI(serverHost+"/HelloWorld")) and the former seems to pass the tests while the latter gives the following errors: [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.217 s <<< FAILURE! - in org.jboss.as.quickstarts.helloworld_mutual_ssl.BasicRuntimeIT [ERROR] testHTTPEndpointIsAvailable(org.jboss.as.quickstarts.helloworld_mutual_ssl.BasicRuntimeIT) Time elapsed: 0.196 s <<< FAILURE! java.lang.AssertionError: expected:<200> but was:<500> at org.jboss.as.quickstarts.helloworld_mutual_ssl.BasicRuntimeIT.testHTTPEndpointIsAvailable(BasicRuntimeIT.java:57) When I ran the quickstart following the README file, I saw that https://localhost:8443/helloworld-mutual-ssl redirects to https://localhost:8443/helloworld-mutual-ssl/HelloWorld, so I am not sure why the tests are giving 500 error. If you have any suggestions, please feel free to let me know. Or if leaving it at .uri(new URI(serverHost+"/")) works, then I can change it to that too. Thank you for reviewing the PR :) — Reply to this email directly, view it on GitHub <#727 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEDY6ARDQPWRGQLAWPKVATX6WLOFAVCNFSM6AAAAAA5WAIKHOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNRYGY3TANZWGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

emmartins · 2023-10-10T20:23:43Z

I will have a look at it tomorrow morning (in around 10h).

…

On Tue, 10 Oct 2023 at 21:21, Eduardo Martins ***@***.***> wrote: No, in such case you may skip the whole cloud enhancements (helm chart, openshift maven profile, the readme instructions), and remove the build of the openshift profile on your CI workflow yml. On Tue, 10 Oct 2023 at 20:00, Prarthona Paul ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In > helloworld-mutual-ssl/src/test/java/org/jboss/as/quickstarts/helloworld_mutual_ssl/BasicRuntimeIT.java > <#727 (comment)>: > > > + */ > +public class BasicRuntimeIT { > + > + private static final String DEFAULT_SERVER_HOST = "http://localhost:8080"; > + > + @test > + public void testHTTPEndpointIsAvailable() throws IOException, InterruptedException, URISyntaxException { > + String serverHost = System.getenv("SERVER_HOST"); > + if (serverHost == null) { > + serverHost = System.getProperty("server.host"); > + } > + if (serverHost == null) { > + serverHost = DEFAULT_SERVER_HOST; > + } > + final HttpRequest request = HttpRequest.newBuilder() > + .uri(new URI(serverHost+"/")) > > I have tested it with both .uri(new URI(serverHost+"/")) and .uri(new > URI(serverHost+"/HelloWorld")) and the former seems to pass the tests > while the latter gives the following errors: > > [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.217 s <<< FAILURE! - in org.jboss.as.quickstarts.helloworld_mutual_ssl.BasicRuntimeIT > [ERROR] testHTTPEndpointIsAvailable(org.jboss.as.quickstarts.helloworld_mutual_ssl.BasicRuntimeIT) Time elapsed: 0.196 s <<< FAILURE! > java.lang.AssertionError: expected:<200> but was:<500> > at org.jboss.as.quickstarts.helloworld_mutual_ssl.BasicRuntimeIT.testHTTPEndpointIsAvailable(BasicRuntimeIT.java:57) > > When I ran the quickstart following the README file, I saw that > https://localhost:8443/helloworld-mutual-ssl redirects to > https://localhost:8443/helloworld-mutual-ssl/HelloWorld, so I am not > sure why the tests are giving 500 error. > If you have any suggestions, please feel free to let me know. > Or if leaving it at .uri(new URI(serverHost+"/")) works, then I can > change it to that too. > Thank you for reviewing the PR :) > > — > Reply to this email directly, view it on GitHub > <#727 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAEDY6ARDQPWRGQLAWPKVATX6WLOFAVCNFSM6AAAAAA5WAIKHOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNRYGY3TANZWGA> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

emmartins

Besides the pointed issues, the QS is not working for me, even with standard dist. Anyway I think we all (including @fjuma) agreed that we should truly rework this one, for it to demo usage of the self signed and generated certificate, instead of repeating what we do on the "secured" version of the QS.

helloworld-mutual-ssl/README.adoc

helloworld-mutual-ssl/pom.xml

emmartins · 2023-12-13T15:15:51Z

...-mutual-ssl/src/test/java/org/jboss/as/quickstarts/helloworld_mutual_ssl/BasicRuntimeIT.java

+    private static final String DEFAULT_SERVER_HOST = "https://localhost:8443/helloworld-mutual-ssl";
+
+    @Test
+    public void testHTTPEndpointIsAvailable() throws IOException, InterruptedException, URISyntaxException {


Don't we need certificates on this one too?

Added the same test.
For now it is the same thing as helloworld-mutual-ssl-secured except the URL.
This QS uses https://localhost:8443/helloworld-mutual-ssl/HelloWorld
@emmartins or @fjuma please let me know how you would like this one to be different than the other QS.
Thanks!

fjuma · 2023-12-13T18:58:27Z

helloworld-mutual-ssl/pom.xml

+    <properties>
+        <!-- the version for the Server -->
+        <version.server>30.0.0.Beta1</version.server>
+      <!-- The versions for BOMs, Packs and Plugins -->


The indentation in this block looks off.

fjuma · 2023-12-13T20:14:43Z

helloworld-mutual-ssl/configure-client-certs.cli

@@ -0,0 +1,18 @@
+#configure a key-store in the Elytron subsystem. The path to the keystore file doesn’t actually have to exist yet.


I think the same comments I mentioned in the helloworld-mutual-ssl-secured quickstart apply here as well.

fjuma · 2023-12-13T22:25:03Z

helloworld-mutual-ssl/configure-client-certs.cli

+# Export the certificate to a file called clientCert.crt
+/subsystem=elytron/key-store=clientKS:export-certificate(alias=quickstartUser, path=clientCert.crt, relative-to=jboss.server.config.dir, pem=true)
+
+# Create a the server's truststore


s/a the/the

fjuma · 2023-12-13T22:25:27Z

helloworld-mutual-ssl/configure-client-certs.cli

+# Create a the server's truststore
+/subsystem=elytron/key-store=serverTS:add(path=server.truststore, relative-to=jboss.server.config.dir, credential-reference={clear-text=secret}, type=PKCS12)
+
+# Import a certificate into the server's truststore


s/a certificate/the client's certificate

fjuma · 2023-12-13T22:26:04Z

helloworld-mutual-ssl/configure-ssl.cli

 /subsystem=elytron/trust-manager=qsTrustManager:add(key-store=qsTrustStore)

 # Update the default server-ssl-context to reference the new trust-manager and require client auth
 /subsystem=elytron/server-ssl-context=applicationSSC:write-attribute(name=trust-manager, value=qsTrustManager)
 /subsystem=elytron/server-ssl-context=applicationSSC:write-attribute(name=need-client-auth, value=true)

+#generate the key pair that the server would use for its keystore


The comments in this file should be updated too.

fixed!
Thanks for reviewing!

fjuma · 2023-12-15T20:23:02Z

helloworld-mutual-ssl/configure-client-certs.cli

@@ -0,0 +1,18 @@
+# Configure a key-store in the Elytron subsystem. The path to the keystore file doesn’t actually have to exist yet.


Just to make it more clear that this is just for the client's cert, maybe can reword to:

Configure the client's keystore. This will be used to generate the client's certificate. The path to the keystore file doesn’t actually have to exist yet

fjuma · 2023-12-15T20:23:39Z

helloworld-mutual-ssl/configure-client-certs.cli

+# Generate a new key pair for the client. We'll use an RSA key of size 2048 and we'll use CN=quickstartUser
+/subsystem=elytron/key-store=clientKS:generate-key-pair(alias=quickstartUser, algorithm=RSA, key-size=2048, validity=365, credential-reference={clear-text=secret}, distinguished-name="cn=quickstartUser")
+
+# Export the certificate to a file called clientCert.crt


s/the certificate/the client's certificate

fjuma · 2023-12-15T20:30:49Z

helloworld-mutual-ssl/configure-client-certs.cli

@@ -0,0 +1,18 @@
+# Configure a key-store in the Elytron subsystem. The path to the keystore file doesn’t actually have to exist yet.


Just minor, but configure-client-certs.cli could be renamed to just configure-client-cert.cli.

FIxed!
Thank you!

emmartins · 2023-12-21T11:08:14Z

WFLY 31.0.0.Beta1 has been released, please rebase this PR with upstream/main branch, and update:

All pom.xml to 31.0.0.Final-SNAPSHOT
All pom.xml to 6
Property named version.server to 31.0.0.Beta1
Property named version.plugin.wildfly to 4.2.1.Final

…023Q3

PrarthonaPaul · 2023-12-21T14:26:14Z

Hi @emmartins
I have made the version updates to the pom.xml file.
This QS should be good to go! Thanks

… use a CLI script instead of keytool

emmartins

There are changes needed, but let's do it after this merge.

emmartins · 2024-03-25T10:24:35Z

helloworld-mutual-ssl/README.adoc

----
-$ cd __{jbossHomeName}__/standalone/configuration/
----
+== Set Up the Client Keystore


this section should be moved to after server is started

emmartins · 2024-03-25T11:16:49Z

...-mutual-ssl/src/test/java/org/jboss/as/quickstarts/helloworld_mutual_ssl/BasicRuntimeIT.java

+        if (serverHost == null) {
+            serverHost = DEFAULT_SERVER_HOST;
+        }
+        String serverDir  = System.getenv("SERVER_HOME");


The need for this extra property is not documented in the README, and IMHO it's a bit confusing, I think jbossHome property (matching what is used by the wildfly plugin when starting the server) would be an improvement but if possible wouldn't be simpler to generate the client.keystore.P12 in the user dir, and have the test assume it's at user.dir... if not possible perhaps an extra step to copy client.keystore.P12 to the user.dir?

emmartins · 2024-03-25T11:18:29Z

helloworld-mutual-ssl/pom.xml

+        <dependency>
+            <groupId>org.apache.httpcomponents</groupId>
+            <artifactId>httpclient</artifactId>
+            <version>4.5.13</version>


Please remove version, it's managed by our BOM, and add test scope.

It looks like these changes have been addressed already.
Thanks @emmartins !

openshift-ci bot added the needs-ok-to-test label Oct 6, 2023

PrarthonaPaul force-pushed the helloworld-mutual-ssl-enhancement branch from 32b3ffd to 0cea65f Compare October 6, 2023 16:29

emmartins requested changes Oct 9, 2023

View reviewed changes

PrarthonaPaul force-pushed the helloworld-mutual-ssl-enhancement branch 2 times, most recently from ffbf7e4 to 9522501 Compare October 10, 2023 18:41

PrarthonaPaul force-pushed the helloworld-mutual-ssl-enhancement branch 3 times, most recently from f70a7ee to c035ee5 Compare October 16, 2023 19:28

emmartins requested changes Dec 13, 2023

View reviewed changes

PrarthonaPaul force-pushed the helloworld-mutual-ssl-enhancement branch 4 times, most recently from ef578e2 to 134947c Compare December 13, 2023 21:03

fjuma requested changes Dec 13, 2023

View reviewed changes

PrarthonaPaul force-pushed the helloworld-mutual-ssl-enhancement branch from 134947c to fe3de9c Compare December 15, 2023 14:51

fjuma reviewed Dec 15, 2023

View reviewed changes

PrarthonaPaul force-pushed the helloworld-mutual-ssl-enhancement branch 2 times, most recently from 71e7166 to 83d5ac2 Compare December 20, 2023 14:45

[WFLY-18476] helloworld-mutual-ssl Quickstart Common Enhancements CY2…

309d2e5

…023Q3

PrarthonaPaul force-pushed the helloworld-mutual-ssl-enhancement branch from 743c174 to 29d854f Compare December 21, 2023 14:17

Replaced old client cert generation instructions with instructions to…

6b83c27

… use a CLI script instead of keytool

PrarthonaPaul force-pushed the helloworld-mutual-ssl-enhancement branch from 29d854f to 6b83c27 Compare December 21, 2023 20:33

emmartins approved these changes Mar 25, 2024

View reviewed changes

emmartins merged commit 10d4b0c into wildfly:main Mar 25, 2024
11 of 19 checks passed

		@@ -0,0 +1,15 @@
		name: WildFly helloworld-mutual-ssl Quickstart CI

		@@ -0,0 +1,18 @@
		#configure a key-store in the Elytron subsystem. The path to the keystore file doesn’t actually have to exist yet.

		@@ -0,0 +1,18 @@
		# Configure a key-store in the Elytron subsystem. The path to the keystore file doesn’t actually have to exist yet.

[WFLY-18476] helloworld-mutual-ssl Quickstart Common Enhancements CY2023Q3 #727

[WFLY-18476] helloworld-mutual-ssl Quickstart Common Enhancements CY2023Q3 #727

Conversation

PrarthonaPaul commented Oct 6, 2023

openshift-ci bot commented Oct 6, 2023

emmartins left a comment

Choose a reason for hiding this comment

emmartins Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmartins Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmartins commented Oct 10, 2023 via email

emmartins commented Oct 10, 2023 via email

emmartins 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

PrarthonaPaul Dec 15, 2023 • 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

emmartins commented Dec 21, 2023

PrarthonaPaul commented Dec 21, 2023

emmartins 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

emmartins Oct 9, 2023 •

edited

Loading

emmartins Dec 13, 2023 •

edited

Loading

PrarthonaPaul Dec 15, 2023 •

edited

Loading