covert-encryption / covert

An encryption format offering better security, performance and ease of use than PGP. File a bug if you found anything where we are worse than our competition, and we will fix it.
40 stars 10 forks source link

Changes for armor_max_size and tty_max_size. #59

Closed rocketdey closed 2 years ago

rocketdey commented 2 years ago

This commit closes #26.

codecov[bot] commented 2 years ago

Codecov Report

Merging #59 (195b374) into main (b67fd60) will increase coverage by 1.15%. The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #59      +/-   ##
==========================================
+ Coverage   61.59%   62.74%   +1.15%     
==========================================
  Files          17       17              
  Lines        1997     1997              
  Branches      454      454              
==========================================
+ Hits         1230     1253      +23     
+ Misses        627      606      -21     
+ Partials      140      138       -2     
Impacted Files Coverage Ξ”
covert/__main__.py 62.69% <25.00%> (+2.06%) :arrow_up:
covert/cli.py 57.91% <50.00%> (+5.58%) :arrow_up:
covert/util.py 94.91% <100.00%> (+3.68%) :arrow_up:
covert/archive.py 64.68% <0.00%> (+0.69%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update 236641d...195b374. Read the comment docs.

covert-encryption commented 2 years ago

Now the coverage on files is displayed correctly. You could add this test to cover your change in cli.py and also increase coverage elsewhere because this should hit different parts of the code than the other tests.

def test_end_to_end_shortargs_armored(capsys, tmp_path):
  from covert.__main__ import main
  import sys
  fname = tmp_path / "crypto.covert"

  # Encrypt foo.txt into crypto.covert
  sys.argv = "covert -eRoa tests/data/foo.txt tests/keys/ssh_ed25519.pub".split() + [ str(fname) ]
  ret = main()
  cap = capsys.readouterr()
  assert not ret
  assert not cap.out
  assert "foo" in cap.err

  # Decrypt with key
  sys.argv = "covert -di tests/keys/ssh_ed25519".split() + [ str(fname) ]
  ret = main()
  cap = capsys.readouterr()
  assert not ret
  assert not cap.out
  assert "foo.txt" in cap.err
covert-encryption commented 2 years ago

If you wish, also add another test that writes a 31 MiB test file to tmp_path / "test.dat", then encrypts it with armoring --armor, also using --pad 0 so that it should be slightly below the 32 MiB limit, and try if that can still be extracted.

Also in that same test, you could put -- as the second last argument, and put the input file name as the last argument, to test that feature of the argparser. In this case the name of your test file could instead of test.dat be -test- just to see if that works as intended.

covert-encryption commented 2 years ago

Thanks for the armor/tty changes and for writing useful tests!

That test failure is a bit odd because AFAIK we never close stderr, but pytest could. I'll check if I can dig into it a bit deeper, but separating that test to another test function would already help narrowing it down.

covert-encryption commented 2 years ago

As a workaround, you could also try passing covert argument --debug which prevents it from catching this error, and then you can catch it directly in the test by:

with pytest.raises(ValueError) as excinfo:
  main()
assert "the error message" in str(excinfo.value)

Still, we need to get to the bottom of stderr being closed because that by itself constitutes a bug.

covert-encryption commented 2 years ago

You need these changes to make the tests work properly. Afterwards you also don't need to import covert inside each test functions but can just do the imports at the top of test_cli.py like imports are normally done. This is a diff that you can apply to your files by git apply, or just do the changes by hand because that's probably faster LOL.

In short, we must not from sys import stderr, stdout but rather import sys and use sys.stderr etc, so that pytest can wrap them properly.

diff --git a/covert/__main__.py b/covert/__main__.py
index 5f1e7f6..5b20e51 100644
--- a/covert/__main__.py
+++ b/covert/__main__.py
@@ -1,6 +1,5 @@
 import os
 import sys
-from sys import stderr, stdout
 import colorama
 import covert
 from covert.cli import main_benchmark, main_dec, main_enc
@@ -89,7 +88,7 @@ def argparse():
   av = sys.argv[1:]
   if not av or any(a.lower() in ('-h', '--help') for a in av):
     first, rest = cmdhelp.rstrip().split('\n', 1)
-    if stdout.isatty():
+    if sys.stdout.isatty():
       print(f'\x1B[1;44m{first:78}\x1B[0m\n{rest}')
     else:
       print(f'{first}\n{rest}')
@@ -111,9 +110,9 @@ def argparse():
   elif av[0] in ('bench', 'benchmark'):
     args.mode, ad, modehelp = 'benchmark', benchargs, f"{hdrhelp}"
   else:
-    stderr.write(' πŸ’£  Invalid or missing command (enc/dec/benchmark).\n')
+    sys.stderr.write(' πŸ’£  Invalid or missing command (enc/dec/benchmark).\n')
     sys.exit(1)
-  
+
   aiter = iter(av[1:])
   longargs = [flag[1:] for switches in ad.values() for flag in switches if flag.startswith("--")]
   shortargs = [flag[1:] for switches in ad.values() for flag in switches if not flag.startswith("--")]
@@ -133,7 +132,7 @@ def argparse():
     if not a.startswith('--') and len(a) > 2:
       if any(arg not in shortargs for arg in list(a[1:])):
         falseargs = [arg for arg in list(a[1:]) if arg not in shortargs]
-        stderr.write(f' πŸ’£  {falseargs} is not an argument: covert {args.mode} {a}\n')
+        sys.stderr.write(f' πŸ’£  {falseargs} is not an argument: covert {args.mode} {a}\n')
         sys.exit(1)
       a = [f'-{shortarg}' for shortarg in list(a[1:]) if shortarg in shortargs]
     if isinstance(a, str):
@@ -143,7 +142,7 @@ def argparse():
       if isinstance(av, int):
         continue
       if argvar is None:
-        stderr.write(f'{modehelp}\n πŸ’£  Unknown argument: covert {args.mode} {aprint}\n')
+        sys.stderr.write(f'{modehelp}\n πŸ’£  Unknown argument: covert {args.mode} {aprint}\n')
         sys.exit(1)
       try:
         var = getattr(args, argvar)
@@ -156,7 +155,7 @@ def argparse():
         else:
           setattr(args, argvar, True)
       except StopIteration:
-        stderr.write(f'{modehelp}\n πŸ’£  Argument parameter missing: covert {args.mode} {aprint} …\n')
+        sys.stderr.write(f'{modehelp}\n πŸ’£  Argument parameter missing: covert {args.mode} {aprint} …\n')
         sys.exit(1)

   return args
@@ -193,11 +192,11 @@ def main():
     else:
       raise Exception('This should not be reached')
   except ValueError as e:
-    stderr.write(f"Error: {e}\n")
+    sys.stderr.write(f"Error: {e}\n")
   except BrokenPipeError:
-    stderr.write('I/O error (broken pipe)\n')
+    sys.stderr.write('I/O error (broken pipe)\n')
   except KeyboardInterrupt:
-    stderr.write("Interrupted.\n")
+    sys.stderr.write("Interrupted.\n")

 if __name__ == "__main__":
covert-encryption commented 2 years ago

With these changes all tests pass for me

diff --git a/tests/test_cli.py b/tests/test_cli.py
index 1754310..07b8932 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -161,16 +161,14 @@ def test_end_to_end_large_file(capsys, tmp_path):

   # Try encrypting without -o
   sys.argv = f"covert -ea -R tests/keys/ssh_ed25519.pub".split() + [ str(fname) ]
-  with pytest.raises(ValueError) as excinfo:
-    main()
-  assert "How about -o FILE to write a file?" in str(excinfo.value)
+  main()
   cap = capsys.readouterr()
   assert not cap.out
+  assert "How about -o FILE to write a file?" in cap.err

   # Try encrypting with -o
-  sys.argv = f"covert -eaR tests/keys/ssh_ed25519 -o".split() + [ str(outfname), str(fname) ]
-  with pytest.raises(ValueError) as excinfo:
-    main()
-  assert "The data is too large for --armor." in str(excinfo.value)
+  sys.argv = f"covert -eaR tests/keys/ssh_ed25519.pub -o".split() + [ str(outfname), str(fname) ]
+  main()
   cap = capsys.readouterr()
   assert not cap.out
+  assert "The data is too large for --armor." in cap.err