Skip to content

Conversation

@tvernum
Copy link
Contributor

@tvernum tvernum commented Aug 3, 2021

This commit removes the PemUtils and DerParser classes from X-Pack and
converts all use of those classes to the versions in libs/ssl-config

Relates: #68719

This commit removes the PemUtils and DerParser classes from X-Pack and
converts all use of those classes to the versions in libs/ssl-config

Relates: elastic#68719
@tvernum tvernum requested a review from BigPandaToo August 3, 2021 08:50
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Aug 3, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

+ cert.getClass() + ")");
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't exactly in scope for this PR, but a test case was using the new method so it made sense to add it now..

} catch (AccessControlException securityException) {
throw blockedKeyConfigFile(securityException, environment, KEY_FILE, key);
} catch (GeneralSecurityException e) {
throw new IllegalStateException("Error parsing Private Key from: " + keyPath, e);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The X-Pack PemUtils would throw this IllegalStateException on GeneralSecurityException but the ssl-config version throws GeneralSecurityException.
This isn't the nicest way to handle it, but my plan is to get rid of this class (PEMKeyConfig) entirely, so I opted for the smallest possible change.

assertThat(exception, throwableWithMessage(
"failed to initialize SSL " + sslManagerType + " - " + fileType + " file [" + fileName + "] does not exist"));
assertThat(exception, instanceOf(ElasticsearchException.class));
// This is needed temporarily while we're converting from X-Pack SSL to libs/ssl-config
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we've switched the PEM file reading over to ssl-config, but other scenarios are still using X-Pack, there's a mix of error messages in use.
This if is a temporary measure to keep this PR small, until we unify everything to depend on ssl-config.

@tvernum
Copy link
Contributor Author

tvernum commented Aug 4, 2021

@elasticmachine update branch

Copy link
Contributor

@jkakavas jkakavas left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@BigPandaToo BigPandaToo left a comment

Choose a reason for hiding this comment

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

LGTM

@tvernum tvernum merged commit b371463 into elastic:master Aug 5, 2021
@tvernum tvernum deleted the feature/merge-ssl-config/pemutils branch August 5, 2021 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>refactoring :Security/TLS SSL/TLS, Certificates Team:Security Meta label for security team v8.0.0-alpha2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants