Open srebhan opened 9 months ago
@awnumar any comments?
Hi, thanks for the PR. I'm sorry I haven't had time to review it yet
@awnumar any comment?
@awnumar ping...
@awnumar any news on this PR? Anything I can do to get it merged?
@srebhan sorry again for the delay. I sadly don't have a lot of free time lately.
The PR looks good to me. I have been thinking about how this could actually be integrated into the API. I think it would require an overhaul of how the library is used. In particular, users should be able to initialise an allocator object that they use to create buffers and enclaves, something like:
type Allocator struct {
ma MemAllocator
key Coffer
buffers []Buffer
}
this would also allow us to remove all global state that we currently store within the library. Also it'd be nice to take the opportunity of introducing a breaking change to perform some other clean-up actions:
Allocator
instance would make that easier to handle for them.core
and root libraries. The layering is confusing and doesn't add anything.In terms of your changes in this PR, it feels strange to have the new allocator merged into master when it would remain unused for the time being. How would you feel about either:
@awnumar no worries!
Regarding your suggestion, I'm all for a v2
with the breaking changes you suggested. Being able to create multiple, isolated memguard instances is a good thing IMO. A general interface to memguard could look like this
package memguard
import "runtime"
type Option func(*Guard)
func WithAllocator(allocator MemAllocator) Option {
return func(g *Guard) { g.allocator = allocator }
}
type Guard struct {
allocator MemAllocator
buffers bufferList
key *Coffer
}
func NewGuard(options ...Option) *Guard {
g := &Guard{}
// Apply the options
for _, opt := range options {
opt(g)
}
// Filling in defaults for mandatory options and initialize fields
if g.allocator == nil {
g.allocator = NewPageAllocator()
}
g.key = NewCoffer()
// Make sure we don't leave unencrypted data behind
runtime.SetFinalizer(g, func(dg *Guard){dg.Destroy()})
return g
}
func (g *Guard) Destroy() {
...
}
func (g *Guard) NewBuffer(size int) (*Buffer, error) {
if size < 1 {
return nil, ErrNullBuffer
}
b := new(Buffer)
// Allocate the total needed memory
var err error
b.data, err = g.allocator.Alloc(size)
if err != nil {
Panic(err)
}
// Set remaining properties
b.alive = true
b.mutable = true
// Append the container to list of active buffers.
g.buffers.add(b)
// Return the created Buffer to the caller.
return b, nil
}
...
I'm also willing to help out here, if you could spend some time to review my changes!?!? If so I do propose the following steps:
v2
version.v2
.go.mod
to github.com/awnumar/memguard/v2
, potentially also bump the minimum go version if you like.core
into the main package.init()
functions as good as possible.v2
v2
What do you think?
Hi all, would love to see this project move along and stay maintained. I don't have a ton of time but I would be happy to test things for you @srebhan or @awnumar if that is helpful. Happy also to provide a code review as well if that is helpful.
@srebhan
It's strange to go to /V2 when it's currently not even v1
In terms of the API, how about passing a config struct instead of passing functions to configure it? The approach sounds good overall
@awnumar sorry for the late reply!
It's strange to go to /V2 when it's currently not even v1
Also fine with calling it v1
. ;-)
In terms of the API, how about passing a config struct instead of passing functions to configure it?
The functional option pattern has some strong benefits, but the biggest one IMO is that you can modify specific options only without touching the defaults of other config options. This is especially relevant if you add a new field as every user providing an explicit config will now silently override the new field with nil
...
@srebhan
The functional option pattern has some strong benefits, but the biggest one IMO is that you can modify specific options only without touching the defaults of other config options.
Yeah it does have some benefits, though it's also more verbose leading to long lines or arbitrary newlines in the code. Config structs are used throughout the standard library so they're familiar and easy to reason about.
Also, if a user does not provide a value in a struct (therefore it gets assigned the zero value), this should be interpreted as "leave that option as the default value". This is similar to what the standard library does, e.g. here.
This is especially relevant if you add a new field as every user providing an explicit config will now silently override the new field with nil...
I believe this is a problem shared with the "passing funcs as configs" pattern as any code written before a new config option is added will automatically be assigned the default value.
This PR factors out the memory allocation (and disposal) of buffers to a
MemAllocator
interface abstracting memory allocation strategies. Afterwards, the page-allocation strategy is implemented as an instance of aMemAllocator
including unit-test. Finally, a SLAB allocation strategy is implement to be able to make better use of locked memory pages for systems with a very limited number of available locked pages or for software with many secrets handled by memguard, such as Telegraf.provides a basis to implement #124 fixes #148