google / rpmpack

rpmpack (tar2rpm) - package rpms in pure golang or cli
Apache License 2.0
116 stars 32 forks source link

add pgp signing interface. #39

Closed jarondl closed 4 years ago

jarondl commented 4 years ago

This allows library callers to give us a gpg signing function, which will be used to sign the contents. The callers can create the signature however way they want, for example by using golang's openpgp package or by forking out to gpg. This mostly depends on managing the secret material. If they want to use the user's keys I guess forking to gpg is not too bad.

The test is not great: rpmsample was modified to add a signature, which basically returns the same bytes all the time. This does not actually validate if rpm based distros accept the key.

erikgeiser commented 4 years ago

What's the plan to go forward with this? We would like to use this to implement rpm signing over at https://github.com/goreleaser/nfpm/issues/212.

I like the pluggable AddPGPSigner method from a user's perspective. I don't know if the acceptance really needs to be tested as the user is responsible to generate a valid signature anyway. The tests basically only need to ensure that the signature is performed over the right data and put in the right place afterwards. Maybe a good test would be to create a signed RPM, verify it manually once to be sure it works, then modify the PGP signer to write the input bytes into a golden file and then use this file to test if the signature is always performed over the right data.

jarondl commented 4 years ago

Basically in #38 I was waiting for a user of this feature to tell me if the API is acceptable. Since I don't plan to use this feature, I need to make sure it makes sense before merging.

I like the way you phrased the testing issue (check that the correct input is used, and the output written in the right place), but dealing with the rpm ecosystem, most of the trouble comes from readers of the package.

Anyway, if you thing this pr is useful for you, I can merge it. Did you have a chance to test it with an actual signature?

erikgeiser commented 4 years ago

I just added PGP code in a PR to https://github.com/goreleaser/nfpm that I can directly plug into the AddPGPSigner. I will try this PR out now. nfpm will also have acceptance tests for the signing feature by installing the resulting packages in docker containers.

erikgeiser commented 4 years ago

With a valid signature the rpm can successfully be verified using rpm --import pubkey.asc && rpm -K test.rpm and it is also installable. I also wrote a test that verifies the signature successfully using https://github.com/sassoftware/go-rpmutils.

erikgeiser commented 4 years ago

Would it be possible to make the argument variadic to support multiple signatures? We probably won't need multiple signatures for nfpm but as far as I know the rpm format supports it and the AddPGPSigner method name suggests that you could call it multiple times for multiple signers (as opposed to SetPGPSigner). Or the method could just be renamed to make it clear.

jarondl commented 4 years ago

That's really awesome, thank you for testing it, and for setting up acceptance tests. Now when you say "multiple signatures", do you mean multiple signatures, concatenated, with the same tag (sigPGP = 0x03ea // 1002), or with different tags?

If it is the first option, it is up to the caller. Their method can simply calculate multiple signatures and return them all.

If it is the second, we'll need a way to accept the tag, and perhaps, keep a mapping from tags to signatures... Sounds complicated unless there is a real usecase.

Anyway I agree, I will rename this to SetPGPSigner.

erikgeiser commented 4 years ago

I don't know that much about RPMs but this page[1] seems to suggest in the --adsign section that multiple signatures are supported in principle. I don't know how exactly that's implemented though.

However, it is probably best to add this only if it is actually needed by someone. Renaming the method to SetPGPSigner still leaves the option open to later add the plural method SetPGPSigners(...). I was just concerned that if multiple signatures are needed at some point, the API would be a little awkward with AddPGPSigner in this context.

[1] http://ftp.rpm.org/max-rpm/s1-rpm-pgp-signing-packages.html