Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WFLY-18475] helloworld-mutual-ssl-secured Quickstart Common Enhancements CY2023Q3 #729

Conversation

PrarthonaPaul
Copy link

@PrarthonaPaul PrarthonaPaul commented Oct 10, 2023

Jira Issue: https://issues.redhat.com/browse/WFLY-18475
Notes:

  • This QS is not open shift compatible, as a result, the Openshift profile has not been added.
  • When running mvn verify -Pintegration-testing we need to add another param called -Djboss.server.config.dir, where the value for the param is the path to the standalone/configuration directory for the server. mvn verify -Pintegration-testing -Djboss.server.config.dir=target/server/standalone/configuration -Dserver.host=https://localhost:8443 README would need to be updated to reflect this. CI would also need to be updated for this. Currently there is a null pointer exception for getting the system property for DJboss.server.config.dir.
  • New script has been added to create the client truststore from the server keystore. This is used to configure the client when setting up the SSL programmatically.

@openshift-ci
Copy link

openshift-ci bot commented Oct 10, 2023

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.

@PrarthonaPaul PrarthonaPaul force-pushed the helloworld-mutual-ssl-secured-enhancement branch 5 times, most recently from c2ef03a to c5399b6 Compare October 16, 2023 19:26
@PrarthonaPaul PrarthonaPaul force-pushed the helloworld-mutual-ssl-secured-enhancement branch 5 times, most recently from f21448f to abb9e1a Compare October 19, 2023 15:39
@@ -0,0 +1,19 @@
#configure a key-store in the Elytron subsystem. The path to the keystore file doesn’t actually have to exist yet.
/subsystem=elytron/key-store=mutualKS:add(path=clientCert.P12, relative-to=jboss.server.config.dir, credential-reference={clear-text=secret}, type=PKCS12)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should call it clientKS (just to make it more obvious that this is used for the client cert)

Copy link
Contributor

@fjuma fjuma Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path could be client.keystore or something like that

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats true, I will change it to clientKS.
WRT the path, I named it clientCert.p12 to make it consistent with the naming from the README file. In case someone wanted to use the cli script while going through the QS interactively. But I can change it too. Feel free to let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked the README. Looks like we are using "client.keystore" for the path there (see keytool -genkey -keystore client.keystore ...)

/subsystem=elytron/key-store=mutualKS:export-certificate(alias=example, path=clientCert.crt, relative-to=jboss.server.config.dir, pem=true)

#Create a truststore in the elytron subsystem.
/subsystem=elytron/key-store=mutualTS:add(path=client.keystore, relative-to=jboss.server.config.dir, credential-reference={clear-text=secret}, type=PKCS12)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should call this serverTS (just to make it more obvious that this is used for the server's trust store)

The path could be server.truststore or something like that

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, I only named it client.keystore to keep it consistent with the README naming

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked the README, you're right, we were using client.truststore before but I think server.truststore is a better name that reflects the purpose of this file.

For the resource name, we're actually using qsTrustStore in the README so we can either update that to serverTS or just use qsTrustStore here to be consistent.

/subsystem=elytron/key-store=mutualTS:add(path=client.keystore, relative-to=jboss.server.config.dir, credential-reference={clear-text=secret}, type=PKCS12)

# Import a certificate into a truststore
/subsystem=elytron/key-store=mutualTS:import-certificate(alias=example, path=clientCert.crt, relative-to=jboss.server.config.dir, credential-reference={clear-text=secret}, trust-cacerts=true, validate=false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need trust-cacerts to be specified or can we just use the default?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seemed okay without it. I will update the script to remove it.
Thanks for pointing out

@PrarthonaPaul PrarthonaPaul force-pushed the helloworld-mutual-ssl-secured-enhancement branch 2 times, most recently from 100814d to f917738 Compare October 20, 2023 18:30
@PrarthonaPaul PrarthonaPaul force-pushed the helloworld-mutual-ssl-secured-enhancement branch from f917738 to 3b50b5f Compare October 30, 2023 15:04
helloworld/pom.xml Outdated Show resolved Hide resolved
@PrarthonaPaul PrarthonaPaul force-pushed the helloworld-mutual-ssl-secured-enhancement branch 2 times, most recently from 660802c to d3b2a32 Compare November 2, 2023 15:18
@PrarthonaPaul PrarthonaPaul force-pushed the helloworld-mutual-ssl-secured-enhancement branch 2 times, most recently from b69786d to 1d59e40 Compare November 2, 2023 16:20
/subsystem=elytron/key-manager=applicationKM:write-attribute(name=key-store, value=applicationKS)

#add the key-manager to the ssl-context for the server
/subsystem=elytron/server-ssl-context=applicationSSC:write-attribute(name=key-manager, value=applicationKM)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, why is this needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@@ -0,0 +1,8 @@
#remove the keypairs and certificates from the keystore and truststore
/subsystem=elytron/key-store=serverTS:remove-alias(alias=example)
/subsystem=elytron/key-store=clientKS:remove-alias(alias=example)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the alias needs to be updated based on the values used in the cli scripts above.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!
Thanks @fjuma

@PrarthonaPaul PrarthonaPaul force-pushed the helloworld-mutual-ssl-secured-enhancement branch from a0d4b94 to 1aedb66 Compare December 13, 2023 20:51
@emmartins
Copy link
Contributor

I guess this one in under development and not ready to review?

@PrarthonaPaul
Copy link
Author

I guess this one in under development and not ready to review?

A few small things, and should be good afterwards.
I will ping you once it is ready to review.

@PrarthonaPaul PrarthonaPaul force-pushed the helloworld-mutual-ssl-secured-enhancement branch 3 times, most recently from 975c28d to d8bfc54 Compare December 15, 2023 15:53
@@ -0,0 +1,19 @@
# Configure a key-store in the Elytron subsystem. The path to the keystore file doesn’t actually have to exist yet.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/the certificate/the client's certificate

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/a the/the

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/a certificate/the client's certificate

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

@@ -29,10 +29,13 @@ batch
# Add an application-security-domain in the undertow subsystem to map the client_cert_domain from the quickstart app to the http-authentication-factory
/subsystem=undertow/application-security-domain=client_cert_domain:add(http-authentication-factory=quickstart-http-authentication)

#generate the key pair that the server would use for its keystore
Copy link
Contributor

@fjuma fjuma Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generate the server's certificate

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

@@ -44,8 +44,12 @@
</licenses>

<properties>
<!-- The versions for BOMs, Dependencies and Plugins -->
<version.server.bom>30.0.0.Final</version.server.bom>
<!-- Version for the server -->
Copy link
Contributor

@fjuma fjuma Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think <!-- Version for the server --> isn't aligned with the rest.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

* @author <a href="mailto:[email protected]">Prarthona Paul</a>
*/

public class keystoreUtil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The k should be uppercase, KeystoreUtil.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!


public class keystoreUtil {

public static KeyStore loadKeyPairFromKeyStore(String serverDir, String keyStoreFile, String storePassword, String keyAlias, String keyStoreType) throws KeyStoreException {
Copy link
Contributor

@fjuma fjuma Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this method creates a truststore and adds a cert entry to it, maybe it could be called createTrustStore

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!
Thanks @fjuma !
@emmartins This should be good to review once we resolve this comment: #729 (comment)

@PrarthonaPaul PrarthonaPaul force-pushed the helloworld-mutual-ssl-secured-enhancement branch 3 times, most recently from b717e2a to efca5dd Compare December 20, 2023 14:37
@emmartins
Copy link
Contributor

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

@PrarthonaPaul PrarthonaPaul force-pushed the helloworld-mutual-ssl-secured-enhancement branch from 9ec6834 to 9fa90c5 Compare December 21, 2023 14:35
@PrarthonaPaul
Copy link
Author

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

@PrarthonaPaul PrarthonaPaul force-pushed the helloworld-mutual-ssl-secured-enhancement branch from 9fa90c5 to 33d732a Compare December 21, 2023 20:26
@emmartins
Copy link
Contributor

Hi @PrarthonaPaul, this PR required some changes to bring it up in sync with current main branch, plus a few minor enhancements similar to #885 , so I made a replacement PR for this at #886 , please have a look

@PrarthonaPaul
Copy link
Author

Superseded by #886

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants