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

Improve UX for multiple split masterIdentities #28

Merged
merged 6 commits into from
Jun 9, 2024
Merged

Conversation

Lukas-C
Copy link
Contributor

@Lukas-C Lukas-C commented Jun 3, 2024

Fixes #24.

Changes

First, the implementation introduces a new syntax for specifying identities in age.rekey.masterIdentities:

{
  # This has the same type as the old way to specify an identity.
  identity = ./path-to-identity;
  # Optional; This has the same type as `age.rekey.hostPubkey`
  # and allows explicit association of a pubkey with an identity.
  pubkey = "age1yubikey1<pubkey>";
}

The old syntax continues to be supported through automatic coercion into the new format. If pubkey is specified, it will be used to encrypt files, instead of trying to extract a pubkey from the identity file.

Second, the implementation may extract an "implicit" pubkey from the identity file to use instead of the identity itself. This will only happen if the following conditions are met:

  • The identity does not already specify pubkey using the above syntax.
  • The identity file contains exactly one identity.
  • The identity is of the form AGE-PLUGIN-YUBIKEY-<...>.
  • The pubkey is specified in the form Recipient: age1yubikey1<pubkey>.
  • Exactly one pubkey is found in the identity file. If there is more than one pubkey found, it is considered ambiguous and a hard error, since it may risk encrypting to unwanted (and possibly unsafe) pubkeys.

Every identity file that does not match all of the above criteria will be passed to (r)age without further processing, in order to let the program itself deal with the identity at runtime.

Third, the implementation adds support for the new environment variable AGENIX_REKEY_PRIMARY_IDENTITY, which is used during decryption. If set to a pubkey, agenix-rekey will attempt to locate the key amongst the explicitly and implicitly specified pubkeys:

  • If the pubkey can be successfully matched, the associated identity file is specified in first place of the arguments passed to (r)age and will thus get the first attempt at decrypting the file. The order of the identities that follow remains untouched.
  • If the pubkey cannot be matched, all identities are simply tried in order. A warning is printed during both encryption and decryption operations to inform the user about a potentially inconsistent configuration.

Implementation

I ended up writing a wrapper script for (r)age in ./nix/lib.nix that is shared between for the encrypt and decrypt phases and decides what phase to run based on the first argument it receives. The remaining arguments are directly passed to (r)age. Warnings are deferred to stderr in order to not mess with generators that use the decrypt command in a piping fashion, e.g.:

generator.script = "cat ${./secret.age} | ${decrypt}";

Testing

The current code can successfully handle the following flake.nix. See the comments next to the different masterIdentities and age.secrets for further details:

{
  inputs = {
    nixpkgs.url = "github:nixos/nixpkgs/nixpkgs-unstable";

    agenix = {
      url = "github:ryantm/agenix";
      inputs.nixpkgs.follows = "nixpkgs";
    };
    agenix-rekey = {
      #url = "github:oddlama/agenix-rekey";
      url = "github:Lukas-C/agenix-rekey";
      inputs.nixpkgs.follows = "nixpkgs";
    };
  };

  outputs = inputs@{ self, nixpkgs, agenix, agenix-rekey, ... }:
    let
      system = "x86_64-linux";
      pkgs = import nixpkgs { inherit system; };
    in {
      nixosConfigurations."test" = nixpkgs.lib.nixosSystem {
        modules = [
          agenix.nixosModules.default
          agenix-rekey.nixosModules.default
          {
            nixpkgs = { inherit system; };

            age.rekey = {
              storageMode = "local";
              localStorageDir = ./rekey;

              masterIdentities = [
                # Yubikey A, not plugged in
                ./identities/yubikey-a.pub

                # Yubikey B, plugged in
                ./identities/yubikey-b.pub

                ## Yubikey B, multiple pubkeys for one identity.
                ## Confirmed to fail, since this is not supported.
                #./identities/yubikey-b-multiple-pubkey.pub

                # Unencrypted plain `age` identity with a wonky name.
                # Can be used as-is, since the identity does not require further action,
                # even though this configuration should not be used.
                (./identities + "/test -i key.key")

                # Encrypted plain `age` identity, without an available pubkey.
                # Prompts for the passphrase, during both encryption and decryption.
                ./identities/testkeypass.key

                # (Same) encrypted identity with a pubkey.
                # Would only prompt for the passphrase during decryption.
                {
                  identity = ./identities/testkeypass.key;
                  pubkey = "age1w0reymggaakqgck6d8ndy635deh8p7eavaskpz0u48yqlad8v55qqum065";
                }
              ];
            };

            age.secrets = {
              # Basic secret file created with `agenix edit`.
              test = { rekeyFile = ./test.txt.age; };

              # Basic generated file to test `agenix generate`.
              test-generate = {
                rekeyFile = ./generate.age;
                generator.script = { ... }: "echo generator";
              };

              # Test proper behavior when piping into the `decrypt` command,
              # which is now a wrapper script around (r)age.
              # Requires setting AGENIX_REKEY_PRIMARY_IDENTITY to an invalid pubkey to test properly.
              # The resulting warning should not appear in the file as it is directed to stderr instead.
              test-generate-stdio = {
                rekeyFile = ./generate-stdio.age;
                generator.script = { decrypt, ... }: ''
                  cat ${./generate.age} | ${decrypt}
                '';
              };
            };
          }
        ];
      };

      agenix-rekey = agenix-rekey.configure {
        userFlake = self;
        nodes = self.nixosConfigurations;
      };

      devShells."x86_64-linux".default = pkgs.mkShell {
        packages = [ agenix-rekey.packages."x86_64-linux".default ];
      };
    };
}

AGENIX_REKEY_PRIMARY_IDENTITY set to pubkey of Yubikey B

  • agenix edit:
    1. Prompts for Yubikey B PIN for decryption.
    2. Prompts for testkeypass.key during encryption.
  • agenix generate -f:
    1. Prompts once for Yubikey B PIN for decryption. The PIN is only prompted for once.
    2. Prompts for testkeypass.key for each generated secret during encryption.
  • agenix rekey -f:
    1. Prompts once for specified Yubikey for decryption. The PIN is only prompted for once.

AGENIX_REKEY_PRIMARY_IDENTITY unset

  • agenix edit:
    1. Prompts for Yubikey A PIN for decryption. Prompts for Yubikey B PIN if the first is skipped and (r)age does not fail.
    2. Prompts for testkeypass.key during encryption.
  • agenix generate -f:
    1. Prompts for Yubikey A PIN for decryption. Prompts for Yubikey B PIN if the first is skipped and (r)age does not fail. Will prompt for PIN of Yubikey B every time Yubikey A is skipped.
    2. Prompts for testkeypass.key for each generated secret during encryption.
  • agenix rekey -f:
    1. Prompts for Yubikey A PIN for decryption. Prompts for Yubikey B PIN if the first is skipped and (r)age does not fail. Will prompt for PIN of Yubikey B every time Yubikey A is skipped.

AGENIX_REKEY_PRIMARY_IDENTITY set to invalid pubkey

Behavior is the same as if unset. The following warning is printed at least once for every one of the three operations:

warning: Environment variable AGENIX_REKEY_PRIMARY_IDENTITY is set, but matches none of the pubkeys found by agenix-rekey.
warning: Please check that your pubkeys and identities are set up correctly.
warning: Value of AGENIX_REKEY_PRIMARY_IDENTITY: "test"
warning: Pubkeys found:
warning:   age1yubikey1<pubkey-a> for file "/nix/store/<hash>-source/identities/yubikey-a.pub"
warning:   age1yubikey1<pubkey-b> for file "/nix/store/<hash>-source/identities/yubikey-b.pub"
warning:   age1<pubkey> for file "/nix/store/<hash>-source/identities/testkeypass.key"

TODO

  • Documentation

Copy link
Owner

@oddlama oddlama left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation! I've already looked through your additions and the new script but I don't think I have anything to add at this point. This is looking pretty great already!

I guess you can just copy some parts of your PR description and put it in as documentation. All that's needed is probably a mention of the environment variable and an update to the masterIdentities option description to point out how it works now. For any more details I'd say you can just link to this PR.

I'm not sure which additional warnings you'd fancy? I'd definitely be fine with what you already have.

Thanks again!

@Lukas-C
Copy link
Contributor Author

Lukas-C commented Jun 4, 2024

I was originally considering warnings that indicate a "potentially degraded" user experience, e.g. a warning if a Yubikey identity does not have an associated pubkey. However I think now that a proper explanation in the manual/readme is required anyways and will be sufficient. No need to create additional complexity where there doesn't have to be.

So if you are happy with the current state, I will only add the remaining documentation and leave the PR otherwise as is.

- document new age.rekey.masterIdentities syntax
- document AGENIX_REKEY_PRIMARY_IDENTITY environment variable
@Lukas-C Lukas-C marked this pull request as ready for review June 8, 2024 15:13
@Lukas-C
Copy link
Contributor Author

Lukas-C commented Jun 8, 2024

I have now expanded the README and hope the explanations are clear enough so that people know what is possible, what to do and where to look.

Minor remarks:

  • I had to omit some of the details of the new masterIdentities type signature, since readability otherwise would have suffered even more. Instead I made due with a link to the actual lines of code. At the moment it points to the location where the corresponding lines should end up after the merge, so we should probably update this to a proper permalink afterwards.
  • I added a separate section for the new environment variable (and potential future ones). Since we're somehow still missing a terminal prompt emoji in the Unicode standard, I made due with a keyboard (⌨) as the section logo.

@oddlama
Copy link
Owner

oddlama commented Jun 9, 2024

I have now expanded the README and hope the explanations are clear enough so that people know what is possible, what to do and where to look.

Looks good, thanks.

Minor remarks:

* I had to omit some of the details of the new `masterIdentities` type signature, since readability otherwise would have suffered even more. Instead I made due with a link to the actual lines of code. At the moment it points to the location where the corresponding lines should end up after the merge, so we should probably update this to a proper permalink afterwards.

I'll probably switch to mdbook rendered documentation at some point, then it will hopefully be more readable.

* I added a separate section for the new environment variable (and potential future ones). Since we're somehow still missing a terminal prompt emoji in the Unicode standard, I made due with a keyboard (⌨) as the section logo.

Good idea!

I think this is good to now, thanks again for all your work!

@oddlama oddlama merged commit c202721 into oddlama:main Jun 9, 2024
@Lukas-C
Copy link
Contributor Author

Lukas-C commented Jun 9, 2024

Happy to contribute, thank you for your openness towards my suggestions and for the constructive discussion and feedback :D

@Lukas-C Lukas-C mentioned this pull request Jun 9, 2024
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.

Improve UX for multiple split masterIdentities
2 participants