awnumar / memguard

Secure software enclave for storage of sensitive information in memory.
Apache License 2.0
2.49k stars 124 forks source link

Remove initialization in `init()` #155

Closed srebhan closed 5 months ago

srebhan commented 6 months ago

When using memguard in Telegraf we also need to support constraint environments without memory-locking or memory-protection capabilities (e.g. containers, cloud environments or certain architectures). To do so we switch between an implementation using memguard and one without.

However, because memguard initializes some parts in init() functions (e.g. here) we will require the capabilities mentioned above even without using the functionality as-soon-as we are importing the package. This makes it impossible to switch between a non-protected and protected solution during runtime.

Instead we should initialize the required parts either explicitly (requiring a v2 as it is a breaking change) or by using sync.Once at the first use of the functionality.

I'm willing to contribute a PR if there is an agreement on the way forward.

awnumar commented 6 months ago

It is unfortunate that the implementation currently relies on package-level global variables. It would be better if everything was scoped to individual memguard.Instance things that stored their own state about what Buffers exist, what the Coffer key is, etc. And then perhaps also a single thread-safe global list of these instances for the purposes of cleaning up at program termination.

This would be a breaking change though (which should be okay to make for the time being given we are pre-v1). But for the time being it's probably easier to go with your other suggestion of using a sync.Once on the first usage of a Coffer.

I've seen suggestions in the past of having an environment variable to turn off memory locking, or detecting the ability to lock at runtime, but these are somewhat more complicated routes too.

awnumar commented 6 months ago

156 may incidentally help your use-case, let me know what you think

srebhan commented 5 months ago

@awnumar I can confirm that #156 fixes the issue for us (see user comment in https://github.com/influxdata/telegraf/pull/13998).