go icon indicating copy to clipboard operation
go copied to clipboard

proposal: secret,crypto/subtle: make all "bubbles" inherited across goroutines

Open aclements opened this issue 1 month ago • 11 comments

Proposal Details

In recent Go releases, we've been growing more types of "goroutine bubbles". As of Go 1.25, we have profile labels, testing/synctest, and crypto/subtle.WithDataIndependentTiming. For Go 1.26, we've accepted secret.Do and are likely to accept fips140.WithoutEnforcement.

However, we've been adding these in a somewhat ad hoc manner. As a result, these are inconsistent in how they're "inherited" if a goroutine within a bubble starts a new goroutine. Currently, synctest and profile label bubbles are inherited. Profile labels, in addition to being inherited, can be passed around via contexts so they can be used temporarily by helper goroutines. However, secret.Do and crypto/subtle.WithDataIndependentTiming aren't inherited at all.

We propose rationalizing these by making secret.Do and crypto/subtle.WithDataIndependentTiming inherited, like the other bubbles.

Rationale

There is no perfect answer here. The main argument for inheritance is that it is more decoupled than the other options: whether or not an API uses goroutines internally must be an implementation detail, and not be part of its API surface. Any form of bubble necessarily chips away at this at least a little because bubbles can reveal whether an implementation hands work off to a pre-existing goroutine, or lazily starts background goroutines. However, inheritance does the right thing for any self-contained parallelism, which is more common, whereas not inheriting exposes internal parallelism in most situations.

The argument for not inheriting for secret.Do and crypto/subtle.WithDataIndependentTiming was that they should only be used very tightly around well-understood code. However, this isn't a strong reason; inheritance just didn't seem useful, and we didn't consider broader similarity.

Profile labels attempt to thread this needle by attaching the bubbled state to a Context. This allows background goroutines to disown the bubbled state, and pre-existing goroutines to opt in to some other bubbled state. However, based on a sampling of Go code, this seems to be rarely used functionality. It's possible there should be a broader mechanism for doing this with other bubbled state, but there almost certainly shouldn't be a per-bubble mechanism.

Alternatives considered

We could make starting a goroutine panic within a secret.Do or crypto/subtle.WithDataIndependentTiming bubble. The argument for this is that it's almost certainly a mistake to start a goroutine in what should be tightly-scoped and tightly-controlled code used in these bubbles. But this runs into the problem of exposing whether or not an API uses concurrency internally.

Compatibility

secret.Do has not yet been released, so there is no compatibility concern to changing it.

crypto/subtle.WithDataIndependentTiming was released in Go 1.25. This would be a change in its semantics. However, it's unlikely to affect existing code, and if it does, the worst case is that additional code would now run with data independent timing, causing it to run slower.

aclements avatar Nov 26 '25 21:11 aclements

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

gabyhelp avatar Nov 26 '25 21:11 gabyhelp

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — aclements for the proposal review group

aclements avatar Nov 26 '25 21:11 aclements

crypto/subtle.WithDataIndependentTiming was released in Go 1.25. This would be a change in its semantics. However, it's unlikely to affect existing code, and if it does, the worst case is that additional code would now run with data independent timing, causing it to run slower.

I think this is acceptable, it will not break any cryptography, and while perf regressions in uncommon cases are unfortunate, we can call this out explicitly in release notes.

This will require some additional scheduler complexity (it'll need to know how to set/unset DIT and lock/unlock goroutines from an OS thread when switching between goroutines), but that seems reasonable.

rolandshoemaker avatar Nov 26 '25 21:11 rolandshoemaker

This will require some additional scheduler complexity (it'll need to know how to set/unset DIT and lock/unlock goroutines from an OS thread when switching between goroutines), but that seems reasonable.

Let's explore the implementation to understand the risk here. My guess is that secret.Do will be a trivial, low-risk change that we might as well land now for its first release. If we have to hold off on WithDIT, so be it.

aclements avatar Dec 03 '25 18:12 aclements

Based on the discussion above, this proposal seems like a likely accept. — aclements for the proposal review group

The proposal is to make the "bubbles" created by crypto/subtle.WithDataIndependentTiming and secret.Do inherited by newly started goroutines.

aclements avatar Dec 03 '25 21:12 aclements

If we're going all-in on inheriting these properties in downstream goroutines, I think, like synctest, we should make this propagation observable. synctest bubbles appear in stack traces and text-based goroutine profiles, for example. I think we should require the same observability bits for both DIT and secret.Do.

(My specific concern is with crypto/subtle.WithDataIndependentTiming where accidentally spawning a goroutine with the DIT bit set will just have regular instructions executing more slowly without an obvious cause. That is, it's not obviously visible in CPU profiles, like runtime.addSecret from secret.Do might be.)

Taking this a step further, we could consider also exporting a goroutine count metric in runtime/metrics that specifies how many goroutines currently have the secret and DIT bits set, respectively, but I think that's probably a little less useful, since I expect most deployments probably won't choose to track those metrics. As an aside, we should consider a dump of all metrics something that's possible to do with net/http/pprof. Anyway...

mknyszek avatar Dec 05 '25 16:12 mknyszek

This is the second time I've missed a bubble-related proposal and then put comments in during final call :) . I apologize, I don't intend on making this a regular occurence.

I'd like to make a case for not inheriting secret.Do state across goroutines.

As mentioned, inheriting could "leak" secret.Do state into other goroutines if it happens to be spawning them during its calculation. I think the canonical example for this is net/http.Client, where a goroutine can stick around for as long as the server keeps our connection alive. The obvious sign to the user that something is wrong would be runtime.freeSpecial taking up more CPU time, but that is easy for someone to either miss it, or dismiss it as just requirements of the runtime. Having the cost be hidden in the runtime makes the failure mode hard to diagnose for people who are not intimately familiar with it.

In addition, I don't think anything is enabled by having goroutines inherit secret state. During the initial implementation of secret.Do, there was an idea that you could use it outside the forward-secrecy context. Perhaps you would authenticate a connection with a token that you fetch from the environment and then you "forget" it with secret.Do. As I developed the package, I kept running into various ways in which state was copied into unexpectant places in the runtime, and paired with what an communications library might do behind the scenes, I no longer think that using the package correctly is possible with arbitrary functions. There are just too many ways of leaking state, even with runtime support for zero-ing out free'd memory.

Allowing arbitrary calculation from a secret.Do call gives an implicit promise of confidentiality that we cannot fufill in it's current state. In fact, I feel like there might be gains to be had from restricting what is possible in a secret.Do call even more, but I'd have to experiment more with the interface as I integrate it into crypto/tls to figure out just where the line should be drawn between a usable interface and ease of implementation.

I know this looks like I'm just badmouthing the package I just pushed to include, but I think there's a definite use for it, just much more locked down version, perhaps even one that panics on heap allocations.

As for the other bubbles, I think inheritance there is justified. The performance implications of DIT inheritance are shared with the secret ones, but its implementation much simpler and doesn't constrain the runtime like the secret package does.

DanielMorsing avatar Dec 08 '25 22:12 DanielMorsing

This is the second time I've missed a bubble-related proposal and then put comments in during final call :)

Sorry, I should have spread this proposal more widely. :(

... I no longer think that using the package correctly is possible with arbitrary functions.

If use of secret.Do needs to be very tightly scoped, then doesn't that suggest that it doesn't really matter if secrecy is inherited or not, and therefore consistency wins?

In fact, I feel like there might be gains to be had from restricting what is possible in a secret.Do call even more, ...

An alternative we considered would be for starting a goroutine to panic in secret.Do. I'm curious for your thoughts on that.

The performance implications of DIT inheritance are shared with the secret ones, but its implementation much simpler and doesn't constrain the runtime like the secret package does.

I'm confused by this. Isn't the implementation of inheritance for secret.Do trivial? If the creating goroutine is in secret mode, then wrap the entry of the created goroutine in a secret.Do (or equivalent).

aclements avatar Dec 08 '25 22:12 aclements

As mentioned, inheriting could "leak" secret.Do state into other goroutines if it happens to be spawning them during its calculation.

would tracking that kind of thing be why a bubble should be inherited? Or am I missing something? I would think a bubble would help protect that data rather than creating an untrackable potential leak of it.

BenjamenMeyer avatar Dec 09 '25 15:12 BenjamenMeyer

An alternative we considered would be for starting a goroutine to panic in secret.Do. I'm curious for your thoughts on that.

I think that would be valuable, since it would restrict secret mode being run on just any old function and give an immediate signal to users that there's a potential misuse occuring. Even though there's a big documentation comment, I fear that people will see secret.Do as the catch-all "do the thing, but secretly", rather than a precision tool that allows for zero-ing out things that are opaque to user code (registers, the stack, signal stack, allocations that are "hidden").

It would also solve the problem of inheritance potentially causing an accumulating, silent performance issue, albeit in a bit of a sledgehammer-y way.

We could probably stand to add another run-mode for platforms that don't support secret mode. One where none of the guarantees are attempted, but the runtime will still panic if goroutines are launched. This would prevent code from accidentally developing skew between the platforms, although I suspect we could also solve this issue by just porting the code. Since we ripped out a lot of the gnarly OS signal code, porting should be much simpler.

I'm confused by this. Isn't the implementation of inheritance for secret.Do trivial? If the creating goroutine is in secret mode, then wrap the entry of the created goroutine in a secret.Do (or equivalent).

Ok, I did not explain this well enough. What I'm worried about is the interaction between arbitrary code running in secret mode and the runtime, and leaving us room to adjust the contract. Given what I learned from implementing the package, I expect that secret.Do is going to need to run with some restrictions on what you can do from within that code and I would rather be able to enforce these with panics.

For example, right now, the interaction between inserting/deleting in a map and the memory clearing after a secret allocation is unspecified. The guarantees offered after these operations is also unspecified. I think a reasonable way to fix this problem is to disallow map inserts while running secret code. It is not necessary to be supported for our prototype case of "do some forward secrecy cryptography, throw away the result". Panicking in this situation would also signal to developers that they are performing operations under which there are no secrecy guarantees.

But if the restrictions are paired with goroutines inheriting the bubble state, then we might have code that unexpectedly runs under this mode and leave a time bomb.

I don't see subtle.WithDataIndependentTiming running into these issues. It sets architectual state that is largely outside the go runtime and its interactions with it are far more limited.

DanielMorsing avatar Dec 09 '25 21:12 DanielMorsing

I have a very strong preference against making spawning goroutines inside the bubble panic. In practice it does not encourage precision bubbles, because if you ignore the docs (which say to make precision bubbles) and bubble something large which might have goroutines but currently doesn't, nothing breaks and you don't learn about the risk. Instead, down the line, three libraries deep, someone adds a goroutine a year later and suddenly the program is panic'ing and a confused developer is demanding the open source library rollback the goroutine.

We can't make language limitations silently span module boundaries. This applies to map inserts too, and all future restrictions we might wish to add. This was discussed before. https://github.com/golang/go/issues/74630#issuecomment-3582623681

"You misused secret.Do so your program is not as secure or is slower" is fine. We've gone 10+ years without secure.Do, it's certainly an improvement, but it's not critical, so it's fine if using it wrong fails to help the program that's misusing it. "You misused secret.Do so now your dependencies need to keep one had tied behind their back" is not fine.

As for inherit or not, I don't think it matters: the correct use of secret.Do is around tightly scoped secret manipulation. Correct usage does not involve spawning goroutines, so as long as incorrect usage doesn't have negative externalities (like the panics above), I don't care what it does. Keeping bubbles consistent sounds nice, but weak preference.

FiloSottile avatar Dec 09 '25 23:12 FiloSottile

Change https://go.dev/cl/728921 mentions this issue: runtime/secret: warn users about allocations, loosen gurantees.

gopherbot avatar Dec 10 '25 11:12 gopherbot

Change https://go.dev/cl/728922 mentions this issue: runtime/secret: implement goroutine inheriting secret state

gopherbot avatar Dec 10 '25 11:12 gopherbot

ok, I think we're talking slightly orthogonal issues around secret.Do. What I am worried about is "can we restrict the computation that happens inside a call to secret.Do beyond it fighting us for performance reasons". If that is the case, then inheriting the state in a goroutine is a time bomb. I think my main worry is that while developing secret, it feels like the stars had to align to make it possible and I am frankly a bit surprised that we managed to get a usable package in the end. My fear is that we will come up with some useful optimization in the future and the secret package being there, doing arbitrary computation in a weird execution environment, will severely limit our choices.

Filippo makes an excellent point about language subsets and how that would be a bad thing for composing development with programs yet to be written. If restrictions are off the tables, then bubble inheritance more a choice about what is the least surprising. Having been made aware of yet another potential bubble, I think the standardization on "yes, bubbles inherit" is warranted.

DanielMorsing avatar Dec 11 '25 13:12 DanielMorsing

Having been made aware of yet another potential bubble, I think the standardization on "yes, bubbles inherit" is warranted.

I don't personally see memory regions as a bubble. It's a dynamic scope, and I don't know of a performant implementation that would allow for any kind of useful inheritance. (This has come up in the discussion, and it's possible to extend the implementation in the future, maybe, and/or add a new API for it, but I am doubtful. We're also fighting the language on that front too, and I think we would need to introduce new language/synchronization concepts to actually make it work. But that's outside the scope of this issue.)

Perhaps for that reason the function name for regions should not be Do but Scope or Local or something. I also proposed a way to turn it off via Ignore, though that was unpopular.

mknyszek avatar Dec 11 '25 16:12 mknyszek

My fear is that we will come up with some useful optimization in the future and the secret package being there, doing arbitrary computation in a weird execution environment, will severely limit our choices.

I agree that if we lift secret.Do out of an experiment, we need to really think through about how it might restrict future optimizations, specifically things the compiler cannot do because it doesn't let secret.Do do its job anymore. I suppose the "batch allocations into a tiny block" optimization is one very direct example of this. (It's a little funny that we immediately ran into this problem. 😆) This seems like less of a problem for other the other bubbles.

mknyszek avatar Dec 11 '25 16:12 mknyszek