NetFileCache and LocalRepo

[AlexanderDzhoganov]

• LocalRepo - use a local folder as a CKAN repo
• Replaced Cache with NetFileCache, now everything is cached by url according to Allow file caching by URL, not (just) filename #111
• No tests yet :(
• Doesn't get filename from HTTP headers yet

[pjf]

Reviewing this now. Although if I can ask a favour for future PRs, if there's two unrelated features (eg: NetFileCache and LocalRepo), if they could be made as separate PRs, that would be awesome. (I also understand that sometimes they can't be, if one depends upon the other.)

From what I've looked at so far, these are looking good. :)

[pjf]

Hmm, do we really want this? I might be mis-reading the code, but surely I can do:

cache.CommitDownload(url, "cheese-sandwich");
cache.CommitDownload(url, "vegemite-sandwich");

In other words, I can use the same URL twice, but end up with two entirely different file-names that it caches to. When we try to retrieve from the cache, we could even end up with a different file back, because we allow multiple files to be cached with the same URL.

This might be fine as a private method, but for public methods I'd be much happier if the only thing we supply to the cache is the URL, otherwise we've got all sorts of ways we can end up with inconsistencies.

Alternatively, this might be fine if it ensures that any previous file has been removed that we've cached using that URL.

[AlexanderDzhoganov]

The first commit will remove the temporary file so the second commit will throw.

[pjf]

We seem to be a little bit short on documentation for the new cache class; is there any chance you can add some? It wasn't apparent to me that we must use GetTemporaryPathForURL if we want to be able to store a file in the cache, which had me initially scratching my head as to why it existed.

Likewise, IsCached seems to be mixing its functionality. It returns the cached file in addition to indicating if we've got the file cached or not. I'd love to see them split:

• bool IsCached(Url url)
• string GetCachedFile(Url url) // Returns null on failure

[pjf]

Uh oh! That's going to give us lots of pain on multi-user systems! If /tmp/ckan_temp has been created by one user, then it can't be used by another.

What's more, it may even expose us to symlink attacks. If an attacker places a symlink in /tmp/ckan_temp to point to somewhere else on the system, we'll go start writing files there.

While neither of these are going to keep me awake at night, they're both easily fixed. While we don't have mkdtemp available to us in C#, we can use GetTempFileName to create a temp file that we know did not exist previously, and then delete that and create a directory in its steed.

[Wizarth]

I heartily endorse either system provided or otherwise generated randomized
temp files/folders.

On Sat, Nov 8, 2014 at 2:48 PM, Paul Fenwick [email protected]
wrote:

In CKAN/CKAN/NetFileCache.cs:

+using System.Text;
+
+namespace CKAN
+{
+

• public class NetFileCache
• {

•    private string tempPath = "";
  

•    private string cachePath = "";
  

•    public NetFileCache(string _cachePath, string _tempPath = null)
  

•    {
  

•        if (_tempPath == null)
  

•        {
  

•            tempPath = Path.Combine(Path.GetTempPath(), "ckan_temp");
  

Uh oh! That's going to give us lots of pain on multi-user systems! If
/tmp/ckan_temp has been created by one user, then it can't be used by
another.

What's more, it may even expose us to symlink attacks. If an attacker
places a symlink in /tmp/ckan_temp to point to somewhere else on the
system, we'll go start writing files there.

While neither of these are going to keep me awake at night, they're both
easily fixed. While we don't have mkdtemp available to us in C#, we can
use GetTempFileName to create a temp file that we know did not exist
previously, and then delete that and create a directory in its steed.

—
Reply to this email directly or view it on GitHub
https://github.com/KSP-CKAN/CKAN/pull/282/files#r20049525.

[AlexanderDzhoganov]

This is easily fixable

[pjf]

It looks like our KSP class already clears the cache of files: https://github.com/KSP-CKAN/CKAN/blob/master/CKAN/CKAN/KSP.cs#L180

[pjf]

This is included as part of #296, which also fixes a few issues. :)