Portability Issues

[pbertera]

• moved os.chroot() in self.chroot() so one can overwrite the method is os.chroot() isn't supported
• cached the os.getuid() exception
• used the absolute file path for TFTP / HTTP request handling

[psychomario]

This brings up a larger question of "Do we support Windows?". I don't think I have a functional Windows install to test it, and off the top of my head we don't use any platform specifics other than chroot.

[pbertera]

I'm still working on that, I'll advise you when can be merged (if still makes sense to merge).

[psychomario]

Out of interest, what is the justification for changing to absolute paths? After we chroot it doesn't really make much difference.

[psychomario]

You seem to be converting to an absolute path, and then converting back to a relative path. Would it not be simpler to just leave it as relative paths?

[pbertera]

I'm gonna close this PR until I don't have a better solution. I'll come back when I'm ready.

[psychomario]

OK. This is a good idea, I hope you're not discouraged by the number of critical comments. I'm going to spin up a Windows VM to perform some testing so I can identify other areas where we're not portable.

[pbertera]

I closed because I still commit and changing things, I don't wanna waste your time looking at things in movement. I'll re-open it

[pbertera]

More generally speaking about portability: I don't see big issue to make this code portable. IMHO doesn't makes sens to get a big effort in that, so we can achieve the windows support with some limitations (for example path traversal issue). More or less: do you wanna use this code on Windows ? it runs but you are warned: isn't secure and not 100% supported.

[psychomario]

I'm currently investigating how much would need to be changed for Window support. I will have an issue posted shortly and we can decide whether or not the changes are worth it.

[pbertera]

humm.. I'm thinking if would be better to remove the os.chroot from the modules and put into the pypxe-server.py: if you're using PyPXE as a library and instantiate a TFTP and an HTTP server passing to both the netbootDirectory (different than ".") you have an issue because the first thread calling the os.chroot() changes the root directory, so you need to call the first daemon with:

tftp.TFTPD(netbootDirectory="/tmp/netboot")

and the second with:

http.HTTPD(netbootDirectory="/")

this because os.chroot() is affecting the whole process.

IMHO a simpler design would be to remove the os.chroot() from the module and let the caller to handle with that.

[psychomario]

Good catch.

We might be better off completely removing chroot and implementing a path parser inside each module. This removes cross-platform chroot issues, and also means the modules are safely independant. @mmattioli thoughts?

[pbertera]

IMHO we can leave the chroot into the command line launcher, I don't see issues here. about the path parsing inside the module we can add a method (something like filter_request()) evaluating the request, If the method returns false the client receives a file not found error. So one can overwrite it and parse the request path.

In case we leave the chroot() into the launcher we can implement a dummy filter_request() returning True

[psychomario]

We'll need to implement the parser before we remove the chroot. HTTP relies on chroot to handle the requests.

[pbertera]

Why? Removing the chroot and leaving the chdir the file path can be built relatively from the current dir using the request.. Or I'm missing something?

Bertera Pietro http://www.bertera.it

[psychomario]

Currently for HTTP we recieve requests such as "/memdisk". The chroot means that that is a valid file path, without the chroot it isnt. We could make it relative by prefixing it with a . but if we're implementing a parser we might as well do that first so as not to leave it open to traversal.

[mmattioli]

The goal of the PR is to make this portable to Windows systems or for some variations or *nix that we haven't already covered?

If this is for Windows, I think it's opening a can of words we really don't want to open:

• Windows firewall is a total disaster and will definitely cause roadblocks
• Almost any/all anti-virus programs will probably have a meltdown if we ran this and it saw traffic coming in/out
• UAC is an abomination and will definitely cause issues
• Windows uses NTFS file permissions. Need I say more?
• The directory issue is one of the biggest, unless we can figure out a simple try/except at the very beginning of each service (so we don't have to do it again in pypxe-server.py) then it's going to be a total mess.

These are just a few things that comes to mind when I think about making this portable for Windows. On top of developing the portability fixes for the current implementation we have, we would have to test every future implementation on Windows to make sure we haven't done something that can't be done in Windows.

All of the above being said, I'm definitely all for simplifying things and/or fixing the relative/absolute directory implementation if it can be done more cleanly but I really don't think we should be doing so with a Windows-based mindset.

[psychomario]

Those are some good points. We can probably disregard Windows. Even so, we need to avoid chroot as it affects the process and this is counter to the independence of the modules.

[mmattioli]

@psychomario like I said I'm all for improving portability in that sense; cleaning up things such as chroot and whatnot enhance portability for the project both as a whole and for module independence when used as a library. I just want it to be clear that we shouldn't be doing it with a Windows-based mindset, we should be doing it with the mindset of having an overall cleaner implementation.

[psychomario]

That's a fair shout, and we should probably stick with that mindset going forward. However, it only took me modifying a few lines to get it working fine on Windows. And half of those are bugs anyway. But often supporting Windows for more advanced functionality ends in horrible cross platform hacks, which everyone can agree should be avoided.

[mmattioli]

Closing this as we've addressed the issue and come to a conclusion as to how we should handle portability moving forward.