RANGER-5734: Update EmbeddedServer to use custom TrustStore - #1188
RANGER-5734: Update EmbeddedServer to use custom TrustStore#1188vikaskr22 wants to merge 2 commits into
Conversation
| truststorePass = CredentialReader.getDecryptedString(providerPath.trim(), truststoreAlias.trim(), EmbeddedServerUtil.getConfig("ranger.truststore.file.type", RANGER_TRUSTSTORE_FILE_TYPE_DEFAULT)); | ||
|
|
||
| if (StringUtils.isBlank(truststorePass) || "none".equalsIgnoreCase(truststorePass.trim())) { | ||
| truststorePass = EmbeddedServerUtil.getConfig("ranger.service.https.attrib.truststore.pass"); |
There was a problem hiding this comment.
The fallback uses a config key that does not exist anywhere else in the repo:
Ranger’s documented and setup-script-populated property is ranger.truststore.password (see ranger-admin-default-site.xml and security-admin/scripts/setup.sh). A typical install will have:
ranger.truststore.file
ranger.truststore.alias
ranger.truststore.password (plain text in site XML)
With the new code, if credential-store decryption fails or returns "none", the fallback reads a non-existent property, so truststorePass stays blank and the truststore is never applied — even when ranger.truststore.password is configured.
There was a problem hiding this comment.
@pradeepagrawal8184 , I have tried to follow the pattern of getTrustManager() method along with introduction of a new property named ranger.service.https.attrib.truststore.pass .
First it tries to read password from CredentialStore and if it's empty, then fallbacks to above property.
Should we add above property in the ranger-admin-default-site.xml as well ? And should we also need to update the setup.sh script to populate this new property ?
| String truststoreAlias = EmbeddedServerUtil.getConfig("ranger.truststore.alias"); | ||
| String truststorePass = null; | ||
|
|
||
| if (providerPath != null && truststoreAlias != null) { |
There was a problem hiding this comment.
Truststore password resolution nests the plain-text fallback inside the providerPath && truststoreAlias block. If either is missing, truststorePass is never resolved from plain text. That’s inconsistent with keystore behavior and can block valid configs.
Suggested pattern (aligned with keystore + existing Ranger properties):
if (providerPath != null && truststoreAlias != null) { truststorePass = CredentialReader.getDecryptedString(...); } if (StringUtils.isBlank(truststorePass) || "none".equalsIgnoreCase(truststorePass.trim())) { truststorePass = EmbeddedServerUtil.getConfig("ranger.truststore.password"); }
There was a problem hiding this comment.
Agree. I am fixing this.
| ssl.setAttribute("keystorePass", keystorePass); | ||
| ssl.setAttribute("keystoreFile", keystoreFile); | ||
|
|
||
| String trustStoreFile = EmbeddedServerUtil.getConfig("ranger.truststore.file"); |
There was a problem hiding this comment.
Keystore gets upfront validation via validateHttpsKeystore() with a clear LOG.severe on failure. Truststore has no equivalent — a bad path or password only surfaces as a vague info log:
a validateHttpsTruststore() (or reuse of existing loading logic) would give operators actionable errors, similar to keystore.
There was a problem hiding this comment.
@pradeepagrawal8184 , Isn't getTrustManagers() method doing the same thing ? If there is any such misconfigurations, wouldn't it be caught there and logged ?
There was a problem hiding this comment.
then find why do we have getKeyManagers
| ssl.setAttribute("truststorePass", truststorePass); | ||
| ssl.setAttribute("truststoreFile", trustStoreFile); | ||
| } else { | ||
| LOG.info("TrustStore is not set, TrustStoreFile is " + trustStoreFile + " and is TruststorePass empty " + StringUtils.isBlank(truststorePass)); |
There was a problem hiding this comment.
Prefer something like: "Truststore not configured for HTTPS connector (file={}, password={})" at DEBUG, or only log when clientAuth requires it.
|
@pradeepagrawal8184 , I have tried to address the above review comments. Testing is still in-progress but request you to please review it if any other aspects are missing. Thanks. |
| updatePropertyToFilePy $propertyName $newPropertyValue $to_file_kms_site | ||
| $PYTHON_COMMAND_INVOKER ranger_credential_helper.py -l "cred/lib/*" -f "$keystore" -k "$policymgr_https_truststore_credential_alias" -v "$ranger_kms_https_truststore_password" -c 1 | ||
| else | ||
| propertyName=ranger.service.https.attrib.truststore.pass |
There was a problem hiding this comment.
When keystore is set, password is written as _, credential helper runs, then a second block may overwrite with plaintext if the keystore file is missing. security-admin’s version is cleaner (fallback only inside the keystore != "" branch).
There was a problem hiding this comment.
I agree, security-admin approach is cleaner than KMS.
On overwrite if keystore is missing, isn't this expected to write the password in the config file ? Both KMS & Admin will have same outcome, through a different IF-Else branching. Am I missing anything ?
| ssl.setAttribute("truststorePass", truststorePass); | ||
| ssl.setAttribute("truststoreFile", trustStoreFile); | ||
| } else { | ||
| LOG.info("Truststore not configured for HTTPS connector. File=" + trustStoreFile + ", and is trustStorePassword empty " + StringUtils.isBlank(truststorePass)); |
There was a problem hiding this comment.
and is => there is extra space.
may be change that to :
Truststore not configured for HTTPS connector. file={}, passwordEmpty={}".
There was a problem hiding this comment.
@pradeepagrawal8184 , output of this is written to catalina.out for Admin and to stdout.log for KMS. This class EmbeddedServer uses JUL, not SLF4J. You can see the imports. With JUL, placeholder pattern {} is not supported, probably that's why everywhere string concatenation is used.
| if (validationError != null) { | ||
| LOG.warning("HTTPS configuration validation for trustStore failed: " + validationError + " TLS handshaking may fail if mTLS is enabled."); | ||
| } | ||
| ssl.setAttribute("truststorePass", truststorePass); |
There was a problem hiding this comment.
if there is validation failure then probably no need to set.
There was a problem hiding this comment.
@pradeepagrawal8184 , this is the same pattern that we use for keystore. We validate and log but proceed further. I believe idea is to not block the user from using any keystore/truststore .
For example: For trustStore verification, we check the trustStore size, it should not be zero. Similarly we check if it contains any TrustedCertificateEntry. Without this, trustStore is still valid, though empty. And the communication without mTLS will continue to work. That's why i am simply logging at warn.
2 participants
What changes were proposed in this pull request?
embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java class reads and uses custom truststore and updates the defaultSSLContext.
But Ranger-Admin/KMS uses the connector approach and Tomcat's connector doesn't use the keyStore/TrustStore from the defaultContext. Here, code to set keyStore into Tomcat's connector is already available but similar code to set custom trustStore was missing. Hence , it was falling back to JVM's default cacerts.
As part of this PR, code has been added to use custom user provided trustStore.
How was this patch tested?
-mvn build has passed