elastic / apm-server

https://www.elastic.co/guide/en/apm/guide/current/index.html
Other
1.22k stars 523 forks source link

Replace md5 with FIPS compliant algorithm #14539

Open maxgio92 opened 2 days ago

maxgio92 commented 2 days ago

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 commented 2 days ago

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 commented 2 days ago

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 commented 2 days ago

@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 commented 2 days ago

+1 on switching away from MD5, but we'll need to be mindful of whether this is a breaking change. Similar issue: https://github.com/elastic/apm-data/issues/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 commented 2 days ago

+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 commented 2 days ago

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 commented 2 days ago

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