Always use OpenSSL backend when goexperiment=opensslcrypto

[qmuntal]

We are currently only using the OpenSSL backend if goexperiment=opensslcrypto and FIPS mode is requested, either via GOFIPS=1 or system-wide setting. In all other situations we fallback to Go crypto.

We inherited this approach from RedHat fork and I initially though it would be good to keep it for the following reasons:

• We don't have to support using OpenSSL in non-FIPS mode.
• An application would still work in case it could not load the OpenSSL libraries, either because the system doesn't have them or because it is using an unsupported version, unless GOFIPS=1.

On the other hand, there are some downsides:

• boring.Enabled is no longer constant, because the OpenSSL backend can be disabled at runtime in some situations. This hurts the Go reachability, inlining and escape analyses, because it can't eliminate non-OpenSSL code paths even when goexperiment=opensslcrypto.
• We diverge from the Boring approach, where goexperiment=boringcrypto always uses the boring backend.

The OpenSSL backend is now more robust, better tested and has a smarter OpenSSL library loading strategy, so I'm leaning towards always using the OpenSSL backend so that boring.Enabled can be made constant and we diverge less from the Boring approach.

If we do this change, applications running with GOFIPS=1 would still panic if OpenSSL can't be load or FIPS can't be configured.

@dagood @jaredpar @chsienki thoughts?

[qmuntal]

I also plan to make boring.Enabled constant when using the CNG backend. All supported Windows version ship with bcrypt.dll, so there are zero chances someone can't run an application built with goexperiment=cngcrypto.

[jaredpar]

Part of the reason for the initial setup was to enable customers to build a binary once then let them decide on FIPS / no-FIPS at runtime. This would change that approach (if I'm reading correctly).

We diverge from the Boring approach, where goexperiment=boringcrypto always uses the boring backend.

That is a good motivator though. If Google is going to push FIPS in this direction it's going to be strange if our implementation is going the other direction.

Before taking this approach though I think we should discuss with RedHat what their plans are. If they're interested in sticking to their current approach then us going the other direction will make it hard to combine our implementation in the golang-fips org (at least I think it will).

[qmuntal]

Part of the reason for the initial setup was to enable customers to build a binary once then let them decide on FIPS / no-FIPS at runtime. This would change that approach (if I'm reading correctly).

This was the main motivation, but it comes from a misunderstanding. Since the beginning we though FIPS==OpenSSL, so if a user doesn't want FIPS, neither wants OpenSSL. What I'm trying to do here is break that correlation. If this proposal is approved, OpenSSL (aka goexperiment=opensslcrypto) will be a precondition to having FIPS (no change here), but it will still be used even if the user does not want FIPS (aka FIPS=0), and users will still be able to decide on FIPS / no-FIPS at runtime.

The main change of this proposal is that users building with goexperiment=opensslcrypto won't be able to run their applications if the runtime environment does not have OpenSSL installed.

Before taking this approach though I think we should discuss with RedHat what their plans are. If they're interested in sticking to their current approach then us going the other direction will make it hard to combine our implementation in the golang-fips org (at least I think it will).

Agree, we should discuss this with them, although the code that decides when to use OpenSSL and enable FIPS lives in our Go fork, not in go-crypto-openssl, so it is potentially less reusable.

[dagood]

I also plan to make boring.Enabled constant when using the CNG backend. All supported Windows version ship with bcrypt.dll, so there are zero chances someone can't run an application built with goexperiment=cngcrypto.

👍 No concerns here.

The main change of this proposal is that users building with goexperiment=opensslcrypto won't be able to run their applications if the runtime environment does not have OpenSSL installed.

So, the obvious question is: do we have any users who must distribute a single build of their app that can run on a machine without OpenSSL and run on a machine with OpenSSL installed with FIPS compliance? These scenarios are the closest I can think of:

• An OSS project (like OSM) that ships binaries. Are there people out there who need to use the project without OpenSSL? Maybe they're on a distro that uses an alternative library to OpenSSL? If the project needs to support these users with its prebuilt distribution and also support FIPS users, this forces the repo to ship a separate "openssl build" for those who target FIPS and a "go crypto build" for those who target non-OpenSSL. It's quite possible that the same group of users who don't use OpenSSL are also fine compiling the project from source, so there would be no concern. But if that's not the case, the overhead and user unfriendliness in splitting linux-amd64 up into two builds might block adoption at some point for some project that would otherwise want to use our build of Go.
• My understanding is that CBL-Mariner always includes OpenSSL because it's depended on by pretty basic packages, so no problem there.
  • In theory if someone wanted not to include OpenSSL (minimal distroless?) but still use Go tools built by the distro, this means they can't do that, because the distro built Go apps with goexperiment=opensslcrypto to give them the option of FIPS-compliance. Excising OpenSSL from Mariner is a corner case that I doubt anyone we support truly needs, and I don't know if Mariner would even support it.

With what I currently know about our users, I don't think anyone hits this edge case, so I don't think we should block the change for this reason. (Especially if the perf gain is significant for a vast majority of our users. 😄) But I think it's worth exploring the possible consequences, because there is a small amount of lost flexibility here.

• We diverge from the Boring approach, where goexperiment=boringcrypto always uses the boring backend.

The Boring approach statically links BoringSSL/BoringCrypto inside the resulting app, correct? (The reason I mention it: this means they don't have any of those dependency issues.)

[jaredpar]

How feasible is it to make this change only apply to the 1.19 and forward releases? Essentially don't change expectations for people on the 1.18 and 1.17 release trains but reset the expectations in 1.19?

[qmuntal]

How feasible is it to make this change only apply to the 1.19 and forward releases? Essentially don't change expectations for people on the 1.18 and 1.17 release trains but reset the expectations in 1.19?

Didn't mention it in the initial description, but I'm only targeting 1.19 as we are already disrupting how the OpenSSL backend is enabled (goexperiment instead of toolchain fork). So yes, it is feasible!

[qmuntal]

So, the obvious question is: do we have any users who must distribute a single build of their app that can run on a machine without OpenSSL and run on a machine with OpenSSL installed with FIPS compliance? These scenarios are the closest I can think of:

If we had to support this use case, which we are now but we won't if this proposal is accepted, both groups would be perf-penalized by not using dedicated binaries. Here are some benchmarks from crypto/sha256:

Apps running on an OpenSSL-free runtime using an OpenSSL-enabled binary

If boring.Enabled is not constant, apps are slower and have more memory footprint: (Baseline is upstream Go)

name          old time/op    new time/op    delta
Hash8Bytes-4     236ns ± 3%     287ns ± 3%  +21.72%  (p=0.000 n=10+9)
Hash1K-4        2.97µs ± 3%    3.08µs ± 1%   +3.67%  (p=0.000 n=8+9)
Hash8K-4        24.9µs ±25%    22.3µs ± 5%     ~     (p=0.356 n=10+9)

name          old speed      new speed      delta
Hash8Bytes-4  33.9MB/s ± 3%  27.9MB/s ± 3%  -17.86%  (p=0.000 n=10+9)
Hash1K-4       345MB/s ± 3%   332MB/s ± 1%   -3.56%  (p=0.000 n=8+9)
Hash8K-4       335MB/s ±22%   367MB/s ± 5%     ~     (p=0.356 n=10+9)

name          old memory/op   new memory/op   delta
Hash8Bytes-4     0.00B        128.00B ± 0%    +Inf%  (p=0.000 n=10+10)
Hash1K-4         0.00B        128.00B ± 0%    +Inf%  (p=0.000 n=10+10)
Hash8K-4         0.00B        128.00B ± 0%    +Inf%  (p=0.000 n=10+10)

name          old allocs/op  new allocs/op  delta
Hash8Bytes-4      0.00           1.00 ± 0%    +Inf%  (p=0.000 n=10+10)
Hash1K-4          0.00           1.00 ± 0%    +Inf%  (p=0.000 n=10+10)
Hash8K-4          0.00           1.00 ± 0%    +Inf%  (p=0.000 n=10+10)

Apps running on runtime with OpenSSL using an OpenSSL-enabled binary

If boring.Enabled is not constant, apps have more memory footprint than if it was constant:

name          old time/op    new time/op    delta
Hash8Bytes-4    2.41µs ±17%    2.25µs ±13%      ~     (p=0.075 n=10+10)
Hash1K-4        5.22µs ± 5%    4.94µs ± 6%    -5.40%  (p=0.004 n=9+9)
Hash8K-4        22.3µs ± 3%    26.0µs ±26%      ~     (p=0.315 n=10+10)

name          old speed      new speed      delta
Hash8Bytes-4  3.35MB/s ±20%  3.57MB/s ±15%      ~     (p=0.072 n=10+10)
Hash1K-4       196MB/s ± 4%   208MB/s ± 6%    +5.79%  (p=0.004 n=9+9)
Hash8K-4       368MB/s ± 2%   324MB/s ±23%      ~     (p=0.315 n=10+10)

name          old memory/op   new memory/op   delta
Hash8Bytes-4     48.0B ± 0%     96.0B ± 0%  +100.00%  (p=0.000 n=10+10)
Hash1K-4         48.0B ± 0%     96.0B ± 0%  +100.00%  (p=0.000 n=10+10)
Hash8K-4         48.0B ± 0%     96.0B ± 0%  +100.00%  (p=0.000 n=10+10)

name          old allocs/op  new allocs/op  delta
Hash8Bytes-4      1.00 ± 0%      2.00 ± 0%  +100.00%  (p=0.000 n=10+10)
Hash1K-4          1.00 ± 0%      2.00 ± 0%  +100.00%  (p=0.000 n=10+10)
Hash8K-4          1.00 ± 0%      2.00 ± 0%  +100.00%  (p=0.000 n=10+10)

The Boring approach statically links BoringSSL/BoringCrypto inside the resulting app, correct? (The reason I mention it: this means they don't have any of those dependency issues.)

Yep.

[qmuntal]

@derekparker could you share your opinion on this topic?

[derekparker]

@qmuntal thanks for the ping on this!

So, from our perspective we're very unlikely to ship any builds with openssl statically linked into the resulting binary. This is mostly due to our security policies and our reliance on dynamic linkage within the operating system to update core components such as openssl when CVEs drop.

That being said, I could potentially see a world where we follow the same path as you describe regarding goexperiment=openssl always calling into openssl regardless of whether we enforce FIPS compliance or not. We use dlopen in order to dynamically link against openssl, so the only runtime dependencies that would cause the dynamic loader to fail would be libdl, glibc and friends.

I understand that making boring.Enabled a constant would have performance improvements because we eliminate the function call overhead (although can the Go compiler not inline that anyways, thus eliminating the function call overhead anyways?). Either way, we're trading the cost of calling a function for boring.Enabled for the cost of crossing the CGO boundary. From my perspective there's overhead either way, and the CGO overhead is much more significant than a Go function calling another Go function.

[dagood]

So, from our perspective we're very unlikely to ship any builds with openssl statically linked into the resulting binary.

To be clear, we're in the same boat, for some similar reasons (letting CBL-Mariner patch their FIPS-compliant OpenSSL libraries at their own pace, decoupled from Go), and some different reasons (shipping one Go toolset build that works on multiple distros/versions each with their own FIPS-compliant OpenSSL).

The reason I mentioned BoringCrypto being statically linked was to point out why we might not always want to follow upstream's way of doing things. Static vs. dynamic linking is the root cause of us needing to think carefully about requiring OpenSSL always be present "ambiently", which isn't relevant with the Boring implementation.

[qmuntal]

I understand that making boring.Enabled a constant would have performance improvements because we eliminate the function call overhead (although can the Go compiler not inline that anyways, thus eliminating the function call overhead anyways?).

I just recalled that in your fork boring.Enabled is a function call. In our case is defined as var Enabled bool. This might have caused confused you when reading this issue, sorry for that. Whether is a function or a variable, what I'm thinking is to change it to const Enable bool, not because the overhead of accessing a variable or calling boring.Enabled(), that most probably will be inlined, but because when it is used inside other functions it adds overhead to the escape and inlining analysis.

Let's see an example from sha/256 (see https://github.com/microsoft/go/issues/641#issuecomment-1186872535 for the benchmarks):

func New() hash.Hash {
    if boring.Enabled {
        return boring.NewSHA256()
    }
    d := new(digest)
    d.Reset()
    return d
}

• If boring.Enabled is not constant, the inline cost is 131, so New() can't be inlined and the returned hash is wrapped in an interface and allocates.
• If boring.Enabled is a constant set to true, the compiler can eliminate the code outside the if statement, reducing the inline cost to 60, so New() can be inlined and the compiler can prove that it always returns a boring.SHA256 type, which avoids the extra allocation of wrapping the hash.

[derekparker]

So, from our perspective we're very unlikely to ship any builds with openssl statically linked into the resulting binary.

To be clear, we're in the same boat, for some similar reasons (letting CBL-Mariner patch their FIPS-compliant OpenSSL libraries at their own pace, decoupled from Go), and some different reasons (shipping one Go toolset build that works on multiple distros/versions each with their own FIPS-compliant OpenSSL).

The reason I mentioned BoringCrypto being statically linked was to point out why we might not always want to follow upstream's way of doing things. Static vs. dynamic linking is the root cause of us needing to think carefully about requiring OpenSSL always be present "ambiently", which isn't relevant with the Boring implementation.

Ack, that makes total sense sorry for the misunderstanding!

I understand that making boring.Enabled a constant would have performance improvements because we eliminate the function call overhead (although can the Go compiler not inline that anyways, thus eliminating the function call overhead anyways?).

I just recalled that in your fork boring.Enabled is a function call. In our case is defined as var Enabled bool. This might have caused confused you when reading this issue, sorry for that. Whether is a function or a variable, what I'm thinking is to change it to const Enable bool, not because the overhead of accessing a variable or calling boring.Enabled(), that most probably will be inlined, but because when it is used inside other functions it adds overhead to the escape and inlining analysis.

Let's see an example from sha/256 (see #641 (comment) for the benchmarks):

func New() hash.Hash {
  if boring.Enabled {
      return boring.NewSHA256()
  }
  d := new(digest)
  d.Reset()
  return d
}

• If boring.Enabled is not constant, the inline cost is 131, so New() can't be inlined and the returned hash is wrapped in an interface and allocates.
• If boring.Enabled is a constant set to true, the compiler can eliminate the code outside the if statement, reducing the inline cost to 60, so New() can be inlined and the compiler can prove that it always returns a boring.SHA256 type, which avoids the extra allocation of wrapping the hash.

Same as above, sorry for the confusion / misunderstanding! I understand what you're saying now, and that makes complete sense.

I suppose, given the clarification on the above, as I understand it the main difference is in whether or not to always call into OpenSSL when compiled with goexperiment=openssl as opposed to determining that at runtime, correct? If that's the case I think I could be convinced on that. That would be a bit of a change for RHEL users, as historically we have preferred an "opt-out" approach instead of an "opt-in" approach (e.g. as secure as possible by default). This means our customers don't have to do anything after building their Go programs to get FIPS compliance other than flipping the switch in the OS. They don't need to build their Go programs any special way. I actually historically advocated for the opposite approach, since customers requiring FIPS is in the minority, in my opinion.

Let me discuss this with my team internally and get back to you all (if you don't mind)!

[qmuntal]

Let me discuss this with my team internally and get back to you all (if you don't mind)!

Sure! And thanks for this detailed answer 👍

historically we have preferred an "opt-out" approach instead of an "opt-in" approach (e.g. as secure as possible by default)

As a general rule, I wouldn't say FIPS mode is neither more nor less secure than standard Go crypto, so it is mainly useful to meet FIPS 140-2 compliance, which is a valid goal in its own. With this in mind, I'm confortable using the "opt-in" approach.

In fact, for go1.19 we already changed the compiler so the openssl backend is an opt-in setting, i.e. goexperiment=openssl, and we are not releasing two separate flavors anymore.

[derekparker]

Hey all!

We've discussed this internally and feel similarly in that we want to continue aligning with upstream and simplify this backend. We've decided to take the same approach outlined here, making calling into the OpenSSL code a compile time decision rather than a runtime decision. As already outlined in this thread this is regardless of whether FIPS is actually activated on the host or not. This will take some refactoring on our end because our codebase assumes that if you're calling into OpenSSL it's only because you want to be FIPS compliant, meaning the code is pretty strict. However, this means we continue to be aligned in our development goals moving forward, which is very exciting!