-
Notifications
You must be signed in to change notification settings - Fork 13
🤖 Cursor: Implement plain text auth cache fall back on headless linux #437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
is this enough to ship the .deb package again for ubuntu ? and for it to work over github codespaces ssh terminal ? |
|
This would be a step forward into fixing microsoft/ado-npm-auth#71 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a welcome change that's been a long time coming. Thanks for working on this!
I don't wish to hold up this PR, but I do have a few questions that might be worth briefly entertaining.
| /// <param name="directoryPath">The directory path to set permissions for.</param> | ||
| private void SetDirectoryPermissions(string directoryPath) | ||
| { | ||
| if (IsLinux()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already do an if Linux check further up the stack before we call this.SetupPlainTextCache. This method is only applicable to Linux which also tells me that maybe it could be a compile-time choice.
| } | ||
| }; | ||
| process.Start(); | ||
| process.WaitForExit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This alone does not account for all errors and might pass silently. For example, if the directory (somehow) has the wrong ownership you might receive an error like
chmod: changing permissions of '/home/rsiemens/.azureauth': Operation not permitted
I think to capture an edge case like this you might need to also check process.ExitCode for a non-zero status.
| /// <param name="filePath">The file path to set permissions for.</param> | ||
| private void SetFilePermissions(string filePath) | ||
| { | ||
| if (IsLinux()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here on the if Linux check. It seems odd to me that it should be available and yet be a no-op on a non-Linux system. Perhaps not a big deal but could be a small improvement. 🤷♂️
| try | ||
| { | ||
| // Set directory permissions to 700 (user read/write/execute, no permissions for group/others) | ||
| var process = new System.Diagnostics.Process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last time I checked AzureAuth was targeting .NET 8. I think this means it would be possible to use the File.SetUnixFileMode method and avoid the need to spawn a subprocess.
var mode = UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute;
File.SetUnixFileMode(directoryPath, mode);This will work for both files and directories, so you could delete both the SetDirectoryPermissions and SetFilePermissions methods in favor of this approach.
Incidentally, it also solves the problem of not knowing whether the call to chmod was successful when the process returns. If there is an error changing the permissions a System.IO.IOException will be thrown.
| /// <returns>True if headless Linux environment, false otherwise.</returns> | ||
| private static bool IsHeadlessLinux() | ||
| { | ||
| // Check if DISPLAY environment variable is not set or empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't yet know for certain if this is true, but I'm not sure that a Wayland environment is required to have $DISPLAY set and might actually just have $WAYLAND_DISPLAY set. As written, I wonder if this would cause a native Wayland environment to prematurely be detected as headless. Was this tested in a Wayland environment?
| } | ||
|
|
||
| // Check if WAYLAND_DISPLAY is not set or empty | ||
| var waylandDisplay = Environment.GetEnvironmentVariable("WAYLAND_DISPLAY"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be reading the logic here wrong, but if I'm on an X11-only environment and $DISPLAY is set, but $WAYLAND_DISPLAY is not set then this will report the environment as headless. I'm not sure if that's correct.
|
|
||
| // Plain text cache fallback for headless Linux | ||
| private const string PlainTextCacheDir = ".azureauth"; | ||
| private const string PlainTextCacheFileName = "msal_cache.json"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you foresee any issues with the fact that we use msal_{tenantId}.cache for the normal cache filename, but just msal_cache.json for the plaintext fallback? Do we not need to worry about different tenants in this way for the plaintext case?
| } | ||
|
|
||
| // Create or ensure cache file exists with proper permissions | ||
| if (!File.Exists(cacheFilePath)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot claims that just checking for existence might be insufficient and that the file should also be a regular file, not a symlink.
Its rationale feels a little flimsy to me in that technically an attacker could pre-create a symlink and permissions changes would just affect the symlink then, not the target file which could still be readable. IMO, if you have an attacker who can do that you're already hosed and no amount of directory permissions is gonna save you.
What do you think?
| var cacheFilePath = Path.Combine(cacheDir, PlainTextCacheFileName); | ||
|
|
||
| // Create directory if it doesn't exist | ||
| if (!Directory.Exists(cacheDir)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot also suggests that the directory should be unconditionally set to 700 as the directory could pre-exist with more a more permissive mode and that wouldn't be appropriate. 🤷♂️
| @@ -0,0 +1,116 @@ | |||
| # Linux Headless Cache Fallback Implementation Summary | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this file need to exist? It seems like it might have been useful for Cursor or immediately afterward for the PR author, but I'm not sure it belongs in IMPLEMENTATION_SUMMARY.md at the root of the repo. If anything it belongs in docs/ and there's already a linux-headless-cache-fallback.md document. Perhaps some of the contents here ought to be merged with that document?
| @@ -0,0 +1,156 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is useful for manually validating the plaintext cache fallback, but I'd like to suggest it belongs in a tests/ directory, not the repo root.
| # Function to simulate headless environment | ||
| simulate_headless() { | ||
| echo "Simulating headless environment..." | ||
|
|
||
| # Save original environment variables | ||
| local original_display="$DISPLAY" | ||
| local original_wayland_display="$WAYLAND_DISPLAY" | ||
|
|
||
| # Unset display variables to simulate headless environment | ||
| unset DISPLAY | ||
| unset WAYLAND_DISPLAY | ||
|
|
||
| echo "Environment variables:" | ||
| echo " DISPLAY: ${DISPLAY:-'not set'}" | ||
| echo " WAYLAND_DISPLAY: ${WAYLAND_DISPLAY:-'not set'}" | ||
|
|
||
| if is_headless; then | ||
| echo "✓ Environment is correctly detected as headless" | ||
| else | ||
| echo "✗ Environment is not detected as headless" | ||
| fi | ||
|
|
||
| # Restore original environment variables | ||
| export DISPLAY="$original_display" | ||
| export WAYLAND_DISPLAY="$original_wayland_display" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function doesn't do anything in the context of AzureAuth running. All state change is limited to the scope of this function.
I don't think that simulating a headless environment by setting/unsetting some environment variables and testing that we've unset them helps us test AzureAuth.
Unless I'm misreading this, it guarantees that if run on a graphical Linux environment a headless test won't be executed with the correct environment variable state.
|
|
||
| echo "=== Test Complete ===" | ||
| echo "" | ||
| echo "Note: This script tests the infrastructure for the cache fallback." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I misunderstood the point of this file. I think it was for integration testing at first. This script doesn't call AzureAuth at all.
Now that I look at this I think this is just testing that the Linux environment this is currently running would allow the user to
- Create environment variables.
- Create files.
- Change file permissions.
I'm not sure that I see sufficient utility in that to think this file needs to be included.
Head branch was pushed to by a user without write access
I've now tested this change on an Ubuntu WaveSpace VM and it works 🥳 !
Since Cursor has been blocked internally at Microsoft. I've used my personal (@kyle-rader) OSS account, from my personal device, using my personal Cursor Pro subscription to have a Cursor background agent implement this feature.
Prompt: