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

Adjust reload keystore test to pass in FIPS #57050

Merged
merged 1 commit into from
May 26, 2020

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented May 21, 2020

In KeystoreWrapper class we determine if the error to decrypt a
given keystore is caused by a wrong password based on the exception
that the SunJCE implementation of AES is throwing
(AEADBadTagException). Other implementations from other Security
Providers might cause decryption to fail in a different way and cause
us to throw a generic error message.
We handle this in this test by matching both possible
exception messages.

Relates: #56889

In KeystoreWrapper class we determine if the error to decrypt a
given keystore is caused by a wrong password based on the exception
that the SunJCE implementation of AES is throwing
(AEADBadTagException). Other implementations from other Security
Providers fail with a different exception and as such we cannot
differentiate between a corrupted file and a wrong passwordin a
foolproof way.
we handle this in this test by matching both possible
exception messages.

Relates: elastic#56889
@jkakavas jkakavas added :Core/Infra/Settings Settings infrastructure and APIs >test-failure Triaged test failures from CI :Security/Security Security issues without another label labels May 21, 2020
@jkakavas jkakavas requested a review from ywangd May 21, 2020 16:15
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Settings)

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label May 21, 2020
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Security Meta label for security team label May 21, 2020
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM. This matches with the check we do in packaging tests.

One question: should we change the exception catching code to still give a good error message for common security providers?

Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM. Also second @rjernst's question. Maybe we could list "wrong password" alongside "corrupted or tampered with"?

@jkakavas
Copy link
Member Author

One question: should we change the exception catching code to still give a good error message for common security providers?

Yes, I will look into it. This Q prompted me to do some initial investigation and I might be mistaken here regarding the cause of the different Exceptions thrown. I now think this has to do with #28515 and how read() and readFully() operates on the CipherInputStream based on the Cipher implementation (read: the SecurityProvider that implements it ) . It turns out that we throw on

throw new SecurityException("Keystore has been corrupted or tampered with");
and not on
throw new SecurityException("Keystore has been corrupted or tampered with", e);
as I assumed. We added the original code in #28515 and then check for the AEADBadTagException later on in the reloading secure settings work and never made the connection nor investigated it in detail until now.

I'd still like to merge this with an updated description to make CI green and then spend the proper time to figure out how we can handle the differences in a consistent way so that we can throw a proper error in all applicable cases

Also second @rjernst's question. Maybe we could list "wrong password" alongside "corrupted or tampered with"?

I wouldn't like us returning an "Error, this might be this or that or the other thing" so I think we could do our best to match expected

@jkakavas
Copy link
Member Author

I opened #57132

@jkakavas jkakavas merged commit afbd8ca into elastic:master May 26, 2020
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request May 26, 2020
In KeystoreWrapper class we determine if the error to decrypt a
given keystore is caused by a wrong password based on the exception
that the SunJCE implementation of AES is throwing
(AEADBadTagException). Other implementations from other Security
Providers might cause decryption to fail in a different way and cause
us to throw a generic error message.
We handle this in this test by matching both possible
exception messages.

Relates: elastic#56889
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request May 26, 2020
In KeystoreWrapper class we determine if the error to decrypt a
given keystore is caused by a wrong password based on the exception
that the SunJCE implementation of AES is throwing
(AEADBadTagException). Other implementations from other Security
Providers might cause decryption to fail in a different way and cause
us to throw a generic error message.
We handle this in this test by matching both possible
exception messages.

Relates: elastic#56889
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request May 26, 2020
In KeystoreWrapper class we determine if the error to decrypt a
given keystore is caused by a wrong password based on the exception
that the SunJCE implementation of AES is throwing
(AEADBadTagException). Other implementations from other Security
Providers might cause decryption to fail in a different way and cause
us to throw a generic error message.
We handle this in this test by matching both possible
exception messages.

Relates: elastic#56889
jkakavas added a commit that referenced this pull request May 26, 2020
In KeystoreWrapper class we determine if the error to decrypt a
given keystore is caused by a wrong password based on the exception
that the SunJCE implementation of AES is throwing
(AEADBadTagException). Other implementations from other Security
Providers might cause decryption to fail in a different way and cause
us to throw a generic error message.
We handle this in this test by matching both possible
exception messages.

Relates: #56889
jkakavas added a commit that referenced this pull request May 26, 2020
In KeystoreWrapper class we determine if the error to decrypt a
given keystore is caused by a wrong password based on the exception
that the SunJCE implementation of AES is throwing
(AEADBadTagException). Other implementations from other Security
Providers might cause decryption to fail in a different way and cause
us to throw a generic error message.
We handle this in this test by matching both possible
exception messages.

Relates: #56889
jkakavas added a commit that referenced this pull request May 26, 2020
In KeystoreWrapper class we determine if the error to decrypt a
given keystore is caused by a wrong password based on the exception
that the SunJCE implementation of AES is throwing
(AEADBadTagException). Other implementations from other Security
Providers might cause decryption to fail in a different way and cause
us to throw a generic error message.
We handle this in this test by matching both possible
exception messages.

Relates: #56889
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Settings Settings infrastructure and APIs :Security/Security Security issues without another label Team:Core/Infra Meta label for core/infra team Team:Security Meta label for security team >test-failure Triaged test failures from CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants