[Suggestions] Can the API be improved?

[awnumar]

As we move closer to v1.0.0, do you have any suggestions for improvements to the API?

I've used the library myself in my own projects, so primarily that's the only indication I have of the suitability, ease-of-use, and just general quality of the API.

So, to those of you that use memguard (or not), add a comment with your opinions or suggestions.

[shaleh]

This was an easy drop in replacement for password handling in some code I was just working on. A quick change of types and calling Buffer() instead of casting the string to []byte. Thanks.

One API question. Is EqualTo the right choice? The bytes package exposes Equal. I would think having the buffer emulate bytes would be more natural. You already do this with the Equal function that checks two buffers.

[awnumar]

@shaleh Yeah, you should definitely use the comparison functions exposed in the memguard API over any others. That's because we perform comparisons in constant-time, which prevents timing-attacks.

[shaleh]

Sorry, I was not clear.

What I meant to say is I think the name of the EqualTo method should be Equal to match what the bytes package exposes and what you expose with the Equal function. Without looking I first used passphrase.Equal() and had to fix the call in my code.

[awnumar]

@shaleh Ahh, good shout. Do you think it will cause confusion with the Equal function?

[shaleh]

That is a good question. Do you expect more people will want to compare LockedBuffer or compare a LockedBuffer to []byte? I expect comparison to []byte to be more common but I have a very focused use case.

Maybe you could flip the uses? Make the []byte comparison be a function and the method handle checking against another LockedBuffer?

The name EqualTo does not make it any more clear that the expected parameter is a []byte.

[shaleh]

While we are discussing names, why LockedBuffer? Not saying you need to change it, I was just not expecting it.

[awnumar]

@shaleh I expect more comparisons between LockedBuffers than between bytes, but my expectations mean very little. I'll definitely think about the naming again. I don't think that they should be flipped from functions to methods, and vice versa, though.

And it's funny you should mention that. I'm planning on changing the name of a container too, but can't settle on one. My current best attempt is SecureEnclave or some variation. Maybe just Enclave.

[josephlr]

Personally, I like the Locked/Unlocked naming, as it matches with what the OS does (mlock, VirtualLock, etc...)

SecureEnclave could potentially collide with Intex SGX, which refers to their isolated code areas as "Secure Enclaves".

[awnumar]

@shaleh I renamed EqualTo => EqualBytes. This makes the intention of the function more clear.

I can't keep them both named Equal since they would conflict with each other. I kept Equal as is, since it follows the bytes package more closely that way. As in, bytes.Equal() compares bytes, and memguard.Equal() compares LockedBuffers.

[shaleh]

Sounds good.

[awnumar]

There is an unmerged, active branch right now where LockedBuffer has been renamed to Enclave. Originally maybe LockedBuffer was appropriate because of what was actually going on in the background, but as the library stands now, mlock is a small part of what we do.

I know it breaks reverse-compatibility and @josephlr, in particular, noted that the name may conflict, but my main motivation for the rename is seeing functions like this:

func password() (*memguard.LockedBuffer, error) {
    /...
}

It's just too long a name for a type. I briefly considered naming it a single letter too, like the testing package does, but couldn't settle on one. I think Enclave is short enough unless someone has another suggestion. I'll hold off on merging the change for a while.

[shaleh]

Why not just call it Buffer? The memguard module name gives sufficient clarity.

On October 22, 2017 7:56:18 AM PDT, Awn notifications@github.com wrote:

There is an unmerged, active branch right now where LockedBuffer has been renamed to Enclave. Originally maybe LockedBuffer was appropriate because of what was actually going on in the background, but as the library stands now, mlock is a small part of what we do.

I know it breaks reverse-compatibility and @josephlr, in particular, noted that the name may conflict, but my main motivation for the rename is seeing functions like this:

func password() (*memguard.LockedBuffer, error) {
   /...
}

It's just too long a name for a type. I briefly considered naming it a single letter too, like the testing package does, but couldn't settle on one. I think Enclave is short enough unless someone has another suggestion. I'll hold off on merging the change for a while.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/awnumar/memguard/issues/32#issuecomment-338483645

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[awnumar]

Hmm, Buffer could work. But then you'd have a .Buffer() inside a memguard.Buffer.

EDIT:

Nah, you're right. We already have a LockedBuffer so that's not an issue.

EDIT II:

I still do prefer Enclave though. I think the connotations of an enclave are that there's more to it than just a region of memory. Our container exposes a mutex, it has various states, it's stored differently, et al. It doesn't just point to some memory location and that's all.

[shaleh]

That could be .Enclave or .Protected

On October 22, 2017 8:35:45 AM PDT, Awn notifications@github.com wrote:

Hmm, Buffer could work. But then you'd have a .Buffer() inside a memguard.Buffer.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/awnumar/memguard/issues/32#issuecomment-338486477

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[anitgandhi]

How possible is it to have LockedBuffers of other data types, e.g. []uint32?

Usecase for this: Go's AES implementation takes in a key and then expands it into the AES key schedule. This process is deterministic so you can argue that they should be protected using something like memguard too. Right now it just does make([]uint32, n): https://github.com/golang/go/blob/master/src/crypto/aes/cipher.go#L46

While it wouldn't be impossible to change how the expansion works to use a byte slice instead, perhaps it might be possible to have memguard support other data type slices.

[awnumar]

@anitgandhi Seems like it's possible. I'll try to think of a clean way of incorporating it into the API.

[anitgandhi]

Interesting. I'll have to try a PoC with that.

I suppose there might be some endian-ness aspects to be aware of, though

[awnumar]

@anitgandhi The least "disturbing" solution I can think of is just to add a bunch of methods like,

func (b LockedBuffer) Int8() []int8
func (b LockedBuffer) Int16() []int16
func (b LockedBuffer) Int32() []int32
...

[anitgandhi]

Thanks for the latest version with the additional data types support @awnumar ! Can't share the code at the moment but I was able to drop it in pretty easily to the existing Go AES code from the standard library and all the tests pass 👍

[anitgandhi]

Update, here's the code of the PoC!

https://github.com/anitgandhi/aesguard

[au-phiware]

Thank you for this library, I intend to use it in an upcoming project and would love to see it graduate to v1! I will also make use of the Uint64 function, but it makes me think that you should rename Buffer to Bytes, which is more in line with bytes.Buffer.

Speaking of bytes.Buffer have you considered implementing the io.Reader, io.ReaderAt, io.WriterTo, io.Seeker, io.ByteScanner, etc. interfaces?

[awnumar]

@au-phiware I've renamed the method as you suggested. It should be merged along with other breaking changes.

Regarding io, can you elaborate on why those interfaces would be useful? Especially considering how small the buffers will usually be.

[au-phiware]

I opened a new issue to discuss io: #68 :smiley:

[awnumar]

Question to all of you:

Currently the Trim function preserves the given container and doesn't destroy it. Is this intuitive? Would you have expected different behaviour?

What about if Trim was a method and updated a container in place? If a Grow method is also added, it could behave in the same way. Would that be better?

Changing the behaviour would be a breaking change, but there are a number of very large, very breaking changes coming up and so I think if I was to change the behaviour, now would be a good time.