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

Support custom search directories #814

Merged
merged 7 commits into from
Sep 10, 2021

Conversation

ralismark
Copy link
Contributor

@ralismark ralismark commented Aug 26, 2021

This add the -Z search-path option, which make tectonic look in additional paths for files.

Resolves #755.

I'm not that much of a fan of that option name (maybe something like -Zinclude is better?), so if you have any suggestions I'm happy to change this PR.

Todo:

  • --hide support
  • untrusted mode support

@codecov
Copy link

codecov bot commented Aug 26, 2021

Codecov Report

Merging #814 (97d7eac) into master (5fb3976) will increase coverage by 0.72%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #814      +/-   ##
==========================================
+ Coverage   46.36%   47.08%   +0.72%     
==========================================
  Files         146      146              
  Lines       59468    59488      +20     
==========================================
+ Hits        27571    28009     +438     
+ Misses      31897    31479     -418     
Impacted Files Coverage Δ
src/unstable_opts.rs 64.44% <72.72%> (+10.95%) ⬆️
crates/bridge_core/src/lib.rs 65.54% <100.00%> (+0.20%) ⬆️
src/driver.rs 73.76% <100.00%> (+1.43%) ⬆️
crates/geturl/src/reqwest.rs 77.58% <0.00%> (-3.45%) ⬇️
crates/docmodel/src/workspace.rs 73.23% <0.00%> (-1.77%) ⬇️
crates/bundles/src/cache.rs 78.82% <0.00%> (-0.40%) ⬇️
crates/io_base/src/lib.rs 75.94% <0.00%> (+0.42%) ⬆️
crates/io_base/src/filesystem.rs 57.01% <0.00%> (+0.87%) ⬆️
crates/engine_xetex/xetex/xetex-ini.c 88.41% <0.00%> (+1.22%) ⬆️
... and 13 more

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 5fb3976...97d7eac. Read the comment docs.

Copy link
Collaborator

@pkgw pkgw left a comment

Choose a reason for hiding this comment

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

Thank you, this is very nicely done! My only request (besides to stylistic comment I have below) is that you also update the book documentation, in docs/src/v2cli/compile.md. A copy-paste of the help message is basically all that's needed.

Personally, I think that search-path is a great name for this option.

src/unstable_opts.rs Outdated Show resolved Hide resolved
@ralismark
Copy link
Contributor Author

Fixed those two things. There's a couple more things that might be worth fixing

  1. Making this option work with untrusted mode (i.e. by forbidding extra include directories). Currently, we don't have the ability to completely forbid looking up absolute paths so it won't affect anything, but the ability to arbitrarily add search paths seems like the kind of thing we'd want to forbid in a locked-down mode.
  2. Making this work with path hiding i.e. --hide.

@ralismark ralismark marked this pull request as draft August 28, 2021 10:14
@ralismark ralismark marked this pull request as ready for review September 5, 2021 13:49
@pkgw
Copy link
Collaborator

pkgw commented Sep 10, 2021

Thank you! I've pushed a tiny tweak to keep the unstable options alphabetized in the docs, and will merge once CI passes (modulo the arxiv test expected failure).

(Whenever I have a list of things in source that doesn't have any inherent ordering, I try to keep it alphabetized for tidiness.)

edit: FWIW, my feeling is that adding search paths isn't necessarily something that should be blocked with untrusted inputs, but we can always change that later if needed. The person invoking the program is always going to be able to choose what gets input, they just may not have detailed knowledge of what engine functionality that input attempts to invoke. It would definitely have security implications if there was some kind of \AddRandomFilesystemDirectoryToSearchPath primitive that we exposed, but if the set of search paths is only accessible at the program-invocation level, I don't believe it's an issue. Put another way, the person who can add search paths is also the person who can turn off all of our untrusted-input machinery anyway.

@ralismark
Copy link
Contributor Author

The use case for --untrusted is that the input document is untrusted, but how we invoke Tectonic is trusted?

In that case, it's relatively easy to make it always allowed, without completely stripping out security support:

diff --git a/crates/bridge_core/src/lib.rs b/crates/bridge_core/src/lib.rs
index 7aa496b0..1eaeacd1 100644
--- a/crates/bridge_core/src/lib.rs
+++ b/crates/bridge_core/src/lib.rs
@@ -767,7 +767,7 @@ impl SecuritySettings {

     /// Query whether we're allowed to specify extra paths to read files from.
     pub fn allow_extra_search_paths(&self) -> bool {
-        !self.disable_insecures
+        true
     }
 }

@pkgw
Copy link
Collaborator

pkgw commented Sep 10, 2021

@ralismark That's right. There basically isn't any choice to trusting the user that's invoking the code, because they can alter the environment and set up LD_PRELOAD or just compile a new executable with hacked source code, or whatever. Things would be a little different if Tectonic was installed in some kind of multi-user setuid setup or anything, but that's not the case (and it's hard to see how it ever would be).

The Windows vcpkg build has an error that I think must have been introduced with Rust 1.55, which was released today, since the build worked before. I'll merge this and iron that out in another PR.

@pkgw pkgw merged commit 2f12a40 into tectonic-typesetting:master Sep 10, 2021
@ralismark
Copy link
Contributor Author

@pkgw just a note that what's in master doesn't have that above patch to allow extra search paths in --untrusted.

@pkgw
Copy link
Collaborator

pkgw commented Sep 13, 2021

@ralismark Right. Creating a nice opportunity for someone to get their feet wet with a small pull request :-) (Basically I didn't want to wait for CI to run again, which isn't a very good reason but we're being too careful, not too sloppy, so I don't feel too bad about leaving this loose end dangling.)

@ratmice
Copy link
Contributor

ratmice commented Sep 17, 2021

Just some questions/observations, about this as well as untrusted now doing two things.

  1. search-path is an unstable option because it affects reproducibility? I.e. a document requiring the option won't build without that option specified, but paths may differ between systems requiring different paths to be set between different systems?
  2. if search-path was also a Tectonic.toml thing there suddenly would be trusted/untrusted security implications (I could actually use this now with git submodules) this would also seem to make it reproducible assuming only paths relative to the Tectonic.toml were allowed (Which sounds like a job for https://github.com/bytecodealliance/cap-std).

For now i'll just say if 2 becomes a thing the behavior of the untrusted bit is somewhat nuanced.
It might be expected that relative paths is allowed in non-hidden directories during untrusted or something.

There is a worry here that untrusted disables both shell-escape and search-paths, when one might want to disable one without the other. It is a bit concerning that tectonic -Z shell-escape=false foo.tex enables shell-escape (It is actually ignored entirely, see -Z shell-escape=foo, we might want to make that an error (rather than extend shell-escape to accept a bool?).

Edit: Should probably say i'm interested in working on this if any of the above sounds okay to yourselves

@pkgw
Copy link
Collaborator

pkgw commented Sep 17, 2021

@ratmice Thanks for your comments! Here's a smattering of responses ...

  • I think search-path should be unstable mainly because of its reproducibility implications — not so much that the CLI interface might evolve, but frankly I hope that the "unstable" terminology discourages people from using it a bit. (That being said, we're not like Rust where you have to be on a "nightly" train in order to use unstable options at all, so it's a very low level of discouragement.) I want to prevent the CLI from growing a profusion of command-line options and modes, and I believe the best way to do that is to encode that sort of configuration into Tectonic.toml, and I think that the search-path functionality would need to become more sophisticated before exposing it in Tectonic.toml. Because ...
  • I agree with what you say about the implications of search-path going into Tectonic.toml. The key is that this changes the search path from something the user fully controls, to something that the input document can affect. As you say, it would make a ton of sense to allow this if the search path is something relative to the source document directory. (Or the overall multi-document workspace, if/when we flesh out that concept.)
  • I tried to design the security model so that it can be a bit more fine-grained than a boolean trusted/untrusted if needed. The vision is that there should always be a global untrusted mode that means "prevent this document from doing anything naughty that we can possibly think of", but if there's a use-case for allowing a subset of potentially-dangerous options, we should make it possible.
  • I agree that if we accept -Z shell-escape=false with a meaning of "enable shell-escape", that's bad.

So in terms of working on things:

  • A fix to prevent surprises of the form -Z shell-escape=false would definitely be welcome
  • Addition of support for search-path in Tectonic.toml would also be very welcome, with the restriction that in untrusted mode the search path needs to be a subdirectory of the Tectonic.toml file. (Or maybe one level of parent directory since we don't support a "workspace" concept yet, but you want to prevent processing of ../../../../../../../../../../etc/shadow. And note that the default is trusted mode!)

Finally, I'll mention a related but distinct idea. I'd like to eventually add a reproducible = True flag in Tectonic.toml that would indicate that the document is intended to be fully reproducible. This would, e.g., cause the engine to reject an absolute search-path setting even if the document is trusted. Eventually we'd also reject uses of system fonts not available in the bundle, require that locale-dependent settings like the paper size be specified explicitly, and so on. I think this would be a nice way to promote reproducible documents without being too restrictive. In principle reproducibility and trust are separate issues, but often non-reproducible = system-dependent = access outside of the document sandbox = trust implications.

This was referenced Oct 21, 2021
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.

User-configurable search path(s) for .sty files, etc. ("-Z searchpath=..."?)
3 participants