Skip to content

Commit 5a93c35

Browse files
artemgavrilovartemgavrilov
committed
PG-1661 Validate key coming from key providers
Check that key that we retreived from key provider is valid.
1 parent 38961e0 commit 5a93c35

File tree

10 files changed

+236
-31
lines changed

10 files changed

+236
-31
lines changed

‎contrib/pg_tde/expected/key_provider.out

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ WARNING: The WAL encryption feature is currently in beta and may be unstable. D
252252
(1 row)
253253

254254
SELECT pg_tde_change_global_key_provider_file('global-provider','/tmp/global-provider-file-2');
255-
ERROR: could not fetch key "server-key" used as server key from modified key provider "global-provider": 0
255+
ERROR: key "server-key" used as server key not found in modified key provider "global-provider"
256256
-- Modifying key providers fails if new settings can't fetch existing database key
257257
SELECT pg_tde_add_global_key_provider_file('global-provider2', '/tmp/global-provider-file-1');
258258
pg_tde_add_global_key_provider_file
@@ -279,7 +279,7 @@ SELECT pg_tde_set_key_using_global_key_provider('database-key', 'global-provider
279279

280280
\c :regress_database
281281
SELECT pg_tde_change_global_key_provider_file('global-provider2', '/tmp/global-provider-file-2');
282-
ERROR: could not fetch key "database-key" used by database "db_using_global_provider" from modified key provider "global-provider2": 0
282+
ERROR: key "database-key" used by database "db_using_global_provider" not found in key provider "global-provider2"
283283
DROP DATABASE db_using_global_provider;
284284
CREATE DATABASE db_using_database_provider;
285285
\c db_using_database_provider;
@@ -303,7 +303,7 @@ SELECT pg_tde_set_key_using_database_key_provider('database-key', 'db-provider')
303303
(1 row)
304304

305305
SELECT pg_tde_change_database_key_provider_file('db-provider', '/tmp/db-provider-file-2');
306-
ERROR: could not fetch key "database-key" used by database "db_using_database_provider" from modified key provider "db-provider": 0
306+
ERROR: key "database-key" used by database "db_using_database_provider" not found in key provider "db-provider"
307307
\c :regress_database
308308
DROP DATABASE db_using_database_provider;
309309
-- Deleting key providers fails if key name is NULL

‎contrib/pg_tde/src/catalog/tde_principal_key.c

Lines changed: 52 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,7 @@ set_principal_key_with_keyring(const char *key_name,
225225
LWLock *lock_files = tde_lwlock_enc_keys();
226226
bool already_has_key;
227227
GenericKeyring *new_keyring;
228-
const KeyInfo *keyInfo = NULL;
228+
KeyInfo *keyInfo = NULL;
229229

230230
/*
231231
* Try to get principal key from cache.
@@ -238,14 +238,15 @@ set_principal_key_with_keyring(const char *key_name,
238238
new_keyring = GetKeyProviderByName(provider_name, providerOid);
239239

240240
{
241-
KeyringReturnCode kr_ret;
241+
KeyringReturnCode return_code;
242242

243-
keyInfo = KeyringGetKey(new_keyring, key_name, &kr_ret);
243+
keyInfo = KeyringGetKey(new_keyring, key_name, &return_code);
244244

245-
if (kr_ret != KEYRING_CODE_SUCCESS)
245+
if (return_code != KEYRING_CODE_SUCCESS)
246246
{
247247
ereport(ERROR,
248-
errmsg("could not successfully query key provider \"%s\"", new_keyring->provider_name));
248+
errmsg("failed to retrieve principal key \"%s\" from key provider \"%s\"", key_name, new_keyring->provider_name),
249+
errdetail("%s", KeyringErrorCodeToString(return_code)));
249250
}
250251
}
251252

@@ -289,6 +290,7 @@ set_principal_key_with_keyring(const char *key_name,
289290

290291
LWLockRelease(lock_files);
291292

293+
pfree(keyInfo);
292294
pfree(new_keyring);
293295
pfree(new_principal_key);
294296
}
@@ -303,7 +305,7 @@ xl_tde_perform_rotate_key(XLogPrincipalKeyRotate *xlrec)
303305
TDEPrincipalKey *new_principal_key;
304306
GenericKeyring *new_keyring;
305307
KeyInfo *keyInfo;
306-
KeyringReturnCode kr_ret;
308+
KeyringReturnCode return_code;
307309

308310
LWLockAcquire(tde_lwlock_enc_keys(), LW_EXCLUSIVE);
309311

@@ -316,19 +318,20 @@ xl_tde_perform_rotate_key(XLogPrincipalKeyRotate *xlrec)
316318
}
317319

318320
new_keyring = GetKeyProviderByID(xlrec->keyringId, xlrec->databaseId);
319-
keyInfo = KeyringGetKey(new_keyring, xlrec->keyName, &kr_ret);
321+
keyInfo = KeyringGetKey(new_keyring, xlrec->keyName, &return_code);
320322

321-
if (kr_ret != KEYRING_CODE_SUCCESS)
323+
if (return_code != KEYRING_CODE_SUCCESS)
322324
{
323325
ereport(ERROR,
324-
errmsg("failed to retrieve principal key from keyring provider: \"%s\"", new_keyring->provider_name),
325-
errdetail("Error code: %d", kr_ret));
326+
errmsg("failed to retrieve principal key \"%s\" from key provider \"%s\"", xlrec->keyName, new_keyring->provider_name),
327+
errdetail("%s", KeyringErrorCodeToString(return_code)));
326328
}
327329

328330
/* The new key should be on keyring by this time */
329331
if (keyInfo == NULL)
330332
{
331-
ereport(ERROR, errmsg("failed to retrieve principal key from keyring for database %u.", xlrec->databaseId));
333+
ereport(ERROR, errmsg("failed to retrieve principal key \"%s\" from key provider \"%s\" for database %u",
334+
xlrec->keyName, new_keyring->provider_name, xlrec->databaseId));
332335
}
333336

334337
new_principal_key = palloc_object(TDEPrincipalKey);
@@ -347,6 +350,7 @@ xl_tde_perform_rotate_key(XLogPrincipalKeyRotate *xlrec)
347350

348351
LWLockRelease(tde_lwlock_enc_keys());
349352

353+
pfree(keyInfo);
350354
pfree(new_keyring);
351355
pfree(new_principal_key);
352356
}
@@ -514,7 +518,8 @@ pg_tde_create_principal_key_internal(Oid providerOid,
514518

515519
if (return_code != KEYRING_CODE_SUCCESS)
516520
ereport(ERROR,
517-
errmsg("could not successfully query key provider \"%s\"", provider->provider_name));
521+
errmsg("failed to retrieve principal key \"%s\" from key provider \"%s\"", key_name, provider_name),
522+
errdetail("%s", KeyringErrorCodeToString(return_code)));
518523

519524
if (key_info != NULL)
520525
ereport(ERROR,
@@ -939,11 +944,17 @@ get_principal_key_from_keyring(Oid dbOid)
939944
principalKeyInfo->data.name, principalKeyInfo->data.keyringId));
940945

941946
keyInfo = KeyringGetKey(keyring, principalKeyInfo->data.name, &keyring_ret);
947+
948+
if (keyring_ret != KEYRING_CODE_SUCCESS)
949+
ereport(ERROR,
950+
errmsg("failed to retrieve principal key \"%s\" from key provider \"%s\"", principalKeyInfo->data.name, keyring->provider_name),
951+
errdetail("%s", KeyringErrorCodeToString(keyring_ret)));
952+
942953
if (keyInfo == NULL)
943954
ereport(ERROR,
944955
errcode(ERRCODE_NO_DATA_FOUND),
945-
errmsg("failed to retrieve principal key %s from keyring with ID %d",
946-
principalKeyInfo->data.name, principalKeyInfo->data.keyringId));
956+
errmsg("key \"%s\" not found in key provider \"%s\"",
957+
principalKeyInfo->data.name, keyring->provider_name));
947958

948959
if (!pg_tde_verify_principal_key_info(principalKeyInfo, &keyInfo->data))
949960
ereport(ERROR,
@@ -1184,11 +1195,21 @@ pg_tde_verify_provider_keys_in_use(GenericKeyring *modified_provider)
11841195
KeyInfo *proposed_key;
11851196

11861197
proposed_key = KeyringGetKey(modified_provider, key_name, &return_code);
1198+
1199+
if (return_code != KEYRING_CODE_SUCCESS)
1200+
{
1201+
ereport(ERROR,
1202+
errmsg("failed to retreive \"%s\" key used as server key from modified key provider \"%s\"",
1203+
key_name, modified_provider->provider_name),
1204+
errdetail("%s", KeyringErrorCodeToString(return_code)));
1205+
}
1206+
11871207
if (!proposed_key)
11881208
{
11891209
ereport(ERROR,
1190-
errmsg("could not fetch key \"%s\" used as server key from modified key provider \"%s\": %d",
1191-
key_name, modified_provider->provider_name, return_code));
1210+
errcode(ERRCODE_NO_DATA_FOUND),
1211+
errmsg("key \"%s\" used as server key not found in modified key provider \"%s\"",
1212+
key_name, modified_provider->provider_name));
11921213
}
11931214

11941215
if (!pg_tde_verify_principal_key_info(existing_principal_key, &proposed_key->data))
@@ -1197,6 +1218,8 @@ pg_tde_verify_provider_keys_in_use(GenericKeyring *modified_provider)
11971218
errmsg("key \"%s\" from modified key provider \"%s\" does not match existing server key",
11981219
key_name, modified_provider->provider_name));
11991220
}
1221+
1222+
pfree(proposed_key);
12001223
}
12011224

12021225
if (existing_principal_key)
@@ -1219,11 +1242,20 @@ pg_tde_verify_provider_keys_in_use(GenericKeyring *modified_provider)
12191242
KeyInfo *proposed_key;
12201243

12211244
proposed_key = KeyringGetKey(modified_provider, key_name, &return_code);
1245+
if (return_code != KEYRING_CODE_SUCCESS)
1246+
{
1247+
ereport(ERROR,
1248+
errmsg("failed to retreive \"%s\" key used by database \"%s\" from key provider \"%s\"",
1249+
key_name, database->datname.data, modified_provider->provider_name),
1250+
errdetail("%s", KeyringErrorCodeToString(return_code)));
1251+
}
1252+
12221253
if (!proposed_key)
12231254
{
12241255
ereport(ERROR,
1225-
errmsg("could not fetch key \"%s\" used by database \"%s\" from modified key provider \"%s\": %d",
1226-
key_name, database->datname.data, modified_provider->provider_name, return_code));
1256+
errcode(ERRCODE_NO_DATA_FOUND),
1257+
errmsg("key \"%s\" used by database \"%s\" not found in key provider \"%s\"",
1258+
key_name, database->datname.data, modified_provider->provider_name));
12271259
}
12281260

12291261
if (!pg_tde_verify_principal_key_info(existing_principal_key, &proposed_key->data))
@@ -1232,6 +1264,8 @@ pg_tde_verify_provider_keys_in_use(GenericKeyring *modified_provider)
12321264
errmsg("key \"%s\" from modified key provider \"%s\" does not match existing key used by database \"%s\"",
12331265
key_name, modified_provider->provider_name, database->datname.data));
12341266
}
1267+
1268+
pfree(proposed_key);
12351269
}
12361270

12371271
if (existing_principal_key)

‎contrib/pg_tde/src/include/keyring/keyring_api.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,9 @@ typedef enum ProviderType
1414
} ProviderType;
1515

1616
#define TDE_KEY_NAME_LEN 256
17-
#define MAX_KEY_DATA_SIZE 32 /* maximum 256 bit encryption */
17+
#define KEY_DATA_SIZE_128 16 /* 128 bit encryption */
18+
#define KEY_DATA_SIZE_256 32 /* 256 bit encryption, not yet supported */
19+
#define MAX_KEY_DATA_SIZE KEY_DATA_SIZE_256 /* maximum 256 bit encryption */
1820
#define INTERNAL_KEY_LEN 16
1921

2022
typedef struct KeyData
@@ -35,7 +37,7 @@ typedef enum KeyringReturnCode
3537
KEYRING_CODE_INVALID_PROVIDER = 1,
3638
KEYRING_CODE_RESOURCE_NOT_AVAILABLE = 2,
3739
KEYRING_CODE_INVALID_RESPONSE = 5,
38-
KEYRING_CODE_INVALID_KEY_SIZE = 6,
40+
KEYRING_CODE_INVALID_KEY = 6,
3941
KEYRING_CODE_DATA_CORRUPTED = 7,
4042
} KeyringReturnCode;
4143

@@ -87,5 +89,7 @@ extern void RegisterKeyProviderType(const TDEKeyringRoutine *routine, ProviderTy
8789
extern KeyInfo *KeyringGetKey(GenericKeyring *keyring, const char *key_name, KeyringReturnCode *returnCode);
8890
extern KeyInfo *KeyringGenerateNewKeyAndStore(GenericKeyring *keyring, const char *key_name, unsigned key_len);
8991
extern void KeyringValidate(GenericKeyring *keyring);
92+
extern bool ValidateKey(KeyInfo *key);
93+
extern char *KeyringErrorCodeToString(KeyringReturnCode code);
9094

9195
#endif /* KEYRING_API_H */

‎contrib/pg_tde/src/keyring/keyring_api.c

Lines changed: 65 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,16 +100,57 @@ RegisterKeyProviderType(const TDEKeyringRoutine *routine, ProviderType type)
100100
KeyInfo *
101101
KeyringGetKey(GenericKeyring *keyring, const char *key_name, KeyringReturnCode *returnCode)
102102
{
103+
KeyInfo *key = NULL;
103104
RegisteredKeyProviderType *kp = find_key_provider_type(keyring->type);
104105

105106
if (kp == NULL)
106107
{
107108
ereport(WARNING,
108-
errmsg("Key provider of type %d not registered", keyring->type));
109+
errmsg("key provider of type %d not registered", keyring->type));
109110
*returnCode = KEYRING_CODE_INVALID_PROVIDER;
110111
return NULL;
111112
}
112-
return kp->routine->keyring_get_key(keyring, key_name, returnCode);
113+
key = kp->routine->keyring_get_key(keyring, key_name, returnCode);
114+
115+
if (*returnCode != KEYRING_CODE_SUCCESS || key == NULL)
116+
return NULL;
117+
118+
if (!ValidateKey(key))
119+
{
120+
*returnCode = KEYRING_CODE_INVALID_KEY;
121+
pfree(key);
122+
return NULL;
123+
}
124+
125+
return key;
126+
}
127+
128+
bool
129+
ValidateKey(KeyInfo *key)
130+
{
131+
Assert(key != NULL);
132+
133+
if (key->name[0] == '\0')
134+
{
135+
ereport(WARNING, errmsg("invalid key: name is empty"));
136+
return false;
137+
}
138+
139+
if (key->data.len == 0)
140+
{
141+
ereport(WARNING, errmsg("invalid key: data length is zero"));
142+
return false;
143+
}
144+
145+
/* For now we only support 128-bit keys */
146+
if (key->data.len != KEY_DATA_SIZE_128)
147+
{
148+
ereport(WARNING,
149+
errmsg("invalid key: unsupported key length \"%u\"", key->data.len));
150+
return false;
151+
}
152+
153+
return true;
113154
}
114155

115156
static void
@@ -163,3 +204,25 @@ KeyringValidate(GenericKeyring *keyring)
163204

164205
kp->routine->keyring_validate(keyring);
165206
}
207+
208+
char *
209+
KeyringErrorCodeToString(KeyringReturnCode code)
210+
{
211+
switch (code)
212+
{
213+
case KEYRING_CODE_SUCCESS:
214+
return "Success";
215+
case KEYRING_CODE_INVALID_PROVIDER:
216+
return "Invalid key";
217+
case KEYRING_CODE_RESOURCE_NOT_AVAILABLE:
218+
return "Resource not available";
219+
case KEYRING_CODE_INVALID_RESPONSE:
220+
return "Invalid response from keyring provider";
221+
case KEYRING_CODE_INVALID_KEY:
222+
return "Invalid key";
223+
case KEYRING_CODE_DATA_CORRUPTED:
224+
return "Data corrupted";
225+
default:
226+
return "Unknown error code";
227+
}
228+
}

‎contrib/pg_tde/src/keyring/keyring_kmip.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode
178178
if (key->data.len > sizeof(key->data.data))
179179
{
180180
ereport(WARNING, errmsg("keyring provider returned invalid key size: %d", key->data.len));
181-
*return_code = KEYRING_CODE_INVALID_KEY_SIZE;
181+
*return_code = KEYRING_CODE_INVALID_KEY;
182182
pfree(key);
183183
BIO_free_all(ctx.bio);
184184
SSL_CTX_free(ctx.ssl);

‎contrib/pg_tde/src/keyring/keyring_vault.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode
220220

221221
if (!curl_perform(vault_keyring, url, &str, &httpCode, NULL))
222222
{
223-
*return_code = KEYRING_CODE_INVALID_KEY_SIZE;
223+
*return_code = KEYRING_CODE_INVALID_KEY;
224224
ereport(WARNING,
225225
errmsg("HTTP(S) request to keyring provider \"%s\" failed",
226226
vault_keyring->keyring.provider_name));
@@ -266,7 +266,7 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode
266266

267267
if (key->data.len > MAX_KEY_DATA_SIZE)
268268
{
269-
*return_code = KEYRING_CODE_INVALID_KEY_SIZE;
269+
*return_code = KEYRING_CODE_INVALID_KEY;
270270
ereport(WARNING,
271271
errmsg("keyring provider \"%s\" returned invalid key size: %d",
272272
vault_keyring->keyring.provider_name, key->data.len));

‎contrib/pg_tde/t/expected/basic.out

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,6 @@ SELECT * FROM test_enc ORDER BY id;
6464
TABLEFILE FOUND: yes
6565
CONTAINS FOO (should be empty):
6666
SELECT pg_tde_verify_key()
67-
psql:<stdin>:1: ERROR: failed to retrieve principal key test-db-key from keyring with ID 1
67+
psql:<stdin>:1: ERROR: key "test-db-key" not found in key provider "file-vault"
6868
DROP TABLE test_enc;
6969
DROP EXTENSION pg_tde;

‎contrib/pg_tde/t/expected/change_key_provider.out

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,15 +99,15 @@ SELECT * FROM test_enc ORDER BY id;
9999
-- mv /tmp/change_key_provider_2.per /tmp/change_key_provider_1.per
100100
-- server restart
101101
SELECT pg_tde_verify_key();
102-
psql:<stdin>:1: ERROR: failed to retrieve principal key test-key from keyring with ID 1
102+
psql:<stdin>:1: ERROR: key "test-key" not found in key provider "file-vault"
103103
SELECT pg_tde_is_encrypted('test_enc');
104104
pg_tde_is_encrypted
105105
---------------------
106106
t
107107
(1 row)
108108

109109
SELECT * FROM test_enc ORDER BY id;
110-
psql:<stdin>:1: ERROR: failed to retrieve principal key test-key from keyring with ID 1
110+
psql:<stdin>:1: ERROR: key "test-key" not found in key provider "file-vault"
111111
SELECT pg_tde_change_database_key_provider_file('file-vault', '/tmp/change_key_provider_1.per');
112112
pg_tde_change_database_key_provider_file
113113
------------------------------------------

‎contrib/pg_tde/t/expected/key_validation.out

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
CREATE EXTENSION pg_tde;
2+
SELECT pg_tde_add_database_key_provider_file('test-file-provider', '/tmp/pg_tde_test_key_validation.per');
3+
pg_tde_add_database_key_provider_file
4+
---------------------------------------
5+
6+
(1 row)
7+
8+
SELECT pg_tde_create_key_using_database_key_provider('key1', 'test-file-provider');
9+
pg_tde_create_key_using_database_key_provider
10+
-----------------------------------------------
11+
12+
(1 row)
13+
14+
SELECT pg_tde_create_key_using_database_key_provider('key2', 'test-file-provider');
15+
pg_tde_create_key_using_database_key_provider
16+
-----------------------------------------------
17+
18+
(1 row)
19+
20+
SELECT pg_tde_set_key_using_database_key_provider('key1', 'test-file-provider');
21+
psql:<stdin>:1: WARNING: invalid key: data length is zero
22+
psql:<stdin>:1: ERROR: failed to retrieve principal key "key1" from key provider "test-file-provider"
23+
DETAIL: Invalid key
24+
SELECT pg_tde_set_key_using_database_key_provider('key2', 'test-file-provider');
25+
psql:<stdin>:1: WARNING: invalid key: unsupported key length "4294967295"
26+
psql:<stdin>:1: ERROR: failed to retrieve principal key "key2" from key provider "test-file-provider"
27+
DETAIL: Invalid key

0 commit comments

Comments
 (0)