classification
Title: Expose a way to enable os.symlink on Windows
Type: behavior Stage: resolved
Components: Extension Modules, Windows Versions: Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brian.curtin Nosy List: amaury.forgeotdarc, brian.curtin, eric.smith, jaraco
Priority: normal Keywords: needs review, patch

Created on 2010-07-22 19:58 by brian.curtin, last changed 2010-12-29 02:05 by brian.curtin. This issue is now closed.

Files
File name Uploaded Description Edit
self-omitting-symlink.patch jaraco, 2010-08-01 14:48 review
issue9333.diff brian.curtin, 2010-08-02 20:45 review
issue9333_v2.diff brian.curtin, 2010-08-13 18:12 review
issue9333_v3.diff brian.curtin, 2010-12-28 01:47
Messages (23)
msg111213 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-07-22 19:58
As it currently stands, the possibility exists that some users might not have the SeCreateSymbolicLinkPrivilege privilege enabled (depending on security settings, corporate policy, etc). There should be some method of enabling that privilege outside of the way we do it in the tests (using ctypes).

Attached is a quick prototype...no docs or tests yet. If you don't have the privilege enabled, "os.enable_symlink()" will attempt to enable it for you (True if successful, False if not).


For the security conscious: "The AdjustTokenPrivileges function cannot add new privileges to the access token. It can only enable or disable the token's existing privileges"
msg111222 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2010-07-22 22:18
I think we should consider simply calling this function before running os.symlink. It would be nice if the API were as compatible as possible on both unix and Windows.

My worry is that where code that works on unix systems is simply:

    os.symlink(...)

But now to support symlinks on Windows, one must write:

    if hasattr(os, 'enable_symlink') and not os.enable_symlink():
        raise WindowsError(...)
    os.symlink(...)

Maybe instead of adding os.enable_symlink, Python should include the above logic before attempting to create a symlink and raise an exception if it fails? This would provide a consistent API across platforms for the most common use-case.
msg111225 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-07-22 23:06
That's a way better idea. It would also cut down some of the code in Lib/test/symlink_support.py. I'll take a whack at that and see how it looks.
msg111226 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-07-22 23:38
Wouldn't you have to set this, then restore it? This would then open a non thread-safe race condition, assuming this is a per-process setting, not a thread-local setting.

Not that I'm necessarily opposed, but it's an issue.
msg112323 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2010-08-01 13:40
I'm not sure the setting has to be restored. All that's being added is an attempt to assign the symlink privilege to the current process token.

I would consider to attempt to assign the privilege when the Python process starts up (or when os is loaded) - and if the privilege can't be assigned, remove the .symlink function from os. This change could be coupled with also removing .symlink if running on XP (rather than raising the NotImplementedError).

This approach would avoid the need to expose additional functions, and most importantly would work best with existing implementations -- namely, that if os.symlink exists, one can create symlinks.
msg112335 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2010-08-01 14:48
I'm attaching this patch for discussion: Here's what I had in mind for addressing the NotImplementedError when calling os.symlink on Windows XP. During the posixmodule initialization, if the system does not have the CreateSymbolicLink API call, the function is never added to the module. This will greatly simplify the tests and will limit the impact of this new capability on Windows XP.

This function could be extended to do the same for privilege testing.

Caution - I have not tested the patch, only written it for discussion purposes.

What do you think of this approach to hide the NotImplementedError? If there's no objection, I'll flesh out the complete implementation, update the tests and documentation, and resubmit another patch.

How about using this technique to hide the function when the privilege isn't present?

The one objection I could see w.r.t. privileges is the privilege could be granted after the module is initialized - meaning that a given process would need to have the privilege granted before importing the os module.
msg112540 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-08-02 20:45
Here's a patch implementing a similar idea to what Jason mentioned, complete with the test updates and code removals. It initially adds win_symlink as "_symlink", and on module initialization it will be renamed to "symlink" if the privilege is available.

There are two test failures in test_tarfile that I'll have to work out. Other than that, how does this look?
msg112542 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-08-02 20:48
Forgot to mention: I've only run this on Win7 at the moment. I'll need to take a look at how this works on older Windows.
msg112546 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2010-08-02 20:56
I love it -- especially all of the '-' lines in the patch! Good work. Is it conceivable that a Windows XP user would have that privilege (and thus would have access to a non-functional os.symlink function)?

One thing I particularly like is this provides an interface (os._symlink) that developers can use if they expect privileges to change at runtime, but for the majority of use cases, the os.symlink interface is otherwise largely consistent.
msg112554 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-08-02 21:31
I'll have to investigate the possibility of the privilege occurring on XP -- I'm doubtful that it exists there, but I'll confirm.

Currently "os._symlink" is not exposed -- it gets swallowed up in Lib/os.py in the "nt" section starting on line 55 (it is available as nt._symlink, though). This is another point I need to confirm, but I don't think a process' available privileges can change during runtime, or at least I'm not familiar with that. For that reason, I just do the "enable_symlink()" on init and what happens there is what stays for the lifetime of the interpreter.

If available privileges can in fact change - and I'm not sure how we'd test that - "enable_symlink()" would have to be exposed.
msg113804 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-08-13 18:12
I've tried changing privileges for a user and I'm not seeing that they get reflected in real-time while an application is running. Maybe I'm not doing it right, but I'm not seeing it. I'm also not able to find anything about that being supported or anyone else trying this...actually, there's very little privilege related info anywhere outside of "these are the available privileges".

I'll spend a little more time and see if I can channel any sysadmin types to see if they know anything about this. It's looking like we'll just have to try enabling on module loading and if it works, great, otherwise there's no os.symlink for the lifetime of the interpreter.

Attached is a patch that works safely on XP/Server 2003 where there is no symlink possibility. It still has that test_tarfile error which I'll get to next.
msg123097 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-12-02 18:33
Fixed in r86935.

Tests pass on the following setups:
- Windows 7 (regular user - no symlink privilege)
- Windows 7 (administrator + symlink privilege)
- Windows Server 2003 (no symlink abilities)
- Arch Linux (just a sanity check)

I'm going to create a follow-up issue to explain how this works for the Windows part of the FAQ. There is documentation, but beginners could probably use some tips and explanation.
msg123145 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-12-02 23:38
So the presence of os.symlink depends on some dynamic privilege?

It seems to me that it's the first time in Python. For example, os.chroot() is always available on Unix, even when the user is not root.  Of course the call will fail at runtime.

Why not simply raise an exception when the user has not enough privileges? (I mean OSError or WindowsError of course, not AttributeError)
msg123149 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-12-03 00:31
> So the presence of os.symlink depends on some dynamic privilege?

Yes.


> Why not simply raise an exception when the user has not enough
> privileges? (I mean OSError or WindowsError of course, not AttributeError)

My thinking was that anyone writing cross-platform code which uses symlink in any way is already doing hasattr(os, "symlink"), and if they get a symlink attribute, it should work. With an exception they would have to add an additional try/except for the common case that os.symlink would fail due to lack of privilege on Windows.

I suspect that most people are not running with the required privilege, as evidenced by a look around the web at how others have written and tested this area of code in their applications. Even if someone has an account with administrator-level access, the command prompt starts up with regular privileges, so even those users (e.g., myself) would experience os.symlink raising an exception. Until the application is started explicitly with administrator privileges by an account blessed with access to the symlink privilege does the os.symlink even provide value.

This was noticed in virtualenv3 right off the bat when the first os.symlink checkin happened (see msg112322). They do the hasattr check and go ahead expecting it to work, and it would not work in their case no matter what checks they would do. I've seen other applications setup to do the same thing.


In the end, I'd rather not make people do this:

if hasattr(os, "symlink"):
    if not os.path.exists(dest):
        try:
            os.symlink(src, dest)
        except OSError:
            print("no privilege")

but instead allow them to do what they are likely already be doing:            

if hasattr(os, "symlink"):
    if not os.path.exists(dest):
        os.symlink(src, dest)


For new uses of os.symlink on Windows versions which support it, it may appear a bit unorthodox. I accept that, and I wish it could "just work", but we're given a complex set of hoops to jump through in order to make this usable in any case. I made my decision based on the small percentage of times where this functionality is even available, coupled with existing usage of the function.
msg123289 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) Date: 2010-12-03 20:01
Thanks and good work, Brian.

I think ,though, I'm leaning toward agreeing with Amaury on the presence of the symlink attribute in os.

I can easily see the justification for hiding it in legacy environments (Windows XP & Server 2003), where the relevance diminishes over time, but since we're talking about Python 3, with limited adoption, I'm inclined to suggest it's better to err on the side of breaking existing code and getting the code right, rather than backward compatibility.

To me, the test `hasattr(os, 'symlink')` does not effectively communicate the nuances of the underlying functionality. It doesn't provide a run-time environment any data on why it may or may not be present.

While it does provide excellent backward compatibility (based on a survey of use-cases), I worry it's not the best solution, and might be undesirable in the long run.

Would it be possible to provide a `can_symlink` or `user_can_symlink` function which would be recommended to replace the `hasattr` test? Perhaps we consider keeping the current implementation, deprecate the use of the hasattr test, and prepare for a change in 3.3 or 3.4 where the symlink method is always present on Windows systems >= 6.0.

I defer to Brian's opinion on this, but did want to share my mild discomfort with the current implementation.
msg123490 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-12-06 18:47
I'll come up with a patch to make the attribute always available, but raise OSError when the privilege is not held.
msg124753 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-12-28 01:47
Here's a patch. I think this works more like what you guys are looking for. Tests pass on Windows 7 and I checked it on a Mac to be sure, and it's good there too.
msg124754 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-12-28 01:51
(hit enter too soon, sorry)

The patch makes os.symlink always available on Windows machines, but it will only have an effect when privileged. Windows XP and Windows 2003 will still receive NotImplementedError, as the underlying calls aren't available there. On Vista and 7 for non-privileged users, OSError will be raised.
msg124766 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-12-28 07:53
The patch looks good, I only have stylistic remarks:
- We normally don't use windows-specific types in CPython code. Please use int instead of BOOL. And C variables are usually lowercase, even module globals. I suggest something like "static int win32_can_symlink;"
- the enable_symlink() function should be "static" as well.
msg124797 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-12-28 14:33
Thanks for having a look.
Checked in with the suggested changes to r87539.
msg124804 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-12-28 17:09
Checked in a small doc update in r87547. Removes the part about os.symlink not being available, and mentions the OSError.
msg124835 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-12-29 01:10
Tests now fail on windows XP:
http://www.python.org/dev/buildbot/all/builders/x86%20XP-4%203.x/builds/3874/steps/test/logs/stdio

os.symlink() may raise NotImplementedError, and test.support.can_symlink() should catch it.
msg124840 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-12-29 02:05
Oops, sorry. Fixed in r87561.
History
Date User Action Args
2010-12-29 02:05:42brian.curtinsetstatus: open -> closed

messages: + msg124840
stage: commit review -> resolved
2010-12-29 01:10:46amaury.forgeotdarcsetstatus: closed -> open

messages: + msg124835
stage: resolved -> commit review
2010-12-28 17:09:15brian.curtinsetmessages: + msg124804
2010-12-28 14:33:37brian.curtinsetstatus: open -> closed

messages: + msg124797
resolution: fixed
stage: commit review -> resolved
2010-12-28 07:53:06amaury.forgeotdarcsetmessages: + msg124766
2010-12-28 01:51:52brian.curtinsetmessages: + msg124754
2010-12-28 01:47:42brian.curtinsetfiles: + issue9333_v3.diff

messages: + msg124753
2010-12-06 18:47:30brian.curtinsetresolution: fixed -> (no value)
messages: + msg123490
2010-12-03 20:01:55jaracosetmessages: + msg123289
2010-12-03 00:31:56brian.curtinsetstatus: closed -> open

messages: + msg123149
stage: resolved -> commit review
2010-12-02 23:38:28amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg123145
2010-12-02 18:33:13brian.curtinsetstatus: open -> closed
resolution: fixed
messages: + msg123097

stage: patch review -> resolved
2010-08-13 18:13:05brian.curtinsetfiles: + issue9333_v2.diff

messages: + msg113804
components: + Windows
keywords: + needs review
2010-08-02 21:31:06brian.curtinsetmessages: + msg112554
2010-08-02 20:56:29jaracosetmessages: + msg112546
2010-08-02 20:48:04brian.curtinsetmessages: + msg112542
2010-08-02 20:45:37brian.curtinsetfiles: + issue9333.diff

messages: + msg112540
stage: patch review
2010-08-02 20:37:47brian.curtinsetfiles: - enable_symlink.diff
2010-08-01 14:48:24jaracosetfiles: + self-omitting-symlink.patch

messages: + msg112335
2010-08-01 13:40:11jaracosetmessages: + msg112323
2010-07-22 23:38:08eric.smithsetmessages: + msg111226
2010-07-22 23:06:30brian.curtinsetmessages: + msg111225
2010-07-22 22:18:34jaracosetmessages: + msg111222
2010-07-22 19:58:21brian.curtincreate