Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement openpgp.cert.d based keystore #3437

Merged
merged 2 commits into from
Nov 12, 2024
Merged

Implement openpgp.cert.d based keystore #3437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ee40a5
Refactor s backend into shared helper functions
ffesti Oct 31, 2024
427e5f9
Implement openpgp.cert.d based keystore
ffesti Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
180 changes: 143 additions & 37 deletions lib/keystore.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
#include <string>

#include <fcntl.h>
#include <unistd.h>
#include <sys/file.h>

#include <rpm/header.h>
#include <rpm/rpmbase64.h>
Expand All @@ -24,11 +26,11 @@ using namespace rpm;

static int makePubkeyHeader(rpmts ts, rpmPubkey key, Header * hdrp);

rpmRC keystore_fs::load_keys(rpmtxn txn, rpmKeyring keyring)
static rpmRC load_keys_from_glob(rpmtxn txn, rpmKeyring keyring, string glob)
{
ARGV_t files = NULL;
/* XXX TODO: deal with chroot path issues */
char *pkpath = rpmGetPath(rpmtxnRootDir(txn), "%{_keyringpath}/*.key", NULL);
char *pkpath = rpmGetPath(rpmtxnRootDir(txn), glob.c_str(), NULL);

rpmlog(RPMLOG_DEBUG, "loading keyring from pubkeys in %s\n", pkpath);
if (rpmGlob(pkpath, NULL, &files)) {
Expand All @@ -55,6 +57,53 @@ rpmRC keystore_fs::load_keys(rpmtxn txn, rpmKeyring keyring)
return RPMRC_OK;
}

static rpmRC write_key_to_disk(rpmPubkey key, string & dir, string & filename, int replace, rpmFlags flags)
{
rpmRC rc = RPMRC_FAIL;
char *keyval = rpmPubkeyArmorWrap(key);
string tmppath = "";
string path = dir + "/" + filename;

if (replace) {
tmppath = dir + "/" + filename + ".new";
unlink(tmppath.c_str());
}

if (rpmMkdirs(NULL, dir.c_str())) {
rpmlog(RPMLOG_ERR, _("failed to create keyring directory %s: %s\n"),
path.c_str(), strerror(errno));
goto exit;
}

if (FD_t fd = Fopen(replace ? tmppath.c_str() : path.c_str(), "wx")) {
size_t keylen = strlen(keyval);
if (Fwrite(keyval, 1, keylen, fd) == keylen)
rc = RPMRC_OK;
Fclose(fd);
}
if (!rc && replace && rename(tmppath.c_str(), path.c_str()) != 0)
rc = RPMRC_FAIL;

if (rc) {
rpmlog(RPMLOG_ERR, _("failed to import key: %s: %s\n"),
replace ? tmppath.c_str() : path.c_str(), strerror(errno));
if (replace)
unlink(tmppath.c_str());
}

exit:
free(keyval);
return rc;
}


/*****************************************************************************/

rpmRC keystore_fs::load_keys(rpmtxn txn, rpmKeyring keyring)
{
return load_keys_from_glob(txn, keyring, "%{_keyringpath}/*.key");
}

rpmRC keystore_fs::delete_key(rpmtxn txn, const string & keyid, const string & newname)
{
rpmRC rc = RPMRC_NOTFOUND;
Expand Down Expand Up @@ -82,55 +131,112 @@ rpmRC keystore_fs::delete_key(rpmtxn txn, rpmPubkey key)
rpmRC keystore_fs::import_key(rpmtxn txn, rpmPubkey key, int replace, rpmFlags flags)
{
rpmRC rc = RPMRC_FAIL;
const char *fp = rpmPubkeyFingerprintAsHex(key);
char *keyfmt = rstrscat(NULL, "gpg-pubkey-", fp, ".key", NULL);
char *keyval = rpmPubkeyArmorWrap(key);
char *path = rpmGenPath(rpmtxnRootDir(txn), "%{_keyringpath}/", keyfmt);
char *tmppath = NULL;
string fp = rpmPubkeyFingerprintAsHex(key);
string keyfmt = "gpg-pubkey-" + fp + ".key";
char *dir = rpmGenPath(rpmtxnRootDir(txn), "%{_keyringpath}/", NULL);
string dirstr = dir;

if (replace) {
rasprintf(&tmppath, "%s.new", path);
unlink(tmppath);
rc = write_key_to_disk(key, dirstr, keyfmt, replace, flags);

if (!rc && replace) {
/* find and delete the old pubkey entry */
pmatilai marked this conversation as resolved.
Show resolved Hide resolved
if (delete_key(txn, fp, keyfmt) == RPMRC_NOTFOUND) {
/* make sure an old, short keyid version gets removed */
delete_key(txn, fp.substr(32), keyfmt);
}
}

if (rpmMkdirs(rpmtxnRootDir(txn), "%{_keyringpath}")) {
free(dir);
return rc;
}

/*****************************************************************************/

static int acquire_write_lock(rpmtxn txn)
{
char * keyringpath = rpmGenPath(rpmtxnRootDir(txn), "%{_keyringpath}/", NULL);
char * lockpath = rpmGenPath(rpmtxnRootDir(txn), "%{_keyringpath}/", "writelock");
int fd = -1;

if (rpmMkdirs(NULL, keyringpath)) {
rpmlog(RPMLOG_ERR, _("failed to create keyring directory %s: %s\n"),
path, strerror(errno));
keyringpath, strerror(errno));
goto exit;
}

if (FD_t fd = Fopen(tmppath ? tmppath : path, "wx")) {
size_t keylen = strlen(keyval);
if (Fwrite(keyval, 1, keylen, fd) == keylen)
rc = RPMRC_OK;
Fclose(fd);
if ((fd = open(lockpath, O_WRONLY|O_CREAT)) == -1) {
rpmlog(RPMLOG_ERR, _("Can't create writelock for keyring at %s: %s\n"), keyringpath, strerror(errno));
} else if (flock(fd, LOCK_EX|LOCK_NB)) {
rpmlog(RPMLOG_ERR, _("Can't acquire writelock for keyring at %s\n"), keyringpath);
close(fd);
fd = -1;
}
if (!rc && tmppath && rename(tmppath, path) != 0)
rc = RPMRC_FAIL;

if (rc) {
rpmlog(RPMLOG_ERR, _("failed to import key: %s: %s\n"),
tmppath ? tmppath : path, strerror(errno));
if (tmppath)
unlink(tmppath);
}
exit:
free(keyringpath);
free(lockpath);
return fd;
}

if (!rc && replace) {
/* find and delete the old pubkey entry */
if (delete_key(txn, fp, keyfmt) == RPMRC_NOTFOUND) {
/* make sure an old, short keyid version gets removed */
delete_key(txn, fp+32, keyfmt);
}
}
static void free_write_lock(int fd)
{
flock(fd, LOCK_UN);
dmnks marked this conversation as resolved.
Show resolved Hide resolved
}

rpmRC keystore_openpgp_cert_d::load_keys(rpmtxn txn, rpmKeyring keyring)
{
return load_keys_from_glob(txn, keyring, "%{_keyringpath}/*/*");
}

rpmRC keystore_openpgp_cert_d::delete_key(rpmtxn txn, rpmPubkey key)
{
rpmRC rc = RPMRC_NOTFOUND;
int lock_fd = -1;

if ((lock_fd = acquire_write_lock(txn)) == -1)
return RPMRC_FAIL;

string fp = rpmPubkeyFingerprintAsHex(key);
string dir = fp.substr(0, 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This routine of obtaining the storage location (where the first two hex chars are the directory name) is a specific thing in this keystore implementation and it's currently duplicated in two places so maybe deserves a separate private method.

That said, we can always do that later, when there's the third use, so feel free to ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have considered that, but it doesn't really make things that much better. Guess it really is missing the third place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

string filename = fp.substr(2);
char * filepath = rpmGetPath(rpmtxnRootDir(txn), "%{_keyringpath}/", dir.c_str(), "/", filename.c_str(), NULL);
Copy link
Contributor

@dmnks dmnks Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a mixed use of rpmGetPath() and rpmGenPath() in the keystore.cc module. Is there a reason for preferring one over the other in those places?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, rpmGenPath was originally used. But as it only supports 3 pieces I used rpmGetPath here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, thanks.

char * dirpath = rpmGetPath(rpmtxnRootDir(txn), "%{_keyringpath}/", dir.c_str(), NULL);

if (!access(filepath, F_OK))
rc = unlink(filepath) ? RPMRC_FAIL : RPMRC_OK;
/* delete directory if empty */
rmdir(dirpath);

free(filepath);
free(dirpath);
free_write_lock(lock_fd);
return rc;
}

rpmRC keystore_openpgp_cert_d::import_key(rpmtxn txn, rpmPubkey key, int replace, rpmFlags flags)
{
rpmRC rc = RPMRC_NOTFOUND;
int lock_fd = -1;

if ((lock_fd = acquire_write_lock(txn)) == -1)
return RPMRC_FAIL;

string fp = rpmPubkeyFingerprintAsHex(key);
string dir = fp.substr(0, 2);
string filename = fp.substr(2);
char *dirpath = rpmGetPath(rpmtxnRootDir(txn), "%{_keyringpath}/", dir.c_str(), NULL);
string dirstr = dirpath;

rc = write_key_to_disk(key, dirstr, filename, replace, flags);

free_write_lock(lock_fd);
free(dirpath);

exit:
free(path);
free(keyval);
free(keyfmt);
free(tmppath);
return rc;
}

/*****************************************************************************/

rpmRC keystore_rpmdb::load_keys(rpmtxn txn, rpmKeyring keyring)
{
Header h;
Expand Down
7 changes: 7 additions & 0 deletions lib/keystore.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@ private:
rpmRC delete_key(rpmtxn txn, const std::string & keyid, unsigned int newinstance = 0);
};

class keystore_openpgp_cert_d : public keystore {
public:
virtual rpmRC load_keys(rpmtxn txn, rpmKeyring keyring);
virtual rpmRC import_key(rpmtxn txn, rpmPubkey key, int replace = 1, rpmFlags flags = 0);
virtual rpmRC delete_key(rpmtxn txn, rpmPubkey key);
};

}; /* namespace */

#endif /* _KEYSTORE_H */
2 changes: 2 additions & 0 deletions lib/rpmts.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,8 @@ static keystore *getKeystore(rpmts ts)
ts->keystore = new keystore_fs();
} else if (rstreq(krtype, "rpmdb")) {
ts->keystore = new keystore_rpmdb();
} else if (rstreq(krtype, "openpgp")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've a feeling we want to leave this name for a proper implementation from Sequoia side, this being a simple standalone version that does not deal with the trust stuff. But lets bikeshed that in the ticket instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I am not too sold on the name either.

ts->keystore = new keystore_openpgp_cert_d();
Copy link
Contributor

@dmnks dmnks Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class name seems a bit verbose to me, how about just keystore_openpgp? I suppose this would be the defacto standard openpgp keystore implementation so it doesn't have to be more specific than that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind the above; I've realized that openpgp is way too generic, this whole signature business is about OpenPGP (at least keystore_rpmdb is, too) so just keep the name as is, it's good.

} else {
/* Fall back to using rpmdb if unknown, for now at least */
rpmlog(RPMLOG_WARNING,
Expand Down
77 changes: 77 additions & 0 deletions tests/rpmsigdig.at
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,83 @@ runroot rpmkeys -Kv /data/RPMS/hello-2.0-1.x86_64.rpm /data/RPMS/hello-1.0-1.i38
RPMTEST_CLEANUP


AT_SETUP([rpmkeys key update (openpgp)])
AT_KEYWORDS([rpmkeys signature])
RPMDB_INIT
# root's .rpmmacros used to keep this build prefix independent
echo "%_keyring openpgp" >> "${RPMTEST}"/root/.rpmmacros
RPMTEST_CHECK([
runroot rpmkeys --import /data/keys/rpm.org-rsa-2048-test.pub
runroot rpmkeys -Kv /data/RPMS/hello-2.0-1.x86_64-signed-with-new-subkey.rpm
],
[1],
[/data/RPMS/hello-2.0-1.x86_64-signed-with-new-subkey.rpm:
Header OpenPGP V4 EdDSA/SHA512 signature, key ID 6323c42711450b6c: NOKEY
Header SHA256 digest: OK
Payload SHA256 digest: OK
],
[])

RPMTEST_CHECK([
runroot_other touch /usr/lib/sysimage/rpm/pubkeys/writelock
runroot_other flock -x /usr/lib/sysimage/rpm/pubkeys/writelock -c "rpmkeys --import /data/keys/rpm.org-rsa-2048-add-subkey.asc"
runroot_other rm /usr/lib/sysimage/rpm/pubkeys/writelock
],
[0],
[],
[error: Can't acquire writelock for keyring at /usr/lib/sysimage/rpm/pubkeys
error: /data/keys/rpm.org-rsa-2048-add-subkey.asc: key 1 import failed.
])


RPMTEST_CHECK([
runroot rpmkeys --list | wc -l
runroot rpmkeys --import /data/keys/rpm.org-rsa-2048-add-subkey.asc
runroot rpmkeys --list | wc -l
],
[0],
[1
1
],
[])

RPMTEST_CHECK([
runroot rpmkeys -Kv /data/RPMS/hello-2.0-1.x86_64-signed-with-new-subkey.rpm
],
[0],
[/data/RPMS/hello-2.0-1.x86_64-signed-with-new-subkey.rpm:
Header OpenPGP V4 EdDSA/SHA512 signature, key fingerprint: 771b18d3d7baa28734333c424344591e1964c5fc: OK
Header SHA256 digest: OK
Payload SHA256 digest: OK
],
[])

RPMTEST_CHECK([
runroot rpmkeys --delete abcd gimmekey 1111aaaa2222bbbb
],
[1],
[],
[error: invalid key id: abcd
error: invalid key id: gimmekey
error: key not found: 1111aaaa2222bbbb
])

RPMTEST_CHECK([
runroot rpmkeys --delete 1964c5fc
],
[0],
[],
[])

RPMTEST_CHECK([
runroot rpmkeys --list | wc -l
],
[0],
[0
],
[])
RPMTEST_CLEANUP

AT_SETUP([rpmkeys -Kv <reconstructed> 1])
AT_KEYWORDS([rpmkeys digest])
RPMDB_INIT
Expand Down
Loading