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

antsibull-docs expects dest-dir to exist #140

Closed
evgeni opened this issue Jul 21, 2020 · 4 comments · Fixed by #144
Closed

antsibull-docs expects dest-dir to exist #140

evgeni opened this issue Jul 21, 2020 · 4 comments · Fixed by #144

Comments

@evgeni
Copy link

evgeni commented Jul 21, 2020

% antsibull-docs collection --use-current --dest-dir lol theforeman.foreman                                              
Traceback (most recent call last):
  File "/home/evgeni/Devel/theforeman/foreman-ansible-modules/venv/bin/antsibull-docs", line 8, in <module>
    sys.exit(main())
  File "/home/evgeni/Devel/theforeman/foreman-ansible-modules/venv/lib64/python3.8/site-packages/antsibull/cli/antsibull_docs.py", line 272, in main
    return run(sys.argv)
  File "/home/evgeni/Devel/theforeman/foreman-ansible-modules/venv/lib64/python3.8/site-packages/antsibull/cli/antsibull_docs.py", line 238, in run
    args: argparse.Namespace = parse_args(program_name, args[1:])
  File "/home/evgeni/Devel/theforeman/foreman-ansible-modules/venv/lib64/python3.8/site-packages/antsibull/cli/antsibull_docs.py", line 211, in parse_args
    _normalize_docs_options(args)
  File "/home/evgeni/Devel/theforeman/foreman-ansible-modules/venv/lib64/python3.8/site-packages/antsibull/cli/antsibull_docs.py", line 48, in _normalize_docs_options
    stat_results = os.stat(args.dest_dir)
FileNotFoundError: [Errno 2] No such file or directory: '/home/evgeni/Devel/theforeman/foreman-ansible-modules/lol'

This is unexpected (for me), as I'd think it would create the directory when it does not exist yet.

@abadger
Copy link
Contributor

abadger commented Jul 21, 2020

So this (and a few other strict permission checks in antsibull) are because there's various insecurities in creating directories. The easiest thing to do is to punt that to the user; forcing them to create the directory. (although that's far from perfect... just because they create the directory doesn't mean that they have made sure that it is all secure).

I don't think I'm going to put effort into changing this for quite a while but there are certain things that could be done if someone else wants to look into it before me. We probably need to walk backwards up the directory tree, looking at the ownership and permissions of all the directories to make sure they're only writable by the user. We can stop when we hit either the root directory or the user's home directory. It might be okay to special case the user's home directory and flat out say that any subdirectory of that is okay. Symlinks probably need to be handled differently than real subdirectories in that case, though. I can't think of any other specialcases that could be more permissive but there could be others.

@evgeni
Copy link
Author

evgeni commented Jul 22, 2020

I am not sure which insecurities you're mentioning, after all we're just dumping a ton of RST files in a folder.

But nevertheless, I think at least the traceback shouldn't happen and the user should get a nice one-line message "please create the destination"? Maybe with an explanation what they should take care of to be secure?

@felixfontein
Copy link
Collaborator

felixfontein commented Jul 22, 2020

That traceback is definitely a bug, a few lines below it would have reported the non-existing directory - if it wouldn't have crashed before.

The security problems are the following: if you have a malicious user in your system, and that user owns the directory, or a more top-level directory (that's not checked yet), they could rename something in the path and create a symlink instead to point somewhere where the user running antsibull can write (but the malicious user cannot), resulting in the user running antsibull accidentally writing files to a location where it does not want to write them to.

This probably isn't a problem in many contexts (like on single-user systems, or when running this in your home directory, which should be sufficiently protected), but appears to be a potential problem in the way it is used internally by the ansible team.

@felixfontein
Copy link
Collaborator

felixfontein commented Jul 22, 2020

#144 should fix the crash and improve the error messages.

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 a pull request may close this issue.

3 participants