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

Alternative approach at signature creation using PGPainless #56

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

vanitasvitae
Copy link
Contributor

This PR allows to use PGPainless for OpenPGP signature creation.
It is an alternative approach to #48 which incorporates some of the feedback given.

Contrary to #48, this PR does not straight up replace the existing BC-based signature implementation, but instead introduces a factory pattern, which allows to swap out the implementation backend.

The code is not yet tested, so I'll mark it as WIP.

Still, I'd like to get some feedback to get an idea of how to proceed :)

@vanitasvitae vanitasvitae marked this pull request as draft April 3, 2024 20:06
Copy link
Contributor Author

@vanitasvitae vanitasvitae left a comment

Choose a reason for hiding this comment

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

I'm happy for feedback :)
Please let me know, how you'd inject the PgpSignerCreatorFactory and PgpSignatureProcessorFactory instances via the optional dependency :)

case PublicKeyAlgorithmTags.DSA: // 17
case PublicKeyAlgorithmTags.EDDSA_LEGACY: // 22
getLogger().info("DSA: {}", value);
signature.putBlob(RpmSignatureTag.GPG, value);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

RpmSignatureTag.GPG was missing, so I added it.
Do I understand the protocol correctly in that RsaSignatureProcessor (and analogue PGPainlessSignatureProcessor) do implement the "RPM v3 signature with the same key on the header+payload" the manual is talking about?

Again, the documentation only mentions ECDSA, not EdDSA.

Copy link
Contributor

Choose a reason for hiding this comment

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

We support RPMv4, not RPMv3, so I don't think we want the older tags mentioned there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, you can keep the constants defined, but I am not sure we want to write them in the header.

Copy link
Contributor Author

@vanitasvitae vanitasvitae Apr 3, 2024

Choose a reason for hiding this comment

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

RsaSignatureProcessor is using RpmSignatureTag.PGP (which as far as I understand is RPM v3) here.

Copy link
Contributor

Choose a reason for hiding this comment

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

RsaSignatureProcessor is using RpmSignatureTag.PGP (which as far as I understand is RPM v3) here.

It depends what version of RPM we are emulating. I can find "RPMSIGTAG_PGP, RPMSIGTAG_GPG (and RPMSIGTAG_PGP5) are no longer created in >= 4.16". So, we can have both sets, but if we are going to only have one set, it should not be these.

I think the new ones only process the header, whereas the old ones processed the header together with the payload.

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'm still confused.
To slim down the PR I'd remove the new constant again.
However, what RpmSignatureTag should I use in the PGPainlessSignatureProcessor for EdDSA/DSA signatures?
If RpmSignatureTag.PGP (used for RSA) is also no longer created in newer Rpm versions, shall I just remove the PGPainlessSignatureProcessor altogether? What about its BC counterpart (RsaSignatureProcessor)? Remove that as well? It appears it is still in use in RpmFileSignatureProcessor. Is this class also obsolete then?

Copy link
Contributor

@dwalluck dwalluck Apr 4, 2024

Choose a reason for hiding this comment

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

There are two types of signatures. One is header-only and one is header and payload. That makes two possible for RSA and two possible for DSA/EdDSA for a total of four signature tags.

I think it is like this:

  1. DSA tag: 267 type: header (RPMv4)
  2. RSA tag: 268 type: header (RPMv4)
  3. PGP tag: 1002 type: header+payload (RPMv3; deprecated)
  4. GPG tag: 1005 type: header+payload (RPMv3; deprecated)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So RpmFileSignatureProcessor currently uses RPMv3 signing then (RsaSignatureProcessor uses PGP tag).
I guess I'll leave RsaSignatureProcessor untouched and also won't change RpmFileSignatureProcessor in this PR.

I'll remove PGPainlessSignatureProcessor (which would do RPMv3 signing) and instead only implement PGPainlessHeaderSignatureProcessor.

@vanitasvitae
Copy link
Contributor Author

I'm not super fond of the name of the module, so feel free to provide suggestions :)

@dwalluck
Copy link
Contributor

dwalluck commented Apr 3, 2024

I'm not super fond of the name of the module, so feel free to provide suggestions :)

I would just call the directory/module pgpainless, and the artifact packager-pgpainless.

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.

2 participants