Gracefully handle exceptions from Security Providers#65464
Gracefully handle exceptions from Security Providers#65464jkakavas merged 7 commits intoelastic:masterfrom
Conversation
In certain situtations, such as when configured in FIPS 140 mode, the Java security provider in use might throw a subclass of java.lang.Error. We currently do not catch these and as a result the JVM exits, shutting down elasticsearch. This commit attempts to address this by catching AssertionErrors that we are aware that might be thrown when a PBKDF2 implementation is used from a Security Provider in FIPS 140 mode, with the password input being less than 14 bytes (112 bits). - In our PBKDF2 family of hashers, we catch the AssertionError and throw an ElasticsearchException while creating or verifying the hash. We throw on verification instead of simply returning false on purpose so that the message bubbles up and the cause becomes obvious (otherwise it would be indistinguishable from a wrong password). - In KeyStoreWrapper, we catch the AssertionError in order to wrap and rethrow with a helpful message. This can happen when using any of the keystore CLI commands, when the node starts or when we attempt to reload secure settings. - In the `elasticsearch-users` tool, we catch the AssertionError and throw an appropriate UserException.
|
Pinging @elastic/es-security (Team:Security) |
.../tools/keystore-cli/src/main/java/org/elasticsearch/common/settings/BaseKeyStoreCommand.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cli/KeyStoreAwareCommand.java
Outdated
Show resolved
Hide resolved
| PBEKeySpec keySpec = new PBEKeySpec(data.getChars(), salt, cost, PBKDF2_KEY_LENGTH); | ||
| result.put(Base64.getEncoder().encodeToString(secretKeyFactory.generateSecret(keySpec).getEncoded())); | ||
| return result.array(); | ||
| } catch (AssertionError ae){ |
There was a problem hiding this comment.
My thinking is that we should scope this tightly first and expand if/when we get reports/CI failures for additional errors. The ones we know are thrown are all AssertionError and catching all Error felt too wide of a net. Not holding a strong opinion on this, happy to discuss
There was a problem hiding this comment.
Somehow missed this comment in the previous pass ... I see you already considered it. I suggest a wider net (even wider then Error) because 1) any future fix to this would require a release which is purely overhead; 2) I cannot think of a valid reason for ES to shutdown here due to any kinda of error. So wider net (at least Error) should provide mostly benefit.
There was a problem hiding this comment.
Thanks for the comment Yang. I see your points. I'll let Lyudmila weigh in too and we can reach a consensus
There was a problem hiding this comment.
I would leave an ability to throw and shutdown for when something unthinkable happens. Catching Error should be ok though
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java
Outdated
Show resolved
Hide resolved
ywangd
left a comment
There was a problem hiding this comment.
I left a few comments. The main question I have is whether we should be more liberal and catch Throwable instead of AssertionError, which is a choice of BC. Since we are trying to cover unexpected situations to avoid unpleasant surprises, I wonder whether widen the net is a viable option.
server/src/main/java/org/elasticsearch/cli/KeyStoreAwareCommand.java
Outdated
Show resolved
Hide resolved
| SecretKey secretKey; | ||
| try { | ||
| secretKey = keyFactory.generateSecret(keySpec); | ||
| } catch (AssertionError ae) { |
There was a problem hiding this comment.
I wonder whether we could just be very liberal and catch a Throwable here. So we are sure nothing is going to trip it regardless of the provider.
There was a problem hiding this comment.
I went back and forth as to how wide the net should be. My counter argument is that when something throws an Error instead of an Exception it supposedly denotes an irrecoverable error. We know that this is not true for the AssertionError that BCFIPS throws here, but can we be certain that we want to catch all Throwables that can be thrown from external code. We don't do that in any other place. WDYT ?
There was a problem hiding this comment.
+1 to leave it this way, we shoudn't catch potentially irrecoverable exceptions
There was a problem hiding this comment.
You got me thinking again @ywangd .. On the one hand, I tried to limit the scope of these try-catch blocks to a min so it makes sense that we would catch all irrecoverable errors from that line and give out an error message. On the other hand, the very limited scope makes it less possible that a wider net would catch issues thrown by other security providers that we know nothing of (other than the PBKDF2 112 bits limitation). I'm a bit torn on this one
There was a problem hiding this comment.
Had a short chat with Yang and we decided that catching Error is the way to go based on the arguments that have been made already in this PR
server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java
Outdated
Show resolved
Hide resolved
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java
Outdated
Show resolved
Hide resolved
|
Should we add a test? |
| SecretKey secretKey; | ||
| try { | ||
| secretKey = keyFactory.generateSecret(keySpec); | ||
| } catch (AssertionError ae) { |
There was a problem hiding this comment.
+1 to leave it this way, we shoudn't catch potentially irrecoverable exceptions
server/src/main/java/org/elasticsearch/cli/KeyStoreAwareCommand.java
Outdated
Show resolved
Hide resolved
see description:
|
|
LGTM |
In certain situations, such as when configured in FIPS 140 mode, the Java security provider in use might throw a subclass of java.lang.Error. We currently do not catch these and as a result the JVM exits, shutting down elasticsearch. This commit attempts to address this by catching subclasses of Error that might be thrown for instance when a PBKDF2 implementation is used from a Security Provider in FIPS 140 mode, with the password input being less than 14 bytes (112 bits). - In our PBKDF2 family of hashers, we catch the Error and throw an ElasticsearchException while creating or verifying the hash. We throw on verification instead of simply returning false on purpose so that the message bubbles up and the cause becomes obvious (otherwise it would be indistinguishable from a wrong password). - In KeyStoreWrapper, we catch the Error in order to wrap and re-throw a GeneralSecurityException with a helpful message. This can happen when using any of the keystore CLI commands, when the node starts or when we attempt to reload secure settings. - In the `elasticsearch-users` tool, we catch the ElasticsearchException that the Hasher class re-throws and throw an appropriate UserException. Tests are missing because it's not trivial to set CI in fips approved mode right now, and thus any tests would need to be muted. There is a parallel effort in elastic#64024 to enable that and tests will be added in a followup.
In certain situations, such as when configured in FIPS 140 mode, the Java security provider in use might throw a subclass of java.lang.Error. We currently do not catch these and as a result the JVM exits, shutting down elasticsearch. This commit attempts to address this by catching subclasses of Error that might be thrown for instance when a PBKDF2 implementation is used from a Security Provider in FIPS 140 mode, with the password input being less than 14 bytes (112 bits). - In our PBKDF2 family of hashers, we catch the Error and throw an ElasticsearchException while creating or verifying the hash. We throw on verification instead of simply returning false on purpose so that the message bubbles up and the cause becomes obvious (otherwise it would be indistinguishable from a wrong password). - In KeyStoreWrapper, we catch the Error in order to wrap and re-throw a GeneralSecurityException with a helpful message. This can happen when using any of the keystore CLI commands, when the node starts or when we attempt to reload secure settings. - In the `elasticsearch-users` tool, we catch the ElasticsearchException that the Hasher class re-throws and throw an appropriate UserException. Tests are missing because it's not trivial to set CI in fips approved mode right now, and thus any tests would need to be muted. There is a parallel effort in elastic#64024 to enable that and tests will be added in a followup.
In certain situations, such as when configured in FIPS 140 mode, the Java security provider in use might throw a subclass of java.lang.Error. We currently do not catch these and as a result the JVM exits, shutting down elasticsearch. This commit attempts to address this by catching subclasses of Error that might be thrown for instance when a PBKDF2 implementation is used from a Security Provider in FIPS 140 mode, with the password input being less than 14 bytes (112 bits). - In our PBKDF2 family of hashers, we catch the Error and throw an ElasticsearchException while creating or verifying the hash. We throw on verification instead of simply returning false on purpose so that the message bubbles up and the cause becomes obvious (otherwise it would be indistinguishable from a wrong password). - In KeyStoreWrapper, we catch the Error in order to wrap and re-throw a GeneralSecurityException with a helpful message. This can happen when using any of the keystore CLI commands, when the node starts or when we attempt to reload secure settings. - In the `elasticsearch-users` tool, we catch the ElasticsearchException that the Hasher class re-throws and throw an appropriate UserException. Tests are missing because it's not trivial to set CI in fips approved mode right now, and thus any tests would need to be muted. There is a parallel effort in #64024 to enable that and tests will be added in a followup.
In certain situations, such as when configured in FIPS 140 mode, the Java security provider in use might throw a subclass of java.lang.Error. We currently do not catch these and as a result the JVM exits, shutting down elasticsearch. This commit attempts to address this by catching subclasses of Error that might be thrown for instance when a PBKDF2 implementation is used from a Security Provider in FIPS 140 mode, with the password input being less than 14 bytes (112 bits). - In our PBKDF2 family of hashers, we catch the Error and throw an ElasticsearchException while creating or verifying the hash. We throw on verification instead of simply returning false on purpose so that the message bubbles up and the cause becomes obvious (otherwise it would be indistinguishable from a wrong password). - In KeyStoreWrapper, we catch the Error in order to wrap and re-throw a GeneralSecurityException with a helpful message. This can happen when using any of the keystore CLI commands, when the node starts or when we attempt to reload secure settings. - In the `elasticsearch-users` tool, we catch the ElasticsearchException that the Hasher class re-throws and throw an appropriate UserException. Tests are missing because it's not trivial to set CI in fips approved mode right now, and thus any tests would need to be muted. There is a parallel effort in #64024 to enable that and tests will be added in a followup.
We handled the exceptions thrown by Security Providers in the case of short encryption keys in elastic#65464 and this commit adds a couple of tests to validate that the appropriate exceptions are thrown when encryption keys derived from short passwords are in use, in FIPS 140-2 mode.
We handled the exceptions thrown by Security Providers in the case of short encryption keys in #65464 and this commit adds a couple of tests to validate that the appropriate exceptions are thrown when encryption keys derived from short passwords are in use, in FIPS 140-2 mode.
We handled the exceptions thrown by Security Providers in the case of short encryption keys in elastic#65464 and this commit adds a couple of tests to validate that the appropriate exceptions are thrown when encryption keys derived from short passwords are in use, in FIPS 140-2 mode.
We handled the exceptions thrown by Security Providers in the case of short encryption keys in elastic#65464 and this commit adds a couple of tests to validate that the appropriate exceptions are thrown when encryption keys derived from short passwords are in use, in FIPS 140-2 mode.
We handled the exceptions thrown by Security Providers in the case of short encryption keys in #65464 and this commit adds a couple of tests to validate that the appropriate exceptions are thrown when encryption keys derived from short passwords are in use, in FIPS 140-2 mode.
We handled the exceptions thrown by Security Providers in the case of short encryption keys in #65464 and this commit adds a couple of tests to validate that the appropriate exceptions are thrown when encryption keys derived from short passwords are in use, in FIPS 140-2 mode.
In certain situations, such as when configured in FIPS 140 mode,
the Java security provider in use might throw a subclass of
java.lang.Error. We currently do not catch these and as a result
the JVM exits, shutting down elasticsearch.
This commit attempts to address this by catching AssertionErrors
that we are aware that might be thrown when a PBKDF2 implementation
is used from a Security Provider in FIPS 140 mode, with the password
input being less than 14 bytes (112 bits).
throw an ElasticsearchException while creating or verifying the
hash. We throw on verification instead of simply returning false
on purpose so that the message bubbles up and the cause becomes
obvious (otherwise it would be indistinguishable from a wrong
password).
wrap and rethrow with a helpful message. This can happen when using
any of the keystore CLI commands, when the node starts or when we attempt
to reload secure settings.
elasticsearch-userstool, we catch the AssertionError andthrow an appropriate UserException.
Tests are missing because it's not trivial to set CI in fips approved mode
right now, and thus any tests would need to be muted. There is a parallel
effort in #64024 to enable that
and we can either hold off merging this until 64024 is merged and tests
added, or add tests in followup.