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

Replace md5 with FIPS compliant algorithm #14539

Open
maxgio92 opened this issue Nov 5, 2024 · 7 comments · May be fixed by #14921
Open

Replace md5 with FIPS compliant algorithm #14539

maxgio92 opened this issue Nov 5, 2024 · 7 comments · May be fixed by #14921

Comments

@maxgio92
Copy link

maxgio92 commented Nov 5, 2024

Hello,
it would be nice to replace this call to md5 with a call to a FIPS 140 allowed algorithm like sha256 to calculate the etag for the fleet agent configuration.

What do you think?

Thank you

@inge4pres
Copy link
Contributor

hello @maxgio92 👋🏼

i don't know how the eTag is used, but it doesn't seem like sensitive information that needs strong hashing.

Why do you think we should update it?
What benefit would we gain compared to the current usage of md5?

@maxgio92
Copy link
Author

maxgio92 commented Nov 5, 2024

Hi @inge4pres, I see and understand the point.
I think anyway that the advantage is to be able to meet compliance with FIPS-140 that doesn't allow usage of md5 in general, at a pretty low cost.
Thank you

@xnox
Copy link

xnox commented Nov 5, 2024

@inge4pres it is not security sensitive, but many FIPS go toolchains block crypto/md5 usage such that it fails at runtime. This triggered go coverage to switch away from md5 to a non-security hash FNV-1a instead (which is available in go standard library, and faster than md5) see https://go-review.googlesource.com/c/go/+/617360

Another option, which webpack chose is to switch to xxhash (which is even faster than above two) for eTag like purposes.

md5 often raises suspicion, because whilst today it almost almost used as a non-security relevant hash; it used to be classed as a secure one, and today it is a very slow choice for non-security one.

Does this make sense?

@axw
Copy link
Member

axw commented Nov 5, 2024

+1 on switching away from MD5, but we'll need to be mindful of whether this is a breaking change. Similar issue: elastic/apm-data#146

It's might be possible that we can remove the linked code altogether, as we have a new mechanism for agent configuration these days. I think we kept that around for migration, but that's nearly 2 years old now.

@xnox
Copy link

xnox commented Nov 5, 2024

+1 on switching away from MD5, but we'll need to be mindful of whether this is a breaking change. Similar issue: elastic/apm-data#146

It's might be possible that we can remove the linked code altogether, as we have a new mechanism for agent configuration these days. I think we kept that around for migration, but that's nearly 2 years old now.

would you be open compiling it out with a go build tag? that is sensitive to a custom one or any of the fips-toolchain ones? //go:build requirefips || goexperiment.systemcrypto || goexperiment.boringcrypto will ensure that a custom buildtag (popular in other projects) or a fips-oriented toolchain result in an MD5-less behaviour. And people who use such build tags or toolchains, likely to compile full set of clients and server, and deploy only a consistent set.

@kruskall
Copy link
Member

kruskall commented Nov 5, 2024

that setup call is only used for the fallback method, it's not called if agentcfg is configured to use ES and should probably be removed in 9.0.0

IMO there's no need for adding a build tag

@xnox
Copy link

xnox commented Nov 5, 2024

@maxgio92 it also means we can likely use go-msft toolchain as is; and let md5 fail at runtime.

@kruskall kruskall linked a pull request Dec 12, 2024 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants