Skip to content

Commit

Permalink
lr_gpg_check_signature: Forward PGP error messages from RPM
Browse files Browse the repository at this point in the history 
When debugging a test failure with RPM using internal OpenPGP+OpenSSL
implementation (that's a bug in RPM, not in librepo), I discovered
that librepo tests did not print error messages and that
lr_gpg_check_signature() did not forwarded an error message from RPM.

RPM before rpm-4.19.0-alpha2 did not provided provided any error
messages. That has changed with new functions pgpPrtParams2() and
pgpVerifySignature2().

This patch enhances librepo code to use the new RPM functions if
available and to propagate the RPM error messages via an already
existing GError argument.

This patch also enhances librepo tests to actually print the
unexpected error messages.

Both enhancements should help people to debug their failures.

Nonetheless, internal OpenPGP implementation in RPM does not set any
error messages and that will probably not change because RPM is going
to remove that implementation. On the other hand, Sequoia
implementation in RPM forwards the messages from Sequoia library. Yet
I was unbable to obtain any message. Sequoia promissed to improve
their error messaging, especially with a demise of SHA-1. So I believe
this librepo enhancement is useful.

Implementation details: I wrapped pgpPrtParams2() into a function
because it's called at multiple places. Contrary I did not wrap
pgpVerifySignature2() because it's called only at one place.

#281
  • Loading branch information
@ppisar @jrohel
ppisar authored and jrohel committed Oct 4, 2023
1 parent 27cb311 commit 363c7a5
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 18 deletions.
19 changes: 19 additions & 0 deletions CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,25 @@ IF (USE_GPGME)
FIND_PACKAGE(Gpgme REQUIRED)
ELSE (USE_GPGME)
PKG_CHECK_MODULES(RPM REQUIRED rpm>=4.18.0)

INCLUDE(CheckSymbolExists)
SET(CMAKE_REQUIRED_INCLUDES "${RPM_INCLUDE_DIRS}")
SET(CMAKE_REQUIRED_LIBRARIES "${RPM_LIBRARIES}")

# pgpPrtParams2 added after rpm 4.19.0-alpha
CHECK_SYMBOL_EXISTS(pgpPrtParams2 rpm/rpmpgp.h HAVE_PGPPRTPARAMS2)
IF (HAVE_PGPPRTPARAMS2)
ADD_DEFINITIONS(-DHAVE_PGPPRTPARAMS2)
ENDIF()

# pgpVerifySignature2 added after rpm 4.19.0-alpha
CHECK_SYMBOL_EXISTS(pgpVerifySignature2 rpm/rpmpgp.h HAVE_PGPVERIFYSIGNATURE2)
IF (HAVE_PGPVERIFYSIGNATURE2)
ADD_DEFINITIONS(-DHAVE_PGPVERIFYSIGNATURE2)
ENDIF()

UNSET(CMAKE_REQUIRED_INCLUDES)
UNSET(CMAKE_REQUIRED_LIBRARIES)
ENDIF (USE_GPGME)

IF (WITH_ZCHUNK)
Expand Down
66 changes: 52 additions & 14 deletions librepo/gpg_rpm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,13 +156,43 @@ write_memory_to_file(const char * path, const guint8 * buf, size_t len, GError *
return ret;
}

/* Invoke librpmio pgpPrtParams2(), log error and set GError with forwarded
* librpmio error, and return pgpPrtParams2() return code. */
static int
lr_pgpPrtParams2_with_gerror(const uint8_t *pkts, size_t pktlen, unsigned int pkttype,
pgpDigParams *ret, GError **err) {
int retval;
char *message = NULL;

retval =
#ifdef HAVE_PGPPRTPARAMS2
pgpPrtParams2(pkts, pktlen, pkttype, ret, &message);
#else
pgpPrtParams(pkts, pktlen, pkttype, ret);
#endif
if (retval == -1) {
if (message == NULL) {
g_debug("%s: Error during parsing OpenPGP packets", __func__);
g_set_error(err, LR_GPG_ERROR, LRE_GPGERROR,
"Error during parsing OpenPGP packets");
} else {
g_debug("%s: Error during parsing OpenPGP packets: %s", __func__, message);
g_set_error(err, LR_GPG_ERROR, LRE_GPGERROR,
"Error during parsing OpenPGP packets: %s", message);
}
}
/* The message can be set on success. */
if (message != NULL)
free(message);

return retval;
}

// Searches for a key with `keyid` in the OpenPGP packet.
static gboolean
search_key_id(const guint8 * keyid, gchar * buf, size_t len, pgpDigParams * dig_params, GError **err) {
pgpDigParams main_dig_params = NULL;
if (pgpPrtParams((const uint8_t *)buf, len, PGPTAG_PUBLIC_KEY, &main_dig_params) == -1) {
g_debug("%s: Error: Parsing a OpenPGP packet(s) failed", __func__);
g_set_error(err, LR_GPG_ERROR, LRE_GPGERROR, "Parsing a OpenPGP packet(s) failed");
if (lr_pgpPrtParams2_with_gerror((const uint8_t *)buf, len, PGPTAG_PUBLIC_KEY, &main_dig_params, err) == -1) {
return FALSE;
}
if (main_dig_params == NULL) {
Expand Down Expand Up @@ -268,9 +298,7 @@ import_raw_key_from_memory(const guint8 *key, size_t key_len, const char *home_d
}
do {
pgpDigParams main_dig_params = NULL;
if (pgpPrtParams(key, cert_len, PGPTAG_PUBLIC_KEY, &main_dig_params) == -1) {
g_debug("%s: Error: Parsing a OpenPGP packet(s) failed", __func__);
g_set_error(err, LR_GPG_ERROR, LRE_GPGERROR, "Parsing a OpenPGP packet(s) failed");
if (lr_pgpPrtParams2_with_gerror(key, cert_len, PGPTAG_PUBLIC_KEY, &main_dig_params, err) == -1) {
return FALSE;
}
const guint8 * keyid = pgpDigParamsSignID(main_dig_params);
Expand Down Expand Up @@ -396,8 +424,7 @@ lr_gpg_list_keys(gboolean export_keys, const char *home_dir, GError **err)
}

pgpDigParams main_dig_params = NULL;
if (pgpPrtParams((const uint8_t *)buf, len, PGPTAG_PUBLIC_KEY, &main_dig_params) == -1) {
g_debug("%s: Error: Parsing a OpenPGP packet(s) failed", __func__);
if (lr_pgpPrtParams2_with_gerror((const uint8_t *)buf, len, PGPTAG_PUBLIC_KEY, &main_dig_params, NULL) == -1) {
continue;
}

Expand Down Expand Up @@ -518,9 +545,7 @@ check_signature(const gchar * sig_buf, ssize_t sig_buf_len, const gchar * data,
}

pgpDigParams signature_dig_params = NULL;
if (pgpPrtParams(pkts, pkts_len, PGPTAG_SIGNATURE, &signature_dig_params) == -1) {
g_debug("%s: Error during parsing OpenPGP packet(s)", __func__);
g_set_error(err, LR_GPG_ERROR, LRE_GPGERROR, "%s: Error during parsing OpenPGP packet(s)", __func__);
if (lr_pgpPrtParams2_with_gerror(pkts, pkts_len, PGPTAG_SIGNATURE, &signature_dig_params, err) == -1) {
free(pkts);
return FALSE;
}
Expand Down Expand Up @@ -549,16 +574,29 @@ check_signature(const gchar * sig_buf, ssize_t sig_buf_len, const gchar * data,

const unsigned int hash_algo = pgpDigParamsAlgo(signature_dig_params, PGPVAL_HASHALGO);
DIGEST_CTX hashctx = rpmDigestInit(hash_algo, RPMDIGEST_NONE);
char *message = NULL;
rpmDigestUpdate(hashctx, data, data_len);
rpmRC ret_verify = pgpVerifySignature(signing_key_dig_params, signature_dig_params, hashctx);
rpmRC ret_verify =
#ifdef HAVE_PGPVERIFYSIGNATURE2
pgpVerifySignature2(signing_key_dig_params, signature_dig_params, hashctx, &message);
#else
pgpVerifySignature(signing_key_dig_params, signature_dig_params, hashctx);
#endif
rpmDigestFinal(hashctx, NULL, NULL, 0);
pgpDigParamsFree(signing_key_dig_params);

ret = ret_verify == RPMRC_OK || ret_verify == RPMRC_NOTTRUSTED;
if (!ret) {
g_debug("%s: Bad GPG signature", __func__);
g_set_error(err, LR_GPG_ERROR, LRE_BADGPG, "Bad GPG signature");
if (message == NULL) {
g_debug("%s: Bad PGP signature", __func__);
g_set_error(err, LR_GPG_ERROR, LRE_BADGPG, "Bad PGP signature");
} else {
g_debug("%s: Bad PGP signature: %s", __func__, message);
g_set_error(err, LR_GPG_ERROR, LRE_BADGPG, "Bad PGP signature: %s", message);
}
}
if (message != NULL)
free(message);
} else {
g_debug("%s: Signing key not found", __func__);
g_set_error(err, LR_GPG_ERROR, LRE_BADGPG, "Signing key not found");
Expand Down
12 changes: 8 additions & 4 deletions tests/test_gpg.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,10 @@ START_TEST(test_gpg_check_signature)
data_path,
tmp_home_path,
&tmp_err);
ck_assert(ret);
ck_assert_ptr_null(tmp_err);
ck_assert_msg(ret, "Checking valid key and data from file failed with \"%s\"",
(tmp_err && tmp_err->message) ? tmp_err->message : "");
ck_assert_msg(NULL == tmp_err, "Checking valid key and data from file passed but set error \"%s\"",
(tmp_err && tmp_err->message) ? tmp_err->message : "");

// Bad signature signed with unknown key
ret = lr_gpg_check_signature(_signature_path,
Expand Down Expand Up @@ -86,8 +88,10 @@ START_TEST(test_gpg_check_signature)
_data_path,
tmp_home_path,
&tmp_err);
ck_assert(ret);
ck_assert_ptr_null(tmp_err);
ck_assert_msg(ret, "Checking valid key and data from memory failed with \"%s\"",
(tmp_err && tmp_err->message) ? tmp_err->message : "");
ck_assert_msg(NULL == tmp_err, "Checking valid key and data from memory passed but set error \"%s\"",
(tmp_err && tmp_err->message) ? tmp_err->message : "");

// Bad signature signed with known key
ret = lr_gpg_check_signature(_signature_path,
Expand Down

0 comments on commit 363c7a5

Please sign in to comment.