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

Migrate more code away from X-Pack SSL #76142

Merged
merged 10 commits into from
Aug 17, 2021

Conversation

tvernum
Copy link
Contributor

@tvernum tvernum commented Aug 5, 2021

This commit is a bundle of changes to support the removal of X-Pack
SSL in favour of the ssl-config library.

The main changes are:

  1. Migrating some certificate management in PKI and SAML realm to use
    ssl-config
  2. Updating a variety of test cases to use ssl-config for their SSL
    setup and verification

Relates: #68719

This commit is a bundle of changes to support the removal of X-Pack
SSL in favour of the ssl-config library.

The main changes are:
1. Migrating some certificate management in PKI and SAML realm to use
   ssl-config
2. Updating a variety of test cases to use ssl-config for their SSL
   setup and verification
@tvernum tvernum requested review from jkakavas and BigPandaToo August 5, 2021 06:52
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Aug 5, 2021
@elasticmachine
Copy link
Collaborator

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

@tvernum
Copy link
Contributor Author

tvernum commented Aug 5, 2021

This should be the last PR before we make the big cut over.
#72285 still looked a bit big (89 files), and I think this change should cut it down a bit.

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.

Just one nit comment; LGTM otherwise

@tvernum
Copy link
Contributor Author

tvernum commented Aug 16, 2021

@elasticmachine update branch

@tvernum
Copy link
Contributor Author

tvernum commented Aug 17, 2021

@jkakavas Do you want to review this one before I merge?

@jkakavas
Copy link
Member

jkakavas commented Aug 17, 2021 via email

Copy link
Member

@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 Tim, thanks for taking the time to break this down into consumable set of changes

if (keyConfig.hasKeyMaterial() == false) {
return null;
}
final X509KeyManager keyManager = keyConfig.createKeyManager();
if (keyManager == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit but it just caught my eye while scrolling. Only EmptyKeyConfig#createKeyManager can return null and if this is an EmptyKeyConfig then we would have returned already because of if (keyConfig.hasKeyMaterial() == false) { above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we want every use of KeyConfig to know that the case.
I suppose we could document a contract that is hasKeyMaterial is false, then createKeyManager may not return null, but otherwise I don't think it makes sense for SamlRealm to know about all the possible behaviours of KeyConfig and what the return value from one method might would imply about others.

@tvernum tvernum merged commit 5aa4a94 into elastic:master Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :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