bootstrap: allow /dev/fd/<fd> paths for config files

[banks]

Signed-off-by: Paul Banks banks@banksco.de

Description:

As noted in #7528 the newly added sanity checking of possibly sensitive file paths prevents legitimate usage of passing bootstrap via a non-CLOEXEC file descriptor from a generator helper that execs Envoy.

This PR relaxes the validation such that any path resolving to a canonical path with the prefix /dev/fd/ is considered valid.

Risk Level: Low
Testing: Unit test case is added that was failing before the change and passes afterwards. In addition I've manually verified that the old behaviour of allowing /dev/fd/ paths works with my dev binary.
Docs Changes: None, I didn't find this limitation documented currently.
Release Notes: N/A (I didn't see the original change to make /dev invalid in 1.10 notes so I assume this doesn't warrant going in either.)

Fixes #7258

[banks]

Hmm the test failure seems legit - at least filesystem_impl_test is failing in CI. Passed on my machine. I'll take a look.

[lizan]

@banks I guess it is due to on some system {/dev/fd,/0} is a symlink, so the canonical path is outside of it. on my Linux /dev/fd/0 points to /dev/pts/4 (where it links to depends on the process I think.

[banks]

Thanks, I think you’re right. I guess that makes this not the simple fix I hoped. I’ll take another look next week but might need to abandon this if the list of possible places is large across different systems.

…

On 14 Jun 2019, at 22:21, Lizan Zhou ***@***.***> wrote: @banks I guess it is due to on some system {/dev/fd,/0} is a symlink, so the canonical path is outside of it. on my Linux /dev/fd/0 points to /dev/pts/4 (where it links to depends on the process I think. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[banks]

This seems to be the simplest approach here rather than trying to research all the different OS implementations of this path and white list every possible ending point.

I'm not sure if there are any specific downsides of bypassing realpath here?

[htuch]

Sorry for the delay, been catching up on backlog after OOO. I think this is fine for now, but can you leave a TODO to switch to https://en.cppreference.com/w/cpp/filesystem/absolute when we have C++17? The main downside of what you have is that there is no relative path normalization, which we want independent of symlink following.
/wait

[banks]

Added the TODO. I hope this is appropriate - I added it under your username as it seems to be more likely you will have context for this later than I will!

[banks]

/retest

[repokitteh-read-only]

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #7279 (comment) was created by @banks.

see: more, trace.

[banks]

Coverage test fail downloading some dependency...

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[banks]

@htuch since you were assigned on the issue, would this be of interest to merge? Thanks!

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[banks]

Hey @htutch, is this TODO what you meant? Thanks!

…

On 6 Jul 2019, at 11:15, stale[bot] ***@***.***> wrote: This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[htuch]

Thanks!

[htuch]

@banks yep. Can you squash, fix DCO and force push? I can merge when CI DCO check passes.

[banks]

@htuch sorry, been travelling this week I think this should all be good to go now once CI completes.

[htuch]

Thanks!