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.

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

OpenPGP key support #5133

Merged
merged 1 commit into from
Oct 12, 2024
Merged

OpenPGP key support #5133

merged 1 commit into from
Oct 12, 2024

Conversation

mdellweg
Copy link
Member

This adds a repository type as a keyring and content types to handle keys, keyids and key signatures.

fixes #3024

@mdellweg mdellweg force-pushed the 3024_keyring branch 2 times, most recently from 188b241 to 4dfa211 Compare March 14, 2024 15:28
}


def packet_iter(data):
Copy link
Contributor

@dralley dralley Mar 15, 2024

Choose a reason for hiding this comment

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

So, this appears to be a de-novo implementation of PGP packet parsing that we'd be on the hook to maintain. Is that 100% necessary? And if so, why?

Is it at all possible to use an existing maintained library such as sequoia-pgp (via pysequoia)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think i looked for such things and none really ticked all the boxes (It's been a while though).

About pysequoia (from their pypi representation):
Note: This is a work in progress. The API is not stable!
Note that since pysequoia is implemented largely in Rust, a [Rust toolchain](https://rustup.rs/) is necessary for the installation to succeed.

Copy link
Contributor

@dralley dralley Mar 15, 2024

Choose a reason for hiding this comment

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

Note that since pysequoia is implemented largely in Rust, a Rust toolchain is necessary for the installation to succeed.

This part at least would be nothing new for us, several of our deps already check this box. (py)cryptography and json-stream-rs-tokenizer (transitive dep via json-stream) for instance. I think there were maybe one or two others.

Note: This is a work in progress. The API is not stable!

True, but this note is also a few years old, and the API doesn't seem to be particularly unstable. In a tradeoff between occasionally needing to tweak a few lines vs. maintaining a parser implementation for cryptography... my personal preference would be the former.

rpm uses sequoia-pgp (though not via Python) so the quality of the underlying implementation is already trusted so far as Red Hat is concerned. One of the primary authors is the maintainer of the crypto subsystem of the linux kernel.

Copy link
Member Author

Choose a reason for hiding this comment

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

What i still don't see (from looking at their README) is a way to disassemble the bytestream into openpgp packets which we want to store individually. But i should take a closer look.

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot see how pysequoia is providing the raw parsing data we require here.
Having said that, I believe the rfc will not change much giving us not too bad of a maintenance burden. Also let me state that very clear here. We only act on openpgp keys as a content type here. We parse them to find the individual key/signature packets and extract some metadata. We do not perform anything cryptographic here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sequoia has it AFAICT, but pysequoia doesn't have it exposed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mdellweg Could you enumerate anything I missed here? wiktor-k/pysequoia#32

Copy link
Member Author

Choose a reason for hiding this comment

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

At this point it's mainly splitting the key into subpackages, because that way we can collect signatures for keys from separate uploads. Also we need the fingerprints. The rest of the data is mainly conveniently there too. And I don't know what else we want to add later. It's really hard to say what we want/need from this new content type in the future.
Quite honestly i'm not ready add another dependency (let alone one that is not pure python) to "just" parse a simple file format that is probably not going to change. If this was about doing actual cryptography I would say the exact opposite.

Copy link

stale bot commented Jun 13, 2024

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

@stale stale bot added the stale label Jun 13, 2024
@mdellweg mdellweg removed the stale label Jun 14, 2024
Copy link

stale bot commented Sep 13, 2024

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

@stale stale bot added the stale label Sep 13, 2024
@mdellweg mdellweg removed the stale label Sep 13, 2024
@mdellweg mdellweg force-pushed the 3024_keyring branch 2 times, most recently from b7236bb to 0f39a6c Compare September 26, 2024 10:59
@mdellweg mdellweg marked this pull request as ready for review September 27, 2024 09:34
@dralley dralley self-requested a review October 2, 2024 04:04

@property
def key_expired(self):
# TODO handle more cases
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need fixing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

@mdellweg mdellweg Oct 9, 2024

Choose a reason for hiding this comment

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

I think we can wait until someone complains. Removing the comment.
It is a threestate anyway: Yes, No, Don't know. (True/False/None).

class Meta:
default_related_name = "%(app_label)s_%(model_name)s"
permissions = [
# ("sync_openpgpkeyring", "Can start a sync task"),
Copy link
Contributor

Choose a reason for hiding this comment

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

^

"name": "EdDSA",
"format": "om",
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these options are in the PGP standard, but that doesn't mean we want to accept all of them. DSA is long-obsolete, as is MD5 below. How do we want to handle that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I think we just take and organize whatever we are presented with.
This is just meant to be a keyring for public key data. Even for verifying anything, we call out to gpg. And the gpg installed should decline working with deprecated algorithms.
Even more so, we should be able to report on these formats, so a user can make an informed decision.

raise NotImplementedError("Version 3 signatures are not implemented.")
elif version in [4, 5]:
signature_type = data[1]
# pubkey_algorithm = PUBKEY_ALGORITHMS.get(data[2])
Copy link
Contributor

Choose a reason for hiding this comment

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

^

Copy link
Member Author

Choose a reason for hiding this comment

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

This is mainly for documentation purpose.

unhashed_size = (data[6 + hashed_size] << 8) + data[7 + hashed_size]
unhashed_data = data[8 + hashed_size : 8 + hashed_size + unhashed_size]
canary = data[8 + hashed_size + unhashed_size : 10 + hashed_size + unhashed_size]
# signature = data[10 + hashed_size + unhashed_size :]
Copy link
Contributor

Choose a reason for hiding this comment

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

^

raise ValueError("Out of band key signature.")
hash_payload = b""
else:
# TODO:
Copy link
Contributor

Choose a reason for hiding this comment

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

^

n = extract_mpi(data[8:])
e = extract_mpi(data[8 + len(n["raw"])])
fingerprint = hashlib.md5(n["body"] + e["body"]).hexdigest()
# expiration = int.from_bytes(data[5:7], "big")
Copy link
Contributor

Choose a reason for hiding this comment

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

^

pos += len(oid["raw"])
elif item_type == "k":
# KDF
# TODO more analysis
Copy link
Contributor

Choose a reason for hiding this comment

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

^

@mdellweg mdellweg force-pushed the 3024_keyring branch 2 times, most recently from 3ff4ce2 to 8a89ff3 Compare October 2, 2024 11:12
@mdellweg
Copy link
Member Author

mdellweg commented Oct 2, 2024

I removed the comments related to synching, as we don't support that.
Also improved the openpgp "library" module for readability.

@mdellweg mdellweg force-pushed the 3024_keyring branch 2 times, most recently from c48fe0c to 05f5ae5 Compare October 9, 2024 11:03
dralley
dralley previously approved these changes Oct 11, 2024
Copy link
Contributor

@dralley dralley left a comment

Choose a reason for hiding this comment

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

To the extent that I can reasonably review this (not having much subject matter expertise on PGP), it looks fine.

One thing though, it kinda feels like the 3 migrations should be merged into one if possible.

@dralley
Copy link
Contributor

dralley commented Oct 11, 2024

@mdellweg Is it possible to merge those migrations together?

This adds a repository type as a keyring and content types to handle
keys, keyids and key signatures.

fixes pulp#3024
@mdellweg
Copy link
Member Author

Is it possible to merge those migrations together?

Should have done that earlier. It's so hard to constantly rebase them.

@mdellweg mdellweg merged commit b6c1ef3 into pulp:main Oct 12, 2024
12 checks passed
@mdellweg mdellweg deleted the 3024_keyring branch October 12, 2024 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide basic gpg public key management
2 participants