Repository: peopledoc/vault-cli

Issue:
Default umask should be restrictive
If `--umask` is not passed, it should default to `0600`

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/peopledoc/vault-cli/pull/196?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=peopledoc) Report
> Merging [#196](https://codecov.io/gh/peopledoc/vault-cli/pull/196?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=peopledoc) (10d235e) into [master](https://codecov.io/gh/peopledoc/vault-cli/commit/75dcabde33608e66e707f60f1a24cd41ecfcf2e7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=peopledoc) (75dcabd) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/peopledoc/vault-cli/pull/196/graphs/tree.svg?width=650&height=150&src=pr&token=fjUxXKxpOh&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=peopledoc)](https://codecov.io/gh/peopledoc/vault-cli/pull/196?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=peopledoc)

```diff
@@            Coverage Diff            @@
##            master      #196   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          981       977    -4     
  Branches       130       128    -2     
=========================================
- Hits           981       977    -4     
```


| [Impacted Files](https://codecov.io/gh/peopledoc/vault-cli/pull/196?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=peopledoc) | Coverage Δ | |
|---|---|---|
| [vault\_cli/cli.py](https://codecov.io/gh/peopledoc/vault-cli/pull/196/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=peopledoc#diff-dmF1bHRfY2xpL2NsaS5weQ==) | `100.00% <100.00%> (ø)` | |
| [vault\_cli/utils.py](https://codecov.io/gh/peopledoc/vault-cli/pull/196/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=peopledoc#diff-dmF1bHRfY2xpL3V0aWxzLnB5) | `100.00% <0.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/peopledoc/vault-cli/pull/196?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=peopledoc).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=peopledoc)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/peopledoc/vault-cli/pull/196?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=peopledoc). Last update [75dcabd...10d235e](https://codecov.io/gh/peopledoc/vault-cli/pull/196?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=peopledoc). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=peopledoc).


Patch:
diff --git a/docs/howto/permissions.rst b/docs/howto/permissions.rst
index 7c02abc..5486137 100644
--- a/docs/howto/permissions.rst
+++ b/docs/howto/permissions.rst
@@ -6,9 +6,11 @@ use the ``--umask`` option (a value in octal base is expected):
 
 .. code:: console
 
-    $ vault-cli --umask=066 get -o /path/to/secret mysecret
+    $ vault-cli --umask=006 get -o /path/to/secret mysecret
 
-See umask__ for more details on calculating a umask value
+See umask__ for more details on calculating a ``umask`` value. The default
+``umask`` will be ``066``, meaning the file is readable (and writable) by the
+owner only.
 
 .. __: https://en.wikipedia.org/wiki/Umask
 
@@ -18,4 +20,4 @@ Quick crash course:
 - First value controls owner permissions, second value controls group permission,
   third value controls other users permissions
 - 0 is read-write, 2 is read only, 4 is write only, 6 is nothing
-- "Execute" permission cannot be granted through umask
+- "Execute" permission cannot be granted through ``umask``
diff --git a/vault_cli/cli.py b/vault_cli/cli.py
index 8a68744..7b75b0c 100644
--- a/vault_cli/cli.py
+++ b/vault_cli/cli.py
@@ -49,9 +49,8 @@ def set_verbosity(value: int) -> None:
     logger.info(f"Log level set to {logging.getLevelName(level)}")
 
 
-def set_umask(umask: Optional[int]) -> None:
-    if umask is not None:
-        os.umask(umask)
+def set_umask(umask: int) -> None:
+    os.umask(umask)
 
 
 @contextlib.contextmanager
@@ -70,13 +69,11 @@ def print_version(ctx, __, value):
     ctx.exit()
 
 
-def parse_octal(value: Optional[str]) -> Optional[int]:
-    if not value:
-        return None
+def parse_octal(value: str) -> int:
     return int(value, base=8)
 
 
-def click_octal(_, __, value: Optional[str]) -> Optional[int]:
+def click_octal(_, __, value: str) -> int:
     return parse_octal(value)
 
 
@@ -142,7 +139,9 @@ def repr_octal(value: Optional[int]) -> Optional[str]:
 @click.option(
     "--umask",
     callback=click_octal,
-    help="Set umask for newly created files.",
+    default="066",
+    help="Set umask for newly created files. Defaults to files with read-write "
+    "for owner and nothing for group & others",
 )
 @click.option(
     "-v",


Test patch:
diff --git a/tests/integration/test_integration.py b/tests/integration/test_integration.py
index 741e03a..db7e59b 100644
--- a/tests/integration/test_integration.py
+++ b/tests/integration/test_integration.py
@@ -213,8 +213,15 @@ def umask():
     os.umask(current)
 
 
-def test_umask(set_ACD, umask, tmp_path):
+@pytest.mark.parametrize(
+    "flag, expected",
+    [
+        ("", "0o600"),
+        ("--umask=000 ", "0o666"),
+    ],
+)
+def test_umask(set_ACD, umask, tmp_path, flag, expected):
     path = tmp_path / "test_boostrap_env"
-    # umask = 0o066 => permissions = 0o666 - 0o066 = 0o600
-    subprocess.check_output(f"vault-cli --umask=066 get A -o {path}".split())
-    assert oct(path.stat().st_mode & 0o777) == "0o600"
+    # umask = 0o000 => permissions = 0o666 - 0o000 = 0o666
+    subprocess.check_output(f"vault-cli {flag}get A -o {path}".split())
+    assert oct(path.stat().st_mode & 0o777) == expected
diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py
index 9760fce..cf38222 100644
--- a/tests/unit/test_cli.py
+++ b/tests/unit/test_cli.py
@@ -571,7 +571,7 @@ def test_dump_config(cli_runner):
 
     expected_settings = settings.DEFAULTS._as_dict()
     expected_settings.update(
-        {"base_path": "mybase/", "token": "some-token", "verbose": 0, "umask": None}
+        {"base_path": "mybase/", "token": "some-token", "verbose": 0, "umask": "0o066"}
     )
 
     output = yaml.safe_load(result.output)
@@ -870,9 +870,7 @@ def test_ssh_wrong_format_passphrase(cli_runner, vault_with_token, mocker):
     assert result.exit_code > 0
 
 
-@pytest.mark.parametrize(
-    "input, output", [(None, None), ("022", 0o22), ("0o123", 0o123)]
-)
+@pytest.mark.parametrize("input, output", [("022", 0o22), ("0o123", 0o123)])
 def test_parse_octal(input, output):
     assert cli.parse_octal(input) == output