Since we now have the functionality to provide the secrets driver with an encryption key, and the newly introduced attribute to store the encryption scheme across driver restarts, we can use the key to encrypt secrets. While loading the secrets, we check whether the secret is encrypted or not and accordingly get the value.
While the stored encryption scheme ensures the driver can successfully load secrets after a restart, If the user changes the encryption key between driver restarts, any secrets encrypted with the previous key will become permanently inaccessible upon the next restart. Users must ensure key consistency to maintain access to existing encrypted secrets. Signed-off-by: Arun Menon <[email protected]> --- src/conf/virsecretobj.c | 165 ++++++++++++++++++++++++++++++------- src/conf/virsecretobj.h | 10 ++- src/secret/secret_driver.c | 23 ++++-- 3 files changed, 157 insertions(+), 41 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 66270e2751..37b2db960f 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -31,6 +31,10 @@ #include "virhash.h" #include "virlog.h" #include "virstring.h" +#include "virsecret.h" +#include "virrandom.h" +#include "vircrypto.h" +#include "virsecureerase.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -328,6 +332,8 @@ virSecretObjListAdd(virSecretObjList *secrets, virSecretObj *obj; virSecretDef *objdef; virSecretObj *ret = NULL; + const char *encryptionScheme = NULL; + const char *encryptionSchemeExt = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; virObjectRWLockWrite(secrets); @@ -379,10 +385,26 @@ virSecretObjListAdd(virSecretObjList *secrets, goto cleanup; /* Generate the possible configFile and base64File strings - * using the configDir, uuidstr, and appropriate suffix + * using the configDir, uuidstr, and appropriate encryption scheme */ - if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || - !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) + if ((*newdef)->encryption_scheme != VIR_SECRET_ENCRYPTION_SCHEME_NONE + && (*newdef)->encryption_scheme != -1) { + encryptionScheme = virSecretEncryptionSchemeTypeToString((*newdef)->encryption_scheme); + if (!encryptionScheme) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown secret encryption scheme %1$d"), (*newdef)->encryption_scheme); + goto cleanup; + } + encryptionSchemeExt = g_strconcat(".", encryptionScheme, NULL); + if (!(obj->base64File = virFileBuildPath(configDir, uuidstr, encryptionSchemeExt))) { + goto cleanup; + } + } else { + if (!(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) { + goto cleanup; + } + } + if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml"))) goto cleanup; if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) @@ -397,6 +419,7 @@ virSecretObjListAdd(virSecretObjList *secrets, cleanup: virSecretObjEndAPI(&obj); virObjectRWUnlock(secrets); + g_clear_pointer((gpointer *)&encryptionSchemeExt, g_free); return ret; } @@ -680,17 +703,49 @@ virSecretObjSaveConfig(virSecretObj *obj) return 0; } - int -virSecretObjSaveData(virSecretObj *obj) +virSecretObjSaveData(virSecretObj *obj, + virSecretDaemonConfig *driverConfig) { g_autofree char *base64 = NULL; + g_autofree uint8_t *encryptedValue = NULL; + size_t encryptedValueLen = 0; + size_t base64Len = 0; + uint8_t iv[16] = { 0 }; if (!obj->value) return 0; - base64 = g_base64_encode(obj->value, obj->value_size); - + if (obj->def->encryption_scheme == VIR_SECRET_ENCRYPTION_SCHEME_NONE + || obj->def->encryption_scheme == -1) { + base64 = g_base64_encode(obj->value, obj->value_size); + } else { + if (driverConfig == NULL || driverConfig->secrets_encryption_key == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot encrypt secret value without encryption key")); + return -1; + } + if (virRandomBytes(iv, sizeof(iv)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate random IV")); + return -1; + } + if (virCryptoEncryptData(VIR_CRYPTO_CIPHER_AES256CBC, + driverConfig->secrets_encryption_key, driverConfig->secretsKeyLen, + iv, sizeof(iv), + (uint8_t *)obj->value, obj->value_size, + &encryptedValue, &encryptedValueLen) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to encrypt secret value")); + return -1; + } + base64Len = sizeof(iv) + encryptedValueLen; + base64 = g_new0(char, base64Len); + memcpy(base64, iv, sizeof(iv)); + memcpy(base64 + sizeof(iv), encryptedValue, encryptedValueLen); + /* Now the secret is encrypted and stored on disk. However, + * we did not change anything in the obj->value. This is done on + * purpose, as SecretObjGetValue should be able to read it as is. + * This will indeed be a base64 encoded secret*/ + } if (virFileRewriteStr(obj->base64File, S_IRUSR | S_IWUSR, base64) < 0) return -1; @@ -733,27 +788,25 @@ virSecretObjGetValue(virSecretObj *obj) return ret; } - int virSecretObjSetValue(virSecretObj *obj, const unsigned char *value, - size_t value_size) + size_t value_size, + virSecretDaemonConfig *driverConfig) { virSecretDef *def = obj->def; g_autofree unsigned char *old_value = NULL; g_autofree unsigned char *new_value = NULL; size_t old_value_size; - new_value = g_new0(unsigned char, value_size); old_value = obj->value; old_value_size = obj->value_size; - memcpy(new_value, value, value_size); obj->value = g_steal_pointer(&new_value); obj->value_size = value_size; - if (!def->isephemeral && virSecretObjSaveData(obj) < 0) + if (!def->isephemeral && virSecretObjSaveData(obj, driverConfig) < 0) goto error; /* Saved successfully - drop old value */ @@ -786,7 +839,6 @@ virSecretObjSetValueSize(virSecretObj *obj, obj->value_size = value_size; } - static int virSecretLoadValidateUUID(virSecretDef *def, const char *file) @@ -807,11 +859,18 @@ virSecretLoadValidateUUID(virSecretDef *def, static int -virSecretLoadValue(virSecretObj *obj) +virSecretLoadValue(virSecretObj *obj, + virSecretDaemonConfig *driverConfig) { int ret = -1, fd = -1; struct stat st; g_autofree char *contents = NULL; + g_autofree uint8_t *contents_encrypted = NULL; + g_autofree uint8_t *decryptedValue = NULL; + size_t decryptedValueLen = 0; + uint8_t iv[16] = { 0 }; + uint8_t *ciphertext = NULL; + size_t ciphertextLen = 0; if ((fd = open(obj->base64File, O_RDONLY)) == -1) { if (errno == ENOENT) { @@ -841,25 +900,65 @@ virSecretLoadValue(virSecretObj *obj) goto cleanup; } - contents = g_new0(char, st.st_size + 1); - - if (saferead(fd, contents, st.st_size) != st.st_size) { - virReportSystemError(errno, _("cannot read '%1$s'"), - obj->base64File); - goto cleanup; + if (obj->def->encryption_scheme == VIR_SECRET_ENCRYPTION_SCHEME_NONE || + obj->def->encryption_scheme == -1) { + contents = g_new0(char, st.st_size + 1); + if (saferead(fd, contents, st.st_size) != st.st_size) { + virReportSystemError(errno, _("cannot read '%1$s'"), + obj->base64File); + goto cleanup; + } + contents[st.st_size] = '\0'; + obj->value = g_base64_decode(contents, &obj->value_size); + if (obj->value == NULL) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("cannot decode base64 secret value")); + goto cleanup; + } + } else { + if (driverConfig->secrets_encryption_key == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot decrypt secret value without encryption key")); + goto cleanup; + } + contents_encrypted = g_new0(uint8_t, st.st_size); + if (saferead(fd, contents_encrypted, st.st_size) != st.st_size) { + virReportSystemError(errno, _("cannot read '%1$s'"), + obj->base64File); + goto cleanup; + } + if ((st.st_size) < sizeof(iv)) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("Encrypted secret size is invalid")); + goto cleanup; + } + memcpy(iv, contents_encrypted, sizeof(iv)); + ciphertext = contents_encrypted + sizeof(iv); + ciphertextLen = st.st_size - sizeof(iv); + if (virCryptoDecryptData(VIR_CRYPTO_CIPHER_AES256CBC, + driverConfig->secrets_encryption_key, driverConfig->secretsKeyLen, + iv, sizeof(iv), + ciphertext, ciphertextLen, + &decryptedValue, &decryptedValueLen) < 0) { + virReportError(VIR_ERR_INVALID_SECRET, "%s", + _("Decryption of secret value failed")); + goto cleanup; + } + g_free(obj->value); + obj->value = g_steal_pointer(&decryptedValue); + obj->value_size = decryptedValueLen; } - contents[st.st_size] = '\0'; - - VIR_FORCE_CLOSE(fd); - - obj->value = g_base64_decode(contents, &obj->value_size); - ret = 0; cleanup: - if (contents != NULL) - memset(contents, 0, st.st_size); + if (contents != NULL) { + memset(contents, 0, st.st_size+1); + } + if (contents_encrypted != NULL) { + memset(contents_encrypted, 0, st.st_size); + } VIR_FORCE_CLOSE(fd); + virSecureErase(iv, sizeof(iv)); return ret; } @@ -868,7 +967,8 @@ static virSecretObj * virSecretLoad(virSecretObjList *secrets, const char *file, const char *path, - const char *configDir) + const char *configDir, + virSecretDaemonConfig *driverConfig) { g_autoptr(virSecretDef) def = NULL; virSecretObj *obj = NULL; @@ -882,7 +982,7 @@ virSecretLoad(virSecretObjList *secrets, if (!(obj = virSecretObjListAdd(secrets, &def, configDir, NULL))) return NULL; - if (virSecretLoadValue(obj) < 0) { + if (virSecretLoadValue(obj, driverConfig) < 0) { virSecretObjListRemove(secrets, obj); g_clear_pointer(&obj, virObjectUnref); return NULL; @@ -894,7 +994,8 @@ virSecretLoad(virSecretObjList *secrets, int virSecretLoadAllConfigs(virSecretObjList *secrets, - const char *configDir) + const char *configDir, + virSecretDaemonConfig *driverConfig) { g_autoptr(DIR) dir = NULL; struct dirent *de; @@ -915,7 +1016,7 @@ virSecretLoadAllConfigs(virSecretObjList *secrets, if (!(path = virFileBuildPath(configDir, de->d_name, NULL))) continue; - if (!(obj = virSecretLoad(secrets, de->d_name, path, configDir))) { + if (!(obj = virSecretLoad(secrets, de->d_name, path, configDir, driverConfig))) { VIR_ERROR(_("Error reading secret: %1$s"), virGetLastErrorMessage()); continue; diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 17897c5513..f49600a75c 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -23,6 +23,7 @@ #include "internal.h" #include "secret_conf.h" +#include "secret_config.h" typedef struct _virSecretObj virSecretObj; @@ -86,7 +87,8 @@ int virSecretObjSaveConfig(virSecretObj *obj); int -virSecretObjSaveData(virSecretObj *obj); +virSecretObjSaveData(virSecretObj *obj, + virSecretDaemonConfig *driverConfig); virSecretDef * virSecretObjGetDef(virSecretObj *obj); @@ -101,7 +103,8 @@ virSecretObjGetValue(virSecretObj *obj); int virSecretObjSetValue(virSecretObj *obj, const unsigned char *value, - size_t value_size); + size_t value_size, + virSecretDaemonConfig *driverConfig); size_t virSecretObjGetValueSize(virSecretObj *obj); @@ -112,4 +115,5 @@ virSecretObjSetValueSize(virSecretObj *obj, int virSecretLoadAllConfigs(virSecretObjList *secrets, - const char *configDir); + const char *configDir, + virSecretDaemonConfig *cfg); diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 04c3ca49f1..c0cac39a28 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -30,6 +30,7 @@ #include "virlog.h" #include "viralloc.h" #include "secret_conf.h" +#include "secret_config.h" #include "virsecretobj.h" #include "secret_driver.h" #include "virthread.h" @@ -42,6 +43,10 @@ #include "secret_event.h" #include "virutil.h" #include "virinhibitor.h" +#include "virfile.h" +#include "virrandom.h" +#include "vircrypto.h" +#include "virsecureerase.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -70,6 +75,8 @@ struct _virSecretDriverState { /* Immutable pointer, self-locking APIs */ virInhibitor *inhibitor; + + virSecretDaemonConfig *config; }; static virSecretDriverState *driver; @@ -224,7 +231,7 @@ secretDefineXML(virConnectPtr conn, if (!objDef->isephemeral) { if (backup && backup->isephemeral) { - if (virSecretObjSaveData(obj) < 0) + if (virSecretObjSaveData(obj, driver->config) < 0) goto restore_backup; } @@ -307,7 +314,6 @@ secretGetXMLDesc(virSecretPtr secret, return ret; } - static int secretSetValue(virSecretPtr secret, const unsigned char *value, @@ -327,8 +333,7 @@ secretSetValue(virSecretPtr secret, def = virSecretObjGetDef(obj); if (virSecretSetValueEnsureACL(secret->conn, def) < 0) goto cleanup; - - if (virSecretObjSetValue(obj, value, value_size) < 0) + if (virSecretObjSetValue(obj, value, value_size, driver->config) < 0) goto cleanup; event = virSecretEventValueChangedNew(def->uuid, @@ -454,6 +459,7 @@ secretStateCleanupLocked(void) VIR_FREE(driver->configDir); virObjectUnref(driver->secretEventState); + virObjectUnref(driver->config); virInhibitorFree(driver->inhibitor); if (driver->lockFD != -1) @@ -518,6 +524,8 @@ secretStateInitialize(bool privileged, driver->stateDir); goto error; } + if (!(driver->config = virSecretDaemonConfigNew(driver->privileged))) + goto error; driver->inhibitor = virInhibitorNew( VIR_INHIBITOR_WHAT_NONE, @@ -534,7 +542,7 @@ secretStateInitialize(bool privileged, if (!(driver->secrets = virSecretObjListNew())) goto error; - if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0) + if (virSecretLoadAllConfigs(driver->secrets, driver->configDir, driver->config) < 0) goto error; return VIR_DRV_STATE_INIT_COMPLETE; @@ -553,7 +561,10 @@ secretStateReload(void) if (!driver) return -1; - ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir)); + if (!(driver->config = virSecretDaemonConfigNew(driver->privileged))) + return -1; + + ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir, driver->config)); return 0; } -- 2.51.1
