Implement cat-like filtering behaviour for encrypt/decrypt #12112

Merged
merged 7 commits into from Aug 27, 2015

Conversation

Projects
None yet
4 participants
@amenonsen
Contributor

amenonsen commented Aug 26, 2015

As discussed on IRC, this includes some minor functionality-preserving cleanups for VaultEditor, and uses this to implement the stdin/stdout encryption/decryption semantics requested by @bcoca and @abadger.

@abadger

This comment has been minimized.

Show comment
Hide comment
@abadger

abadger Aug 26, 2015

Member

future cleanup note: VaultLib.cipher_name is not a good attribute. It's being used in two ways, I think:

  • as a property of the specific file/data. Since we are able to give different filenames to VaultLib.encrypt()/decrypt() now, having a VaultLib level property specifying the cipher of the last data that went through VaultLib doesn't make sense. Probably better to replace this with a method that returns the cipher_name when given a ciphertext.
  • to tell us what cipher to use to encrypt a file. Because this belongs to the specific file to be encrypted and that can change for each call to encrypt()/decrypt() we probably should pass this in as a parameter to encrypt() instead.

I can't think of any way that self.cipher_name will break current code so we don't have to change it in this PR but it seems conceptually wrong and might be a source of bugs with future code changes.

Member

abadger commented Aug 26, 2015

future cleanup note: VaultLib.cipher_name is not a good attribute. It's being used in two ways, I think:

  • as a property of the specific file/data. Since we are able to give different filenames to VaultLib.encrypt()/decrypt() now, having a VaultLib level property specifying the cipher of the last data that went through VaultLib doesn't make sense. Probably better to replace this with a method that returns the cipher_name when given a ciphertext.
  • to tell us what cipher to use to encrypt a file. Because this belongs to the specific file to be encrypted and that can change for each call to encrypt()/decrypt() we probably should pass this in as a parameter to encrypt() instead.

I can't think of any way that self.cipher_name will break current code so we don't have to change it in this PR but it seems conceptually wrong and might be a source of bugs with future code changes.

@bcoca

View changes

lib/ansible/cli/vault.py
+ if len(self.args) > 1:
+ raise AnsibleOptionsError("At most one input file may be used with the --output option")
+
+ elif len(self.args) == 0:

This comment has been minimized.

@bcoca

bcoca Aug 26, 2015

Member

so even if i try to input from stdin this will error out?

@bcoca

bcoca Aug 26, 2015

Member

so even if i try to input from stdin this will error out?

@abadger

This comment has been minimized.

Show comment
Hide comment
@abadger

abadger Aug 27, 2015

Member

Review and everything here looks good to me. At first I had some questions in the same area as @bcoca did but I think all of the ways of operating that I'd expect are covered. Just not quite the same way as cat:

$ ansible-vault encrypt filename1   # std way we've always called the command
$ ansible-vault encrypt filename1 -o filename2    # New cmdline arg
$ cat filename1 | ansible-vault encrypt -o filename2   # Stdin to named file
$ ansible-vault encrypt filename1 -o - > filename2     # Output to stdout
$ cat filename1 | ansible-vault encrypt filename1 -o - > filename2  # stdin to stdout

There's other UNIX tools that operate this way (using explicit -o- to specify output to stdout) so it looks okay to me. @bcoca does that summary of the options alleviate your concern?

Member

abadger commented Aug 27, 2015

Review and everything here looks good to me. At first I had some questions in the same area as @bcoca did but I think all of the ways of operating that I'd expect are covered. Just not quite the same way as cat:

$ ansible-vault encrypt filename1   # std way we've always called the command
$ ansible-vault encrypt filename1 -o filename2    # New cmdline arg
$ cat filename1 | ansible-vault encrypt -o filename2   # Stdin to named file
$ ansible-vault encrypt filename1 -o - > filename2     # Output to stdout
$ cat filename1 | ansible-vault encrypt filename1 -o - > filename2  # stdin to stdout

There's other UNIX tools that operate this way (using explicit -o- to specify output to stdout) so it looks okay to me. @bcoca does that summary of the options alleviate your concern?

amenonsen added some commits Aug 26, 2015

Simplify VaultEditor methods
We don't need to keep creating VaultLibs everywhere, and we don't need
to keep checking for errors because VaultLib does it already.
Implement cat-like filtering behaviour for encrypt/decrypt
This allows the following invocations:

    # Interactive use, like gpg
    ansible-vault encrypt --output x

    # Non-interactive, for scripting
    echo plaintext|ansible-vault encrypt --output x

    # Separate input and output files
    ansible-vault encrypt input.yml --output output.yml

    # Existing usage (in-place encryption) unchanged
    ansible-vault encrypt inout.yml

…and the analogous cases for ansible-vault decrypt as well.

In all cases, the input and output files can be '-' to read from stdin
or write to stdout. This permits sensitive data to be encrypted and
decrypted without ever hitting disk.
Also support output to stdout with no arguments
This allows "cat plaintext|ansible-vault encrypt > ciphertext".
More helpful prompts from ansible-vault encrypt/decrypt
Now we issue a "Reading … from stdin" prompt if our input isatty(), as
gpg does. We also suppress the "x successful" confirmation message at
the end if we're part of a pipeline.

(The latter requires that we not close sys.stdout in VaultEditor, and
for symmetry we do the same for sys.stdin, though it doesn't matter in
that case.)

abadger added a commit that referenced this pull request Aug 27, 2015

Merge pull request #12112 from amenonsen/vault-stdio
Implement cat-like filtering behaviour for encrypt/decrypt

@abadger abadger merged commit 86b2982 into ansible:devel Aug 27, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abadger

This comment has been minimized.

Show comment
Hide comment
@abadger

abadger Aug 27, 2015

Member

Nice work! I like the new behaviour a lot. thanks @amenonsen !

Merged

Hi!

This has been merged in, and will also be included in the next major release.

If you or anyone else has any further questions, please let us know by stopping by one of the two mailing lists, as appropriate:

Because this project is very active, we're unlikely to see comments made on closed tickets, but the mailing list is a great way to ask questions, or post if you don't think this particular
issue is resolved.

Thank you!

Member

abadger commented Aug 27, 2015

Nice work! I like the new behaviour a lot. thanks @amenonsen !

Merged

Hi!

This has been merged in, and will also be included in the next major release.

If you or anyone else has any further questions, please let us know by stopping by one of the two mailing lists, as appropriate:

Because this project is very active, we're unlikely to see comments made on closed tickets, but the mailing list is a great way to ask questions, or post if you don't think this particular
issue is resolved.

Thank you!

@amenonsen amenonsen deleted the amenonsen:vault-stdio branch Aug 31, 2015

amenonsen added a commit to amenonsen/ansible that referenced this pull request Sep 30, 2015

VaultLib interface cleanups: remove .cipher_name, move .version
As noted in the comments in PR #12112, the cipher_name stored in the
VaultLib conflates the desired cipher for encryption with the cipher
read from an encrypted file header. This change makes the encryption
cipher a parameter to encrypt() and changes _split_header() to return
the extracted values so that decrypt() can use the appropriate cipher
name (which a new helper transforms into a VaultXXX class).

As a consequence, _split_header is renamed to _parse_vault_file, which
also becomes more strict about parsing vault headers. (This also means
that decrypt() doesn't have to check is_encrypted() separately, since
the validation is being done anyway.)

We also move b_version from VaultLib to the individual cipher classes so
that they can manage their own versioning; we also pass the version from
the file header to the cipher's decrypt() method.

This doesn't change any functionality, but it paves the way for easier
format changes without having to introduce ever more cipher classes.

amenonsen added a commit to amenonsen/ansible that referenced this pull request Oct 9, 2015

VaultLib interface cleanups: remove .cipher_name, move .version
As noted in the comments in PR #12112, the cipher_name stored in the
VaultLib conflates the desired cipher for encryption with the cipher
read from an encrypted file header. This change makes the encryption
cipher a parameter to encrypt() and changes _split_header() to return
the extracted values so that decrypt() can use the appropriate cipher
name (which a new helper transforms into a VaultXXX class).

As a consequence, _split_header is renamed to _parse_vault_file, which
also becomes more strict about parsing vault headers. (This also means
that decrypt() doesn't have to check is_encrypted() separately, since
the validation is being done anyway.)

We also move b_version from VaultLib to the individual cipher classes so
that they can manage their own versioning; we also pass the version from
the file header to the cipher's decrypt() method.

This doesn't change any functionality, but it paves the way for easier
format changes without having to introduce ever more cipher classes.

amenonsen added a commit to amenonsen/ansible that referenced this pull request Dec 17, 2015

Various VaultLib interface cleanups
As noted in the comments in PR #12112, the cipher_name stored in the
VaultLib conflates the desired cipher for encryption with the cipher
read from an encrypted file header. This change makes the encryption
cipher a parameter to encrypt() and changes _split_header() to return
the extracted values so that decrypt() can use the appropriate cipher
name (which a new helper transforms into a VaultXXX class).

As a consequence, _split_header is renamed to _parse_vault_header, which
also becomes more strict about parsing vault headers. (This also means
that decrypt() doesn't have to check is_encrypted() separately, since
the validation is being done anyway.)

The earlier _format_output method is renamed to _add_vault_header, and
is no longer responsible for wrapping the output at 80 columns (cipher
classes are now responsible for doing their own formatting; GPG would
need this anyway). Likewise, _parse_vault_header is not responsible for
unwrapping the ciphertext (cipher classes can undo whatever formatting
they originally did).

The cipher classes now do their own versioning. The header stores both
the container format version as well as the individual cipher versions.
For example: $ANSIBLE_VAULT;1.2;AES256;1.2

Also includes various unicode/bytes cleanups by @abadger.

amenonsen added a commit to amenonsen/ansible that referenced this pull request Feb 11, 2016

Various VaultLib interface cleanups
As noted in the comments in PR #12112, the cipher_name stored in the
VaultLib conflates the desired cipher for encryption with the cipher
read from an encrypted file header. This change makes the encryption
cipher a parameter to encrypt() and changes _split_header() to return
the extracted values so that decrypt() can use the appropriate cipher
name (which a new helper transforms into a VaultXXX class).

As a consequence, _split_header is renamed to _parse_vault_header, which
also becomes more strict about parsing vault headers. (This also means
that decrypt() doesn't have to check is_encrypted() separately, since
the validation is being done anyway.)

The earlier _format_output method is renamed to _add_vault_header, and
is no longer responsible for wrapping the output at 80 columns (cipher
classes are now responsible for doing their own formatting; GPG would
need this anyway). Likewise, _parse_vault_header is not responsible for
unwrapping the ciphertext (cipher classes can undo whatever formatting
they originally did).

The cipher classes now do their own versioning. The header stores both
the container format version as well as the individual cipher versions.
For example: $ANSIBLE_VAULT;1.2;AES256;1.2

Also includes various unicode/bytes cleanups by @abadger.

amenonsen added a commit to amenonsen/ansible that referenced this pull request Mar 21, 2016

Various VaultLib interface cleanups
As noted in the comments in PR #12112, the cipher_name stored in the
VaultLib conflates the desired cipher for encryption with the cipher
read from an encrypted file header. This change makes the encryption
cipher a parameter to encrypt() and changes _split_header() to return
the extracted values so that decrypt() can use the appropriate cipher
name (which a new helper transforms into a VaultXXX class).

As a consequence, _split_header is renamed to _parse_vault_header, which
also becomes more strict about parsing vault headers. (This also means
that decrypt() doesn't have to check is_encrypted() separately, since
the validation is being done anyway.)

The earlier _format_output method is renamed to _add_vault_header, and
is no longer responsible for wrapping the output at 80 columns (cipher
classes are now responsible for doing their own formatting; GPG would
need this anyway). Likewise, _parse_vault_header is not responsible for
unwrapping the ciphertext (cipher classes can undo whatever formatting
they originally did).

The cipher classes now do their own versioning. The header stores both
the container format version as well as the individual cipher versions.
For example: $ANSIBLE_VAULT;1.2;AES256;1.2

Also includes various unicode/bytes cleanups by @abadger.

@ansibot ansibot added feature and removed feature_pull_request labels Mar 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment