pgp: modernize and improve, and add tests #1054

hiddeco · 2022-05-06T23:46:59Z

This replaces the current PGP keysource implementation with a modernized
version the Flux project has been using.

It includes utilities to configure the MasterKey via other means than
environment variables, to allow SDK users to have extensive control
over what things are decrypted with. This can for example be combined
with an own keyserver implementation.

To be able to contribute it back upstream while keeping it backwards
compatible with SOPS, a couple of changes have been made compared to
Flux:

Instead of removing the enabling of the agent while making use of
GnuPG, it can now be disabled.
Support for OpenPGP has been added back. Note however my comments
on this in-code, as I am not quite sure to what extend it is used
at the moment, as it will not work on most setups (GnuPG <2.1 was
released in 2017.)
The absolute paths to the pub and sec keyrings can now be configured
by SDK users. This would add more reason to keep OpenPGP around, if
they are able to produce the keyring files themselves via other means
than GnuPG.
When a sec keyring is not detected, the pub keyring is loaded instead. This
to account for GnuPG >=2.1 merging the sec keyring into pub keyring.
Support for fetching keys from servers has been removed. This can be
added back if we need to keep it around for a little longer.

The Flux version of this has compatibility tests with current SOPS:

hiddeco · 2022-05-07T00:14:09Z

@ajvb please have a look. As you can see, introduction of configuration of the MasterKey could already be introduced. But I am happy to discuss other options. I made some other notes in-code, it would be great to have your opinion on this.

If I can get a sense of the general direction we wanted to head at, the other implementations could follow quickly. Thanks ✨

hwine · 2022-05-13T18:27:39Z

@hiddeco can you describe what changes this makes for the user after this is merged? e.g.

configuration of local pgp/gpg setup to work with SOPS
any changes to making a new public key available for SOPS to use

Actually, I'm not really sure I know the answers for the current version 😞 my guesses:

if gpg is in path, you're okay
can only use public keys in local gpg keystore (which implies they must have a user id)

hiddeco · 2022-05-13T18:43:26Z

The PGP look-up via a server is no longer possible, so the key is indeed expected to exist in the user their key ring. However, this can be re-added. Our implementation does not require it, so I did not include it while writing it based on SOPS' own.

Besides this, it changes very little for a CLI user:

GnuPG binary is detected according to same logic, including the ability to overwrite the absolute path of the binary using SOPS_GPG_EXEC.
Instead of failing on the lack of a secring.gpg, it now attempts to open pubring.gpg instead before falling back to shelling out to GnuPG. This is added behavior to cover for users who have their secring.gpg merged into pubring.gpg due to GnuPG >=2.1 changes.

The strength in this PR is mostly in the fact that:

It has test coverage of way more test cases, and revised and restructured code.
It provides a rich set of utilities to work with SOPS as a SDK, to for example decrypt SOPS encrypted files users provide to you. Using these utilities, you could write your own key service server implementation to serve decryption requests with a set of specific secrets.

You can see an example of the latter here. As we are running decryption operations for multiple tenants in the same process, we can't rely on the decrypt.Decrypt of SOPS itself, which utilizes runtime environment variables which are not shared between tenants.

Implementing this would help us (and potentially quite some others) in not having to write their forked key service implementations to control the decryption process using a local server.

hwine · 2022-05-16T16:21:39Z

Thanks for the clarification! So no change to user experience beyond enforcing the previously announced deprecation.

ajvb

Left a few comments but this is looking great to me!

pgp/keysource.go

pgp/keysource_test.go

This replaces the current PGP keysource implementation with a modernized version the Flux project has been using[1]. It includes utilites to configure the MasterKey via other means than environment variables, to allow SDK users to have extensive control over what things are decrypted with. This can for example be combined with an own keyserver implementation. To be able to contribute it back upstream while keeping it backwards compatible with SOPS, a couple of changes have been made compared to Flux: - Instead of removing the enabling of the agent while making use of GnuPG, it can now be disabled. - Support for OpenPGP has been added back. Note however my comments on this in-code, as I am not quite sure to what extend it is used at the moment, as it will not work on most setups (GnuPG <2.1 was released in 2017.) - The absolute paths to the pub and sec keyrings can now be configured by SDK users. This would add more reason to keep OpenPGP around, if they are able to produce the keyring files themselves via other means than GnuPG. - When a sec keyring is not detected, a lookup for the pub keyring is made and loaded instead if found. This to account for GnuPG >=2.1 merging the sec keyring into pub keyring. - Support for fetching keys from servers has been removed. This can be added back if we need to keep it around for a little longer. This has extensive test coverage for GnuPG, but would need coverage for the re-added OpenPGP implementation before it can be deemed ready. [1]: https://github.com/fluxcd/kustomize-controller/tree/ffdda3f3da75aa39a5b5c29997c2654b6a2f1f89/internal/sops/pgp Signed-off-by: Hidde Beydals <[email protected]>

Signed-off-by: Hidde Beydals <[email protected]>

hiddeco · 2022-05-26T09:04:07Z

pgp/keysource.go

+// SopsGpgExecEnv to the absolute path of the replacement binary.
+func gpgBinary() string {
+	binary := "gpg"
+	if envBinary := os.Getenv(SopsGpgExecEnv); envBinary != "" && filepath.IsAbs(envBinary) {


@ajvb did a last comparison round, and we introduced this stricter behavior on the environment variable to ensure nothing magical is injected. Any concerns this may break things for people?

hiddeco · 2022-05-26T09:05:03Z

This has now tests added for OpenPGP as well, plus some coverage around the fallback behavior, etc. If there are no other concerns, it should be OK to merge.

Signed-off-by: Hidde Beydals <[email protected]>

ajvb

Exciting! Merging in here shortly. I'll do some additional functional testing prior to our next release.

james-callahan · 2023-09-18T01:00:30Z

@hiddeco looks like this added --no-default-keyring and --homedir which unfortunately broke usage with qubes split gpg (https://www.qubes-os.org/doc/split-gpg/ / https://github.com/QubesOS/qubes-app-linux-split-gpg)

Will try and followup with them; but I think you'll have a few people coming in with various confusing error messages.

hiddeco added enhancement area/keyservice keyservice/pgp labels May 14, 2022

ajvb reviewed May 19, 2022

View reviewed changes

pgp/keysource.go Show resolved Hide resolved

pgp/keysource.go Show resolved Hide resolved

pgp/keysource.go Show resolved Hide resolved

pgp/keysource_test.go Outdated Show resolved Hide resolved

hiddeco mentioned this pull request May 20, 2022

Relicense SOPS module to MPL 2.0 fluxcd/kustomize-controller#656

Merged

hiddeco added 3 commits May 26, 2022 11:02

pgp: add Cleanup util func to GnuPGHome

c4458d7

Signed-off-by: Hidde Beydals <[email protected]>

pgp: allow disabling OpenPGP capabilities

3c8d637

Signed-off-by: Hidde Beydals <[email protected]>

hiddeco marked this pull request as ready for review May 26, 2022 09:02

hiddeco commented May 26, 2022

View reviewed changes

hiddeco requested a review from ajvb May 27, 2022 21:45

pgp: extend test coverage OpenPGP

8a98401

Signed-off-by: Hidde Beydals <[email protected]>

ajvb approved these changes May 31, 2022

View reviewed changes

ajvb merged commit 0014114 into getsops:develop May 31, 2022

hiddeco deleted the pgp-keysource-imprv branch June 1, 2022 06:29

hiddeco added this to the v3.8.0 milestone Jul 4, 2023

james-callahan mentioned this pull request Sep 18, 2023

sops 3.8.0 doesn't work with qubes split gpg #1294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pgp: modernize and improve, and add tests #1054

pgp: modernize and improve, and add tests #1054

hiddeco commented May 6, 2022 •

edited

Loading

hiddeco commented May 7, 2022

hwine commented May 13, 2022

hiddeco commented May 13, 2022 •

edited

Loading

hwine commented May 16, 2022

ajvb left a comment

hiddeco May 26, 2022

hiddeco commented May 26, 2022

ajvb left a comment

james-callahan commented Sep 18, 2023

pgp: modernize and improve, and add tests #1054

pgp: modernize and improve, and add tests #1054

Conversation

hiddeco commented May 6, 2022 • edited Loading

hiddeco commented May 7, 2022

hwine commented May 13, 2022

hiddeco commented May 13, 2022 • edited Loading

hwine commented May 16, 2022

ajvb left a comment

Choose a reason for hiding this comment

hiddeco May 26, 2022

Choose a reason for hiding this comment

hiddeco commented May 26, 2022

ajvb left a comment

Choose a reason for hiding this comment

james-callahan commented Sep 18, 2023

hiddeco commented May 6, 2022 •

edited

Loading

hiddeco commented May 13, 2022 •

edited

Loading