Issues relating to FILE_URL

[plwhite]

Describe the bug
Various problems related to specifying FILE_URL. These are pretty trivial to fix, so I am happy to submit a PR if you agree, though I have limited capacity to test it fully (beyond manual test in my environment).

To Reproduce
Specify a FILE_URL, and several things go wrong. There are workarounds for all these, and some are relatively cosmetic, but feel like they would be good to fix.

• There is trace at

  photon-docker/src/entrypoint.py

  Line 24 in a03cfc9

  logger.debug(f"FILE_URL: {config.FILE_URL}")

  , but it is debug level, so FILE_URL is not reported. That makes it seem to have been ignored.

• photon-docker/src/downloader.py

  Line 373 in a03cfc9

  desc=f"Downloading {os.path.basename(destination)}",

  logs the destination path when downloading. However, this is problematic, as the logs are of the local filename, yet imply that they are of the file actually being downloaded. Adding the URL to this log would make it far clearer what is going on (including in cases where the URL is constructed in code).

• The download fails if FILE_URL is specified and SKIP_MD5_CHECK is not, as the code grabs the wrong md5 file. Better to mandate that SKIP_MD5_CHECK must be true if FILE_URL is set (or to change the md5 logic with an explicitly passed in md5 path).

Expected behavior
See above.

photon-docker configuration:

All that I did was explicitly set FILE_URL, and then found the issues above.

System Info

This was run on a linux VM, but that isn't relevant.

[rtuszik]

Thanks for your input here. Just laying out my line of thinking with these.

• I decided not to log the FILE_URL because it may contain sensitive information if someone uses basic auth in the url and their file provider. I guess a middle ground could be logging something like FILE_URL = <set>

• Usually the md5 file is placed with the actual index file. This part of the download logic is missing for the downloads with the FILE_URL set. It may make sense to disable it by default while also adding an environment variable for the md5 url that can be used in conjunction and will lead to the md5 check being enabled when set. Given that anyone setting their own file_url likely controls the actual host as well, the trust factor might less relevant.

Happy to hear your thoughts.

[plwhite]

• Good idea on the security point. I think just logging "FILE_URL was set" would help, though you could perhaps also strip any parameters from the URL before logging it - would that answer the security concern?
• I think disabling the md5 check for when FILE_URL is set would be a good approach.

I think in general the code should log what it is downloading. A simple log once the URL is constructed ought to be sufficient there, and if FILE_URL is set then either log it without parameters or as "REDACTED" or something.

Does that all make sense? I'm happy to put together a PR with these changes if so.

[rtuszik]

Sounds good to me.
Looking forward to your PR.

I won't be too available during the next 2/3 weeks, so I might take a few days to review.

[rtuszik]

FYI, I need to update the contribution guidelines, but please PR against the dev branch.

[plwhite]

PR is at #244