-
Notifications
You must be signed in to change notification settings - Fork 16
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
base: master
Are you sure you want to change the base?
Conversation
This factory can be used to create signing streams
Use it in RepostoryCreator
…rocessors and SignerCreators
There was a problem hiding this 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 :)
...rc/main/java/org/eclipse/packager/rpm/signature/pgpainless/PGPainlessSignatureProcessor.java
Outdated
Show resolved
Hide resolved
case PublicKeyAlgorithmTags.DSA: // 17 | ||
case PublicKeyAlgorithmTags.EDDSA_LEGACY: // 22 | ||
getLogger().info("DSA: {}", value); | ||
signature.putBlob(RpmSignatureTag.GPG, value); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- DSA tag: 267 type: header (RPMv4)
- RSA tag: 268 type: header (RPMv4)
- PGP tag: 1002 type: header+payload (RPMv3; deprecated)
- GPG tag: 1005 type: header+payload (RPMv3; deprecated)
There was a problem hiding this comment.
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.
...n/java/org/eclipse/packager/rpm/signature/pgpainless/PGPainlessHeaderSignatureProcessor.java
Outdated
Show resolved
Hide resolved
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. |
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 :)