Improve errors when TLS files cannot be read #44787
Conversation
This change updates the error messages when SSL/TLS is configured using a file that does not exist. It adds tests for the expected error message(s) when various files are missing.
|
Pinging @elastic/es-security |
| } | ||
| if (null == line || PKCS8_FOOTER.equals(line.trim()) == false) { | ||
| throw new IOException("Malformed PEM file, PEM footer is invalid or missing"); | ||
| throw new KeyException("Malformed PEM file, PEM footer is invalid or missing"); |
There was a problem hiding this comment.
This change was needed because readPrivateKey no longer catches & wraps IOException (which, in turn, was needed in order to have specific handling of NoSuchFileException)
jkakavas
left a comment
jkakavas
left a comment
There was a problem hiding this comment.
LGTM, I like this a lot ! How about libs/ssl-config, will you replicate the changes there in a separate PR ?
| } else { | ||
| final String pathString = paths.stream().map(Path::toAbsolutePath).map(Path::toString).collect(Collectors.joining(", ")); | ||
| return new ElasticsearchException( | ||
| "failed to initialize SSL TrustManager - access to read {} files [{}] is blocked;" + |
There was a problem hiding this comment.
| "failed to initialize SSL TrustManager - access to read {} files [{}] is blocked;" + | |
| "failed to initialize SSL TrustManager - access to read one of the {} files [{}] is blocked;" + |
| try { | ||
| return CertParsingUtils.readCertificates(Collections.singletonList(certificate)); | ||
| } catch (FileNotFoundException | NoSuchFileException fileException) { | ||
| throw missingKeyConfigFile(fileException, "certificate", certificate); |
There was a problem hiding this comment.
nit: Maybe have a constant for certificate and truststore in TrustConfig
| try { | ||
| return PemUtils.readPrivateKey(key, keyPassword::getChars); | ||
| } catch (FileNotFoundException | NoSuchFileException fileException) { | ||
| throw missingKeyConfigFile(fileException, "key", key); |
There was a problem hiding this comment.
nit: Maybe have a constant for key and keystore in KeyConfig
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/TrustConfig.java
Show resolved
Hide resolved
It's not my plan. The implementation is sufficiently different there that it would end up being a lot of effort to duplicate the testing & changes for minimal benefit (since ssl-config is not widely used). However, we can't stay in a situation where we are maintaining two overlapping implementations of the same code. |
|
@elasticmachine run elasticsearch-ci/bwc |
This change improves the exception messages that are thrown when the system cannot read TLS resources such as keystores, truststores, certificates, keys or certificate-chains (CAs). This change specifically handles: - Files that do not exist - Files that cannot be read due to file-system permissions - Files that cannot be read due to the ES security-manager Relates: #43079
This change improves the exception messages that are thrown when the system cannot read TLS resources such as keystores, truststores, certificates, keys or certificate-chains (CAs). This change specifically handles: - Files that do not exist - Files that cannot be read due to file-system permissions - Files that cannot be read due to the ES security-manager Relates: elastic#43079 Backport of: elastic#44787
This change improves the exception messages that are thrown when the system cannot read TLS resources such as keystores, truststores, certificates, keys or certificate-chains (CAs). This change specifically handles: - Files that do not exist - Files that cannot be read due to file-system permissions - Files that cannot be read due to the ES security-manager Backport of: #44787
5 participants
This change improves the exception messages that are thrown when the
system cannot read TLS resources such as keystores, truststores,
certificates, keys or certificate-chains (CAs).
This change specifically handles:
Relates: #43079