Consistent error reporting for encryption/decryption in pgcrypto
Commit b918bf86c65 added the errorcode PXE_DECRYPT_FAILED to the existing set
of PXE_ error codes. When pgcrypto was changed to the EVP APIs in 5ff4a67f63,
no new error codes were added in favour of existing ones. This results in
encryption failures returning PXE_ERR_GENERIC, which seems a bit inconsistent.
The attached introduce PXE_ENCRYPT_FAILED and use that for EVP_EncryptUpdate to
ideally be slightly clearer in case of errors. Any reason not to do that
instead of using ERR_GENERIC?
cheers ./daniel
Attachments:
0001-Use-a-more-descriptive-error-for-failed-encryption.patchapplication/octet-stream; name=0001-Use-a-more-descriptive-error-for-failed-encryption.patch; x-unix-mode=0644Download
From 3d58f927eb9763c9803ce41ff6b1442289b65b8e Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Thu, 29 Oct 2020 22:04:46 +0100
Subject: [PATCH] Use a more descriptive error for failed encryption
We already return PXE_DECRYPT_FAILED for decryption errors, so using
PXE_ERR_GENERIC for encryption errors isn't really consistent or for
that matter helpful.
---
contrib/pgcrypto/openssl.c | 2 +-
contrib/pgcrypto/px.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/contrib/pgcrypto/openssl.c b/contrib/pgcrypto/openssl.c
index ed96e4ce53..5ebe213406 100644
--- a/contrib/pgcrypto/openssl.c
+++ b/contrib/pgcrypto/openssl.c
@@ -400,7 +400,7 @@ gen_ossl_encrypt(PX_Cipher *c, const uint8 *data, unsigned dlen,
}
if (!EVP_EncryptUpdate(od->evp_ctx, res, &outlen, data, dlen))
- return PXE_ERR_GENERIC;
+ return PXE_ENCRYPT_FAILED;
return 0;
}
diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
index 6a4681dae9..a243f575d3 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -58,6 +58,7 @@ static const struct error_desc px_err_list[] = {
{PXE_MCRYPT_INTERNAL, "mcrypt internal error"},
{PXE_NO_RANDOM, "Failed to generate strong random bits"},
{PXE_DECRYPT_FAILED, "Decryption failed"},
+ {PXE_ENCRYPT_FAILED, "Encryption failed"},
{PXE_PGP_CORRUPT_DATA, "Wrong key or corrupt data"},
{PXE_PGP_CORRUPT_ARMOR, "Corrupt ascii-armor"},
{PXE_PGP_UNSUPPORTED_COMPR, "Unsupported compression algorithm"},
--
2.21.1 (Apple Git-122.3)
On Thu, Oct 29, 2020 at 10:26:54PM +0100, Daniel Gustafsson wrote:
The attached introduce PXE_ENCRYPT_FAILED and use that for EVP_EncryptUpdate to
ideally be slightly clearer in case of errors. Any reason not to do that
instead of using ERR_GENERIC?
+1. While looking at that, I was wondering of the potential need of
this error code for other encryption code paths, but it happens that
this is only specific to OpenSSL. Rijndael or Blowfish don't need
it.
--
Michael
Hi,
thank you for your contribution.
I did notice that the cfbot [1]http://cfbot.cputube.org/daniel-gustafsson.html is not failing for this patch.
Cheers,
//Georgios
On 30 Oct 2020, at 16:54, Georgios Kokolatos <gkokolatos@protonmail.com> wrote:
I did notice that the cfbot [1] is not failing for this patch.
I assume you mean s/failing/passing/? I noticed the red Travis and Appveyor
runs, will fix over the weekend. Thanks for the heads-up.
cheers ./daniel
On Fri, Oct 30, 2020 at 11:23:27PM +0100, Daniel Gustafsson wrote:
On 30 Oct 2020, at 16:54, Georgios Kokolatos <gkokolatos@protonmail.com> wrote:
I did notice that the cfbot [1] is not failing for this patch.
I assume you mean s/failing/passing/? I noticed the red Travis and Appveyor
runs, will fix over the weekend. Thanks for the heads-up.
It seems to me that you are just missing to declare a new error number
in px.h, so I would suggest to just use -19.
--
Michael
On 31 Oct 2020, at 02:03, Michael Paquier <michael@paquier.xyz> wrote:
It seems to me that you are just missing to declare a new error number
in px.h, so I would suggest to just use -19.
Ah yes, I accidentally fat-fingered the git add -p when splitting up the NSS
patch into bite size pieces. Sorry about that. The attached v2 has the error
declaration.
cheers ./daniel
Attachments:
v2-0001-Use-a-more-descriptive-error-for-failed-encryptio.patchapplication/octet-stream; name=v2-0001-Use-a-more-descriptive-error-for-failed-encryptio.patch; x-unix-mode=0644Download
From 93b47477c9ca3c818e3706fec01616476d70a40c Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Thu, 29 Oct 2020 22:04:46 +0100
Subject: [PATCH v2] Use a more descriptive error for failed encryption
We already return PXE_DECRYPT_FAILED for decryption errors, so using
PXE_ERR_GENERIC for encryption errors isn't really consistent or for
that matter helpful.
---
contrib/pgcrypto/openssl.c | 2 +-
contrib/pgcrypto/px.c | 1 +
contrib/pgcrypto/px.h | 1 +
3 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/contrib/pgcrypto/openssl.c b/contrib/pgcrypto/openssl.c
index ed96e4ce53..5ebe213406 100644
--- a/contrib/pgcrypto/openssl.c
+++ b/contrib/pgcrypto/openssl.c
@@ -400,7 +400,7 @@ gen_ossl_encrypt(PX_Cipher *c, const uint8 *data, unsigned dlen,
}
if (!EVP_EncryptUpdate(od->evp_ctx, res, &outlen, data, dlen))
- return PXE_ERR_GENERIC;
+ return PXE_ENCRYPT_FAILED;
return 0;
}
diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
index 6a4681dae9..a243f575d3 100644
--- a/contrib/pgcrypto/px.c
+++ b/contrib/pgcrypto/px.c
@@ -58,6 +58,7 @@ static const struct error_desc px_err_list[] = {
{PXE_MCRYPT_INTERNAL, "mcrypt internal error"},
{PXE_NO_RANDOM, "Failed to generate strong random bits"},
{PXE_DECRYPT_FAILED, "Decryption failed"},
+ {PXE_ENCRYPT_FAILED, "Encryption failed"},
{PXE_PGP_CORRUPT_DATA, "Wrong key or corrupt data"},
{PXE_PGP_CORRUPT_ARMOR, "Corrupt ascii-armor"},
{PXE_PGP_UNSUPPORTED_COMPR, "Unsupported compression algorithm"},
diff --git a/contrib/pgcrypto/px.h b/contrib/pgcrypto/px.h
index 5487923edb..17d6f22498 100644
--- a/contrib/pgcrypto/px.h
+++ b/contrib/pgcrypto/px.h
@@ -61,6 +61,7 @@
#define PXE_MCRYPT_INTERNAL -16
#define PXE_NO_RANDOM -17
#define PXE_DECRYPT_FAILED -18
+#define PXE_ENCRYPT_FAILED -19
#define PXE_PGP_CORRUPT_DATA -100
#define PXE_PGP_CORRUPT_ARMOR -101
--
2.21.1 (Apple Git-122.3)
On Sat, Oct 31, 2020 at 09:40:12PM +0100, Daniel Gustafsson wrote:
Ah yes, I accidentally fat-fingered the git add -p when splitting up the NSS
patch into bite size pieces. Sorry about that. The attached v2 has the error
declaration.
Thanks for updatng the patch. Applied.
--
Michael