Repository: ceph/go-ceph
Pull request #533 (2021-07-15 12:30:23)

rbd: support image encryption in pacific
## Summary of changes:
Ceph pacific added image encryption (https://docs.ceph.com/en/latest/rbd/rbd-encryption/).  This change adds support to the encryption format api which enables the encryption by creating the encryption header in the image.  

Note that as part of this change I have updated the Makefile to support Pacific by default.  I hope that's ok.

  Signed-off-by: Effi Ofer <effio@il.ibm.com>

## Checklist
- [x] Added tests for features and functional changes
- [x] Public functions and types are documented
- [x] Standard formatting is applied to Go code


phlogistonjohn: One more comment, I forgot:
* The format we do for commit messages is "{topic}: {description}"

Topic is generally the package name. In your case that'd be "rbd". So something like "rbd: add EncryptionFormat method to rbd image type". This is not a hard requirement, beyond setting the topic, just a guideline.
phlogistonjohn: One possible way to adapt the api and remove the use of unsafe.Pointer:
```
type EncryptionOptions interface {
    cephEncyptionOptions() (C.rbd_encryption_format_t, C.rbd_encryption_options_t)
}

type EncryptionOptionsLUKS1 {
    Algorithm     EncryptionAgorithmLUKS1
    Passphrase string
}

func (e EncryptionOptionsLUKS1) cephEncyptionOptions() (C.rbd_encryption_format_t, C.rbd_encryption_options_t) {
   // ... convert go types to C types
}

func (image *Image) EncryptionFormat(opts EncryptionOptions) error {
  // ... implementation ...
}

```

Just a quick thought/suggestion. 
Since we're talking api design - CCing @ansiwen 
effi-ofer:  Thanks for all the helpful comments @phlogistonjohn and @nixpanic.  Let me review them and get back to you.  Just a heads up, it might be as late as Monday before I do :)  
phlogistonjohn: > Thanks for all the helpful comments @phlogistonjohn and @nixpanic. Let me review them and get back to you. Just a heads up, it might be as late as Monday before I do :)

That's perfectly reasonable. Feel free to ask any questions on our feedback as needed. Have a nice weekend. :-)
phlogistonjohn: One last suggestion today:
* Name the files encryption.go and encryption_test.go. It makes it clear what area the code covers and will never go "out of date" with newer ceph releases (the build tags remain the same).
effi-ofer: > One possible way to adapt the api and remove the use of unsafe.Pointer:
> 
> ```go
> type EncryptionOptions interface {
>     cephEncyptionOptions() (C.rbd_encryption_format_t, C.rbd_encryption_options_t)
> }
> 
> type EncryptionOptionsLUKS1 {
>     Algorithm     EncryptionAgorithmLUKS1
>     Passphrase string
> }
> 
> func (e EncryptionOptionsLUKS1) cephEncyptionOptions() (C.rbd_encryption_format_t, C.rbd_encryption_options_t) {
>    // ... convert go types to C types
> }
> 
> func (image *Image) EncryptionFormat(opts EncryptionOptions) error {
>   // ... implementation ...
> }
> ```
> 
> Just a quick thought/suggestion.
> Since we're talking api design - CCing @ansiwen

Good suggestion.  Accepted and implemented.
effi-ofer: Hi @phlogistonjohn and @nixpanic, I have made the changes you requested.  Please take a look again.  Thanks in advance.  
phlogistonjohn: There's also some issues detected in the CI. FWIW with recent changes to the policy on github I have to hit a button every time you submit changes to run the CI. So please bear with us.
phlogistonjohn: > @phlogistonjohn is there a way for me to manually run the CI tests? I just got a few failures on the comments, and it would be great if I can resolve them before I update the PR.

You can't trigger this ci yourself because of [github policies](https://github.blog/2021-04-22-github-actions-update-helping-maintainers-combat-bad-actors/). However, you *can* run the same tests that the CI executes:

You need to have a working container runtime locally, such as podman or docker. Then you can execute `make test-container CEPH_VERSION=pacific`. If all works as it should it will build and run your code [1]. You can also run `make check` to run the linter tool `revive` - but you'll need revive locally - we have not created a container based version of this yet.

Let me know if you have any additional questions.

[1] - there are various options such as ENTRYPOINT_ARGS that can help speed up this process. In this case you can test only rbd package by setting `ENTRYPOINT_ARGS='--test-pkg=rbd`. There's more, but that should cover you for now.
ansiwen: @effi-ofer You last force push didn't bring any changes. Maybe you forgot to commit the changes?
effi-ofer: > Sorry for being late for the party. First, also thanks for the contribution, @effi-ofer!
> 
> I have some issues with the API of `EncryptionFormat()`: we have some redundancy here, because the "format" is both encoded in the `format` and the `opts` arguments, which can lead to inconsistencies or even crashes. One of the major advantages of Go is that it can be significantly safer than C, so let's try to keep that feature. ;-) That means we need to make sure that the `format` argument of the `rbd_encryption_format()` call is always consistent with what its `opts` pointer is pointing to. The first two solutions that come to my mind are:
> 
> 1. We drop the `format` argument and add a `format()` method to the `EncryptionOptions` interface, which returns the according `EncryptionFormat`.
> 
> or, what I actually prefer
> 
> 1. We don't use the interface but a single struct for both LUKS 1 and 2. After all we have to create the C structs anyway, so I don't really see the advantage of keeping them separate, even _if_ they might change in the future. We could easily add the missing info to the struct later or even change it to an interface later, because that could be backwards compatible. That would also remove some code bloat and fix the problem with the freeing of the `CBytes()`. The whole implementation could look something like that:
> 
> ```go
> type EncryptionOptions struct {
> 	Alg        EncryptionAlgorithm
> 	Passphrase []byte
> }
> 
> func (image *Image) EncryptionFormat(format EncryptionFormat, opts EncryptionOptions) error {
> 	if image.image == nil {
> 		return ErrImageNotOpen
> 	}
> 
> 	var cOptsSize C.size_t
> 	var cOptsPtr C.rbd_encryption_options_t
> 	cPassphrase := (*C.char)(C.CBytes(opts.Passphrase))
> 	defer C.free(unsafe.Pointer(cPassphrase))
> 	switch format {
> 	case EncryptionFormatLUKS1:
> 		var cOpts C.rbd_encryption_luks1_format_options_t
> 		cOpts.alg = C.rbd_encryption_algorithm_t(opts.Alg)
> 		cOpts.passphrase = cPassphrase
> 		cOpts.passphrase_size = C.size_t(len(opts.Passphrase))
> 		cOptsSize = C.sizeof_rbd_encryption_luks1_format_options_t
> 		cOptsPtr = C.rbd_encryption_options_t(&cOpts)
> 	case EncryptionFormatLUKS2:
> 		var cOpts C.rbd_encryption_luks2_format_options_t
> 		cOpts.alg = C.rbd_encryption_algorithm_t(opts.Alg)
> 		cOpts.passphrase = cPassphrase
> 		cOpts.passphrase_size = C.size_t(len(opts.Passphrase))
> 		cOptsSize = C.sizeof_rbd_encryption_luks2_format_options_t
> 		cOptsPtr = C.rbd_encryption_options_t(&cOpts)
> 	default:
> 		return ErrEncryptionFormat
> 	}
> 
> 	ret := C.rbd_encryption_format(
> 		image.image,
> 		C.rbd_encryption_format_t(format),
> 		cOptsPtr,
> 		cOptsSize)
> 	return getError(ret)
> }
> ```

Thanks @ansiwen and welcome to the party!
Those are good points and I adopted something along the line of  option #1.  
effi-ofer: Greetings, I am getting this check failure "method receiver 'opts' is not referenced in method's body, consider removing or renaming it" for functions like  
```
func (opts EncryptionFormatOptsLUKS1) getFormat() format {
   return EncryptionFormatLUKS1
}
```
Is there a way for me to bypass or ignore this check?
ansiwen: > Greetings, I am getting this check failure "method receiver 'opts' is not
> referenced in method's body, consider removing or renaming it" for
> functions like
>
> func (opts EncryptionFormatOptsLUKS1) getFormat() format {
>    return EncryptionFormatLUKS1
> }
>
> Is there a way for me to bypass or ignore this check?
>

Although you don’t need it anymore: you can write `func (_
EncryptionFormatOptsLUKS1) getFormat() …` if you don’t use an argument.

ansiwen: Ok, the linter is complaining about too many return arguments. And I have to admit that it's right.

Let's pack the return value in a struct. Then it's also more explicit, that the returned function is a free function.

```go
type cData struct {
  format   C.rbd_encryption_format_t
  opts     C.rbd_encryption_options_t
  optsSize C.size_t
  free     func()
}
```

phlogistonjohn: So I'm glad we're converging on something everyone is happy with, but now the linter is unhappy. By default it doesn't want to detect more than 3 values being returned. I think a simple valid way to avoid this is to keep the last three values of the allocateEncryptionOptions function but move the first back to a `format()` function in the interface like we had before. I *hope* everyone would be ok with that.

Other options would be to either return a struct - not something I'd prefer. Or we can add a linter directive comment - but I don't love that idea either.
phlogistonjohn: @ansiwen sorry - I didn't see your comment until after I wrote mine. I think its OK as its all internal after all. So I think @effi-ofer can pick from either of our suggestions. :-)
ansiwen: 
> Other options would be to either return a struct - not something I'd prefer.

Can you elaborate on the reasons?

Independently of the linter I already had the desire to put it in the struct, but I resisted to make further change-requests. ;-) It would actually make the code shorter in total and more readable. But more importantly, it would give that `func()` a name (which is not a _real_ problem in such short code I admit).

> Or we can add a linter directive comment - but I don't love that idea either.

That you prefer to "inferiprove" code to make the linter shut up instead of directly telling it to shut up is really shocking, btw. 😜

phlogistonjohn: I wish I had caught this sooner:
One small thing, is we prefer (but don't currently strictly enforce) a [Signed-off-by (aka DCO) line in the commit](https://stackoverflow.com/questions/1962094/what-is-the-sign-off-feature-in-git-for).
If it's not too much of a hassle I'd like to see that in your commit message.
effi-ofer: > I wish I had caught this sooner:
> One small thing, is we prefer (but don't currently strictly enforce) a [Signed-off-by (aka DCO) line in the commit](https://stackoverflow.com/questions/1962094/what-is-the-sign-off-feature-in-git-for).
> If it's not too much of a hassle I'd like to see that in your commit message.

Done
ansiwen: @Mergifyio rebase
mergify[bot]: **Command `rebase`: success**
> **Branch has been successfully rebased**
> 

ansiwen: > Thanks for the signed off! Looks like a "gofmt" is needed. But generally looks good to me. Once the CI is happy I'll probably be able to approve.

Yeah, I was a bit confused and restarted the job. Is it normal, that it doesn't show which line?
effi-ofer: Oh shot. I managed to add a blank space that fmt is failing on.  My bad for rushing the commit remark.  Give me a hour or two and I'll patch that.   
phlogistonjohn: > Oh shot. I managed to add a blank space that fmt is failing on. My bad for rushing the commit remark. Give me a hour or two and I'll patch that.
> 
> Update: I wrote the above before I saw your messages @phlogistonjohn and @ansiwen. If you don't mind approving and accepting that would be awesome. I can remove the space in my rbd_encryption_load PR. Thanks again.

I appreciate the sentiment but I rely on the CI to make sure nothing breaking crept in at the last minute, so we can't defer this fix to the next pr as github actions won't run the "real" tests unless "check" is passing. Sorry for the annoyance but we do need this one change in this PR.
phlogistonjohn: Thanks a bunch! CI restarted.

Patch:
diff --git a/rbd/encryption.go b/rbd/encryption.go
new file mode 100644
index 0000000..4b1a6d0
--- /dev/null
+++ b/rbd/encryption.go
@@ -0,0 +1,116 @@
+// +build !octopus,!nautilus
+
+package rbd
+
+// #cgo LDFLAGS: -lrbd
+// /* force XSI-complaint strerror_r() */
+// #define _POSIX_C_SOURCE 200112L
+// #undef _GNU_SOURCE
+// #include <errno.h>
+// #include <stdlib.h>
+// #include <rados/librados.h>
+// #include <rbd/librbd.h>
+import "C"
+
+import (
+	"unsafe"
+)
+
+// cData contains the data needed by the encryption functions
+type cData struct {
+	format   C.rbd_encryption_format_t
+	opts     C.rbd_encryption_options_t
+	optsSize C.size_t
+	free     func()
+}
+
+// EncryptionAlgorithm is the encryption algorithm
+type EncryptionAlgorithm C.rbd_encryption_algorithm_t
+
+// Possible values for EncryptionAlgorithm:
+// EncryptionAlgorithmAES128: AES 128bits
+// EncryptionAlgorithmAES256: AES 256bits
+const (
+	EncryptionAlgorithmAES128 = EncryptionAlgorithm(C.RBD_ENCRYPTION_ALGORITHM_AES128)
+	EncryptionAlgorithmAES256 = EncryptionAlgorithm(C.RBD_ENCRYPTION_ALGORITHM_AES256)
+)
+
+// EncryptionOptionsLUKS1 and EncryptionOptionsLUKS2 are identical
+// structures at the moment, just as they are in the librbd api.
+// The purpose behind creating different identical structures, is to facilitate
+// future modifications of one of the formats, while maintaining backwards
+// compatibility with the other.
+
+// EncryptionOptionsLUKS1 options required for LUKS v1
+type EncryptionOptionsLUKS1 struct {
+	Alg        EncryptionAlgorithm
+	Passphrase []byte
+}
+
+// EncryptionOptionsLUKS2 options required for LUKS v2
+type EncryptionOptionsLUKS2 struct {
+	Alg        EncryptionAlgorithm
+	Passphrase []byte
+}
+
+// EncryptionOptions interface is used to encapsulate the different encryption
+// formats options and enable converting them from go to C structures.
+type EncryptionOptions interface {
+	allocateEncryptionOptions() cData
+}
+
+func (opts EncryptionOptionsLUKS1) allocateEncryptionOptions() cData {
+	var cOpts C.rbd_encryption_luks1_format_options_t
+	var retData cData
+	cOpts.alg = C.rbd_encryption_algorithm_t(opts.Alg)
+	//CBytes allocates memory which we'll free by calling cOptsFree()
+	cOpts.passphrase = (*C.char)(C.CBytes(opts.Passphrase))
+	cOpts.passphrase_size = C.size_t(len(opts.Passphrase))
+	retData.opts = C.rbd_encryption_options_t(&cOpts)
+	retData.optsSize = C.size_t(C.sizeof_rbd_encryption_luks1_format_options_t)
+	retData.free = func() { C.free(unsafe.Pointer(cOpts.passphrase)) }
+	retData.format = C.RBD_ENCRYPTION_FORMAT_LUKS1
+	return retData
+}
+
+func (opts EncryptionOptionsLUKS2) allocateEncryptionOptions() cData {
+	var cOpts C.rbd_encryption_luks2_format_options_t
+	var retData cData
+	cOpts.alg = C.rbd_encryption_algorithm_t(opts.Alg)
+	//CBytes allocates memory which we'll free by calling cOptsFree()
+	cOpts.passphrase = (*C.char)(C.CBytes(opts.Passphrase))
+	cOpts.passphrase_size = C.size_t(len(opts.Passphrase))
+	retData.opts = C.rbd_encryption_options_t(&cOpts)
+	retData.optsSize = C.size_t(C.sizeof_rbd_encryption_luks2_format_options_t)
+	retData.free = func() { C.free(unsafe.Pointer(cOpts.passphrase)) }
+	retData.format = C.RBD_ENCRYPTION_FORMAT_LUKS2
+	return retData
+}
+
+// EncryptionFormat creates an encryption format header
+//
+// Implements:
+//  int rbd_encryption_format(rbd_image_t image,
+//                            rbd_encryption_format_t format,
+//                            rbd_encryption_options_t opts,
+//                            size_t opts_size);
+//
+// To issue an IO against the image, you need to mount the image
+// with libvirt/qemu using the LUKS format, or make a call to
+// rbd_encryption_load().
+func (image *Image) EncryptionFormat(opts EncryptionOptions) error {
+	if image.image == nil {
+		return ErrImageNotOpen
+	}
+
+	encryptionOpts := opts.allocateEncryptionOptions()
+	defer encryptionOpts.free()
+
+	ret := C.rbd_encryption_format(
+		image.image,
+		encryptionOpts.format,
+		encryptionOpts.opts,
+		encryptionOpts.optsSize)
+
+	return getError(ret)
+}


Test patch:
diff --git a/rbd/encryption_test.go b/rbd/encryption_test.go
new file mode 100644
index 0000000..8bad7ce
--- /dev/null
+++ b/rbd/encryption_test.go
@@ -0,0 +1,47 @@
+// +build !octopus,!nautilus
+
+package rbd
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestEncryptionFormat(t *testing.T) {
+	conn := radosConnect(t)
+
+	poolname := GetUUID()
+	err := conn.MakePool(poolname)
+	assert.NoError(t, err)
+
+	ioctx, err := conn.OpenIOContext(poolname)
+	require.NoError(t, err)
+
+	name := GetUUID()
+	testImageSize := uint64(1 << 23) // format requires more than 4194304 bytes
+	options := NewRbdImageOptions()
+	assert.NoError(t,
+		options.SetUint64(ImageOptionOrder, uint64(testImageOrder)))
+	err = CreateImage(ioctx, name, testImageSize, options)
+	assert.NoError(t, err)
+
+	workingImage, err := OpenImage(ioctx, name, NoSnapshot)
+	assert.NoError(t, err)
+
+	var opts EncryptionOptionsLUKS1
+	opts.Alg = EncryptionAlgorithmAES256
+	opts.Passphrase = ([]byte)("test-password")
+	err = workingImage.EncryptionFormat(opts)
+	assert.NoError(t, err)
+
+	err = workingImage.Close()
+	assert.NoError(t, err)
+	err = workingImage.Remove()
+	assert.NoError(t, err)
+
+	ioctx.Destroy()
+	conn.DeletePool(poolname)
+	conn.Shutdown()
+}