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

Log warning when cache hasher not compliant with FIPS #86740

Merged
merged 39 commits into from
Jun 1, 2022

Conversation

n1v0lg
Copy link
Contributor

@n1v0lg n1v0lg commented May 12, 2022

Currently, it's possible to choose a hash function for various cache
hashers (e.g., in ApiKeyService) that is not compliant with FIPS 140
(e.g., MD5). This PR logs a warning on node start if a non-compliant
hashing algorithm is used in FIPS mode.

Note that there are other usages of non-FIPS compliant hash functions,
which are not configured through settings (e.g.
FingerprintProcessor). I plan to address these in a separate PR.

Relates #68743

@n1v0lg n1v0lg added >enhancement :Security/FIPS Running ES in FIPS 140-2 mode Team:Security Meta label for security team labels May 12, 2022
@elasticsearchmachine
Copy link
Collaborator

Hi @n1v0lg, I've created a changelog YAML for you.

@n1v0lg n1v0lg marked this pull request as ready for review May 17, 2022 10:38
@elasticmachine
Copy link
Collaborator

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

@n1v0lg n1v0lg requested a review from ywangd May 17, 2022 10:40
@n1v0lg n1v0lg added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 19, 2022
@n1v0lg n1v0lg removed the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 19, 2022
@n1v0lg
Copy link
Contributor Author

n1v0lg commented May 19, 2022

Ah crap, just found one more wrinkle in this:

https://www.elastic.co/guide/en/elasticsearch/reference/current/fips-140-compliance.html#_password_hashing

Our docs mention that cache.hash_algo should be set to one of the pbkdf_stretch_* values in FIPS mode, i.e., not ssha256. That'll be mighty slow for a cache... At the same time, the default value we use regardless of whether FIPS is enabled or not is ssha256. Changing the default would be a breaking change, especially given the performance implications (default pbkdf_stretch runs 10k iterations).

I think it makes the most sense to change the warning check in this PR to also trigger on ssha256 so we are consistent with our docs but not touch the default setting. Then we can potentially revisit making a breaking change around the setting later. Alternatively, if we deem that ssha256 is acceptable for in-memory hashing we could update the docs. I'm not familiar enough with FIPS to make that call but will google around. @ywangd wdyt? Will raise in the next sec. team meeting.

@ywangd
Copy link
Member

ywangd commented May 20, 2022

I think we should amend the doc. The wording is likely tricky to justify ssha256 is sufficient for in-memory hashing in FIPS mode. But I find it hard to recommend pbkdf2 as cache.hash_algo. Though technically correct, this recommendation is set users to a likely path of failure. Especially with API keys, the secret is already of 20 byte length. Default pbkdf2 takes about 70 ms to compute. This means a single core can only authenticate 15 requests per second. It will not work for a deployment with something like Fleet.

Taking a step back, my original intention was to log a warning message of "xxx is not recommended" which intentionaly avoid having to mention ssha256 explicitly (gray area as I see it). But negative message is not very actionable (because users do not know what would be recommended). So I accepted that a positive message is better, but I wasn't aware our docs recommend pbkdf2 for cache hasing. Now I think we need retrace our decisions. My current preference would be (1) not recommend pbkdf2 for performance reasons or recommend it but also explain the performance implication, and (2) not warn againet ssha256. But let's bring it to the team for broader discussion. Thanks!

@n1v0lg
Copy link
Contributor Author

n1v0lg commented May 20, 2022

This means a single core can only authenticate 15 requests per second.

This is indeed not realistic.

But negative message is not very actionable

The steps that users would likely take is either ignore the warning altogether, read our/NIST's docs, try out options blindly until they hit the right one, or reach out to support. We'd essentially still need to make a recommendation. It also gets complicated with, for example, the NOOP hasher.

I'm with you on your preferences regarding docs and allowing ssha256.

As for interpreting NIST recommendations, I found two applicable resources so far:

  1. 800-63B Digital Identity Guidelines
  2. 800-132 Recommendation for Password-Based Key
    Derivation

(1) Mentions:

Verifiers SHALL store memorized secrets in a form that is resistant to offline attacks. Memorized secrets SHALL be salted and hashed using a suitable one-way key derivation function. ... Examples of suitable key derivation functions include Password-based Key Derivation Function 2 (PBKDF2) [SP 800-132] and Balloon [BALLOON].

(2) essentially specifies PBKDFs and recommends their use for "the
protection of electronically-stored data or for the protection of data protection keys".

Both resources deal with stored data.

If in-memory data does not fall under that category, then as far as NIST recommendations (and FIPS 140-2) go, perhaps it's more about the hash function itself. If it's approved as a secure hash function (e.g. it's not md5) then there is no violation. This doesn't mean we should recommend using an unsalted (or broken!) hash function for cache hashing, we can still recommend ssha256, but at least we now aren't stuck with only pbkdf2. Anyway, I just wanted to write this up somewhere, I'm not sure this is the route we should go and we'll discuss more in sec team meeting.

One final note: FingerprintProcessor does not support pbkdf2 at all (which is very sensible from a performance perspective).

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.

I am tagging this as "request changes" until the team agrees on what message we want to show to the end-users.

@tvernum
Copy link
Contributor

tvernum commented May 23, 2022

My advice would be to make 1 recommendation
For example:

The [{}] password hasher is not recommended when running in FIPS mode. The recommended hasher for [foo.cache.hash_algo] is SSHA256. 

We could also say Other FIPS compatible options are ..., but then the question is which ones we want to list.

@n1v0lg
Copy link
Contributor Author

n1v0lg commented May 23, 2022

@tvernum I like your suggestion.

Wondering if Another FIPS compatible option is PBKDF2, however it may significantly bottle-neck cache performance. is a concise but clear enough way to explain how PBKDF fits it.

@ywangd
Copy link
Member

ywangd commented May 26, 2022

My suggestions are:

  1. Only recommend ssha256 in the warning message (this PR)
  2. Amend the docs to clarify the choice of cache hashing algorithm and add details about why ssha256 is recommended and why pbkdf2 is not recommended even though it is "more" compatible and why it is not really necessary for in-memory credentials.
  3. Update the warning message (separate PR) to add an extra sentence like please refer to the (link-to-the-doc) for more information about choosing the cache hashing algorithm.

@tvernum
Copy link
Contributor

tvernum commented May 26, 2022

@ywangd's suggestion looks good to me. Step 3 would be contingent on us (ES dev) reaching a conclusion on how we want to embed URLs in messages, but we can defer the work until we have an answer there.

@n1v0lg
Copy link
Contributor Author

n1v0lg commented May 31, 2022

Sounds good to me! @ywangd thank you for the suggestion.

@ywangd
Copy link
Member

ywangd commented Jun 1, 2022

Step 3 would be contingent on us (ES dev) reaching a conclusion on how we want to embed URLs in messages, but we can defer the work until we have an answer there.

I have seen a few places where we already embed short URLs in responses or logs. One recent example is:

public static final String HELP_URL = "https://ela.st/fix-slm";

Not sure whether this is considered "official" though.

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

Thanks for being thorough!

@n1v0lg n1v0lg added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) and removed auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Jun 1, 2022
@n1v0lg n1v0lg merged commit 35b4872 into elastic:master Jun 1, 2022
@n1v0lg n1v0lg deleted the enhance/warn-on-md5-in-fips-mode branch June 1, 2022 08:45
n1v0lg added a commit that referenced this pull request Jul 22, 2022
Our docs currently recommend PBKDF2 as a cache hasher in FIPS mode.
However, the performance overhead of PBKDF2 is prohibitive; ssha256
is a more appropriate choice for in-memory credential hashing. This PR
updates the docs to reflect this. See #86740 for more context.
n1v0lg added a commit to n1v0lg/elasticsearch that referenced this pull request Jul 22, 2022
Our docs currently recommend PBKDF2 as a cache hasher in FIPS mode.
However, the performance overhead of PBKDF2 is prohibitive; ssha256
is a more appropriate choice for in-memory credential hashing. This PR
updates the docs to reflect this. See elastic#86740 for more context.
n1v0lg added a commit to n1v0lg/elasticsearch that referenced this pull request Jul 22, 2022
Our docs currently recommend PBKDF2 as a cache hasher in FIPS mode.
However, the performance overhead of PBKDF2 is prohibitive; ssha256
is a more appropriate choice for in-memory credential hashing. This PR
updates the docs to reflect this. See elastic#86740 for more context.
n1v0lg added a commit that referenced this pull request Aug 1, 2022
Our docs currently recommend PBKDF2 as a cache hasher in FIPS mode.
However, the performance overhead of PBKDF2 is prohibitive; ssha256
is a more appropriate choice for in-memory credential hashing. This PR
updates the docs to reflect this. See #86740 for more context.
n1v0lg added a commit that referenced this pull request Aug 4, 2022
Our docs currently recommend PBKDF2 as a cache hasher in FIPS mode.
However, the performance overhead of PBKDF2 is prohibitive; ssha256
is a more appropriate choice for in-memory credential hashing. This PR
updates the docs to reflect this. See #86740 for more context.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/FIPS Running ES in FIPS 140-2 mode Team:Security Meta label for security team v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants