Skip to content

x-pack/libbeat/common/cloudfoundry: Use xxhash instead of SHA1 #43964

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

x-pack/libbeat/common/cloudfoundry: Use xxhash instead of SHA1 #43964

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2742c45
x-pack/libbeat/common/cloudfoundry: Use SHA224 instead of SHA1 in FIP…
shmsr Apr 16, 2025
d74253a
Merge branch 'main' into cloudfoundry-cache-utils-fips
shmsr Apr 17, 2025
3ead73f
x-pack/libbeat/common/cloudfoundry: Use xxhash
shmsr Apr 17, 2025
15724b5
Merge branch 'main' into cloudfoundry-cache-utils-fips
shmsr Apr 29, 2025
afd06f7
Address review comments
shmsr May 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions x-pack/libbeat/common/cloudfoundry/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
package cloudfoundry

import (
"crypto/sha1"
"encoding/base64"
"fmt"
"time"

Expand Down Expand Up @@ -139,9 +137,3 @@ func (c *clientCacheWrap) Close() error {
}
return nil
}

// sanitizeCacheName returns a unique string that can be used safely as part of a file name
func sanitizeCacheName(name string) string {
hash := sha1.Sum([]byte(name))
return base64.RawURLEncoding.EncodeToString(hash[:])
}
18 changes: 18 additions & 0 deletions x-pack/libbeat/common/cloudfoundry/cache_utils_fips.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

//go:build requirefips

package cloudfoundry

import (
"crypto/sha256"
Copy link
Member

Choose a reason for hiding this comment

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

question: do we need a cryptographic function ? If not, I'd suggest we use a non-cryptographic func (e.g. xxhash) to make this more future-proof.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes makes sense. I'll do this.

"encoding/base64"
)

// sanitizeCacheName returns a unique string that can be used safely as part of a file name
func sanitizeCacheName(name string) string {
hash := sha256.Sum224([]byte(name))
return base64.RawURLEncoding.EncodeToString(hash[:])
}
18 changes: 18 additions & 0 deletions x-pack/libbeat/common/cloudfoundry/cache_utils_nofips.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

//go:build !requirefips
Copy link
Member

Choose a reason for hiding this comment

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

question: Can we update the cache key for everyone ? Would this be considered a breaking change ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am also not sure about this. Since sha1.Sum is deterministic, it will always produce the same hash for a fixed input (in this case, the APIAddress). This deterministic behavior means the system might create or reuse a (persistent) cache based on this hash value.

For users who already have that persistent cache in place, changing this logic could potentially be a breaking change. Although it's just a cache implementation detail, I haven't deeply investigated this issue since I haven't worked with cloudfoundry before.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to better understand the implications of changing the cache key so we could assess whether it makes sense to use a single implementation to FIPS and non-FIPS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both hash algorithms are deterministic (they have to be, otherwise we couldn't use them to get a persistent filename). If we wanted to start using a new hash in non-FIPS mode, it would ignore any previously cached data and start a fresh cache on upgrade (which seems fine as a one-time cost since it doesn't actually break any functionality, so I'd be in favor of just switching over wholesale instead of special-casing FIPS).

However, xxhash isn't cryptographically secure -- sha1 is no longer allowed for FIPS, but what's the motivation for switching to an insecure hash instead of sha2 or sha3? I assumed the motivation for the hash was to avoid leaking information about the API address through the filename.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. I also did check, using just one algorithm for both modes (FIPS and non-FIPS) is fine.

@faec I opened the PR by replacing sha1 with SHA-224 (Sum224) (of sha2 fam); here: 2742c45 but later replaced with xxhash as it's just being used for sanitizing a filename for creating a persistent cache (non-crypto usage).

Ref: #43964 (comment)

And why it is ok?

There may be instances where a non-approved cryptographic algorithm can be used for non-cryptographic purposes. For example, SHA1 is not a FIPS 140-3 algorithm, but because Git uses it for non-cryptographic purposes, we can use it. In these cases, we must document why it’s not being used for cryptographic purposes, or disable the feature outright.

Non-approved security functions shall not be used in the approved mode of operation; however, non-approved
cryptographic algorithms may be used in the approved mode of operation if the non-approved algorithms are
not a security function. If a non-approved cryptographic algorithm is used by the module in the approved mode
but is not a security function, the algorithm shall be included as a separate list of non-approved but allowed
algorithms with no security claimed in the Security Policy. However, the module’s certificate shall not
include these algorithms as they do not claim any security and are not used to meet any requirement of FIPS
140-2.

Read the first line ^

So, as it's a non crypto usage, xxhash is better choice compare to the crypto hashes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make the change to just keep one file and just change the algorithm.


package cloudfoundry

import (
"crypto/sha1"
"encoding/base64"
)

// sanitizeCacheName returns a unique string that can be used safely as part of a file name
func sanitizeCacheName(name string) string {
hash := sha1.Sum([]byte(name))
return base64.RawURLEncoding.EncodeToString(hash[:])
}