FISH-230: specify certificate alias for MP rest client and MP config #5445
Conversation
breakponchito
requested review from
OndroMih,
Pandrex247,
pdudits and
rdebusscher
|
Jenkins test |
|
Jenkins test |
|
Jenkins test |
|
Jenkins test |
|
Jenkins test please |
|
|
||
| <artifactId>rest-client-ssl</artifactId> | ||
| <packaging>glassfish-jar</packaging> | ||
| <name>Rest Client ssl context</name> |
There was a problem hiding this comment.
Pendantry - capitals
| <name>Rest Client ssl context</name> | |
| <name>Rest Client SSL Context</name> |
There was a problem hiding this comment.
already fixed, thanks
| String configPath = System.getProperty(PAYARA_BASEDIR_PROPERTY_NAME); | ||
| logger.log(Level.INFO, "Basedir value:"+configPath); | ||
| if (configPath != null) { | ||
| File file = new File(configPath.concat(PAYARA_KEYSTORE_NAME)); |
There was a problem hiding this comment.
PAYARA_KEYSTORE_NAME is hard-coded to /target/test-classes/keystore.jks, so this is by proxy hard-coded to System.getProperty("basedir")/target/test-classes/keystore.jks.
That sounds very specific and more like something that belongs in a test.
There was a problem hiding this comment.
I think this should bind to the server's keystore.
That can likely be achieved by utilizing com.sun.enterprise.security.ssl.SSLUtils (looked up via Globals.get()). I have no idea what either of the methods return over there, but it should give everything except for the chosen alias.
There was a problem hiding this comment.
yeah I returned to the SSLUtils and look better. I said returned because that was my first approach but I changed to add unit testing with mockito because of some static issues I saw previously
| protected SSLContext buildSSlContext(String alias) { | ||
| logger.log(Level.INFO, "Building the SSLContext for the alias"); | ||
| String configPath = System.getProperty(PAYARA_BASEDIR_PROPERTY_NAME); | ||
| logger.log(Level.INFO, "Basedir value:"+configPath); |
There was a problem hiding this comment.
Formatting
| logger.log(Level.INFO, "Basedir value:"+configPath); | |
| logger.log(Level.INFO, "Basedir value:" + configPath); |
There was a problem hiding this comment.
already fixed, thanks
| File file = new File(configPath.concat(PAYARA_KEYSTORE_NAME)); | ||
| logger.log(Level.INFO, file.getPath()); | ||
| try (InputStream is = new FileInputStream(file)) { | ||
| String password = System.getProperty(PAYARA_KEYSTORE_PASSWORD_PROPERTY_NAME); |
There was a problem hiding this comment.
No support for password alias?
Have a look at TranslatedConfigView and its usages around the codebase.
There was a problem hiding this comment.
after returning with the use of the SSLUtils this is not requerid now
|
|
||
| public static final String PAYARA_BASEDIR_PROPERTY_NAME = "basedir"; | ||
|
|
||
| public static final String PAYARA_KEYSTORE_NAME = "/target/test-classes/keystore.jks"; |
There was a problem hiding this comment.
after adding improvements with the use of the SSLUtils this constants were removed
|
|
||
| @Override | ||
| public void onNewClient(Class<?> serviceInterface, RestClientBuilder restClientBuilder) { | ||
| logger.log(Level.INFO,"Evaluating state of the RestClientBuilder after calling build method"); |
There was a problem hiding this comment.
Should this really be INFO?
Seems more like a FINE.
There was a problem hiding this comment.
already fixed, thanks
|
|
||
| if (objectProperty != null && objectProperty instanceof String) { | ||
| String alias = (String) objectProperty; | ||
| logger.log(Level.INFO,"The alias is available from the RestClientBuilder configuration"); |
There was a problem hiding this comment.
"The alias"
What alias? Would be nice to log the name.
And again - should this be INFO? Does a user need to know this as a matter of course?
There was a problem hiding this comment.
already fixed, thanks
| String alias = config.getValue(PAYARA_MP_CONFIG_CLIENT_CERTIFICATE_ALIAS, | ||
| String.class); | ||
| if (alias != null) { | ||
| logger.log(Level.INFO,"The alias is available from the MP Config"); |
There was a problem hiding this comment.
already fixed, thanks
|
|
||
| public static final String PAYARA_MP_CONFIG_CLIENT_CERTIFICATE_ALIAS = "certificate.alias"; | ||
|
|
||
| public static final String PAYARA_BASEDIR_PROPERTY_NAME = "basedir"; |
There was a problem hiding this comment.
This property seems odd to me - what's the purpose of it?
It only seems to get combined with PAYARA_KEYSTORE_NAME and so seems redundant.
basedir also sounds very "maven-y".
There was a problem hiding this comment.
after returning to the use of the SSLUtils now this property was removed
| public class PayaraConstants { | ||
| public static final String PAYARA_REST_CLIENT_CERTIFICATE_ALIAS = "fish.payara.rest.client.certificate.alias"; | ||
|
|
||
| public static final String PAYARA_MP_CONFIG_CLIENT_CERTIFICATE_ALIAS = "certificate.alias"; |
There was a problem hiding this comment.
Is there a reason why we're using separate names for system properties vs. MP config properties?
There was a problem hiding this comment.
From starting requirements was mentioned to use the following combination of properties:
- for JAX-RS: fish.payara.jaxrs.client.certificate.alias
- for MP Rest: fish.payara.rest.client.certificate.alias=mykey
- for MP Config something like: com.mycompany.remoteServices.MyServiceClient/payara/certificate.alias=mykey
In the case of the MP Config after doing test I added on the design wiki just to use the value: certificate.alias=mykey
Do you think this is good or do you prefer to homologate values for example use the same from the MP Rest?
There was a problem hiding this comment.
We can have them different, but I think the MP Config property should at least be prepended with payara. like most of our other properties.
|
Test failure in Jenkins doesn't seem related to this PR since it is happening on other PRs too. |
|
Jenkins test |
|
Jenkins test |
|
Jenkins test |
|
Jenkins test |
|
Jenkins test |
| restClientBuilder.sslContext(customSSLContext); | ||
| } else { | ||
| logger.log(Level.INFO, | ||
| String.format("Although the alias: %s is configured, none keystore available for it", alias)); |
There was a problem hiding this comment.
Readability
| String.format("Although the alias: %s is configured, none keystore available for it", alias)); | |
| String.format("Although the alias: %s is configured, it could not be found in an available keystore", alias)); |
There was a problem hiding this comment.
already fixed
| restClientBuilder.sslContext(customSSLContext); | ||
| } else { | ||
| logger.log(Level.INFO, | ||
| String.format("Although the alias: %s is configured, none keystore available for it", alias)); |
There was a problem hiding this comment.
Readability
| String.format("Although the alias: %s is configured, none keystore available for it", alias)); | |
| String.format("Although the alias: %s is configured, it could not be found in an available keystore", alias)); |
There was a problem hiding this comment.
already fixed
| public class PayaraConstants { | ||
| public static final String PAYARA_REST_CLIENT_CERTIFICATE_ALIAS = "fish.payara.rest.client.certificate.alias"; | ||
|
|
||
| public static final String PAYARA_MP_CONFIG_CLIENT_CERTIFICATE_ALIAS = "certificate.alias"; |
There was a problem hiding this comment.
We can have them different, but I think the MP Config property should at least be prepended with payara. like most of our other properties.
| } | ||
| } | ||
| } catch (NoSuchElementException e) { | ||
| logger.log(Level.SEVERE, String.format("The MP config property %s was not set", |
There was a problem hiding this comment.
Should this be severe? By default it won't be set!
There was a problem hiding this comment.
I moved this to fine because as you mention this is not the default and this is not a potential problem. This only inform that the property was not set when evaluating the MP config
|
Jenkins test |
| Object objectProperty = restClientBuilder.getConfiguration() | ||
| .getProperty(PAYARA_REST_CLIENT_CERTIFICATE_ALIAS); | ||
|
|
||
| if (objectProperty != null && objectProperty instanceof String) { |
There was a problem hiding this comment.
instanceof implies null check
| if (objectProperty != null && objectProperty instanceof String) { | |
| if (objectProperty instanceof String) { |
There was a problem hiding this comment.
already fixed
| * This class is a custom implementation of X509KeyManager to set the custom certificate based on the | ||
| * alias property | ||
| */ | ||
| private class SingleCertificateKeyManager implements X509KeyManager { |
There was a problem hiding this comment.
yes I updated as static, thanks
|
Jenkins test |
|
Jenkins test |
2 similar comments
|
Jenkins test |
|
Jenkins test |
…rtificate-alias-for-jaxrs-and-micro FISH-230: specify certificate alias for MP rest client and MP config
breakponchito
deleted the
FISH-230-specify-certificate-alias-for-jaxrs-and-micro
branch
…rtificate-alias-for-jaxrs-and-micro FISH-230: specify certificate alias for MP rest client and MP config
Description
Adding support for custom configuration of sslContext using certificate alias property
Important Info
Blockers
Testing
New tests
Testing Performed
unit test and manual test documented here: (pending documentation and update of the wiki about design)
I added mock testing for the class
Testing Environment
Windows 10, JDK 8
Documentation
Pending documentation on the wiki page for the user view section: https://payara.atlassian.net/wiki/spaces/PAYAR/pages/3033006087/Feature+Design+-+Provide+Means+to+Specify+a+Certificate+Alias+for+JAX-RS+and+MP+REST+Client
Notes for Reviewers