-
Notifications
You must be signed in to change notification settings - Fork 4k
[DO NOT REVIEW] [Stephanie] Fix for Server Challenge Token Security Incident by stefong99 #28204
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
Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
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.
Pull Request Overview
This PR strengthens the security of server-managed identity token acquisition by normalizing and validating the secret file path, handling missing environment variables, and updating related resources and release notes.
- Introduces
IsSecretFilePathValid
to normalize and validate the token file path and replaces inline validation - Adds handling and a new resource string for missing
ProgramData
environment variable - Renames header parsing variable, updates
StorageSyncResources.resx
, and records the fix inChangeLog.md
Reviewed Changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/StorageSync/StorageSync/Properties/StorageSyncResources.resx | Removed obsolete XML entry and added AgentMI_ProgramDataNotFoundError resource |
src/StorageSync/StorageSync/Interop/ManagedIdentity/ServerManagedIdentityUtils.cs | Refactored challenge token parsing into IsSecretFilePathValid and updated naming |
src/StorageSync/StorageSync/ChangeLog.md | Added a note for the security bug fix in token acquisition |
Files not reviewed (1)
- src/StorageSync/StorageSync/Properties/StorageSyncResources.Designer.cs: Language not supported
Comments suppressed due to low confidence (3)
src/StorageSync/StorageSync/Interop/ManagedIdentity/ServerManagedIdentityUtils.cs:275
- [nitpick] The variable name 'wwwHeader' is ambiguous; consider renaming it to something more descriptive like 'challengeHeaderValue'.
var wwwHeader = authenticateHeaderValues.FirstOrDefault();
src/StorageSync/StorageSync/Interop/ManagedIdentity/ServerManagedIdentityUtils.cs:342
- Provide a description for the 'secretFilePath' parameter in the XML doc to explain its purpose and expected format.
/// <param name="secretFilePath"></param>
src/StorageSync/StorageSync/Interop/ManagedIdentity/ServerManagedIdentityUtils.cs:344
- Add unit tests for 'IsSecretFilePathValid' to cover valid token paths, paths outside the allowed folder, and missing 'ProgramData' or 'SystemDrive' scenarios.
private static bool IsSecretFilePathValid(string secretFilePath)
@@ -327,4 +324,7 @@ | |||
<data name="AgentMI_ArcServerNotEnabled" xml:space="preserve"> | |||
<value>>This hybrid server is not Azure Arc-enabled. Please ensure that the Azure Connected Machine agent is installed and the server is connected to Azure Arc.</value> | |||
</data> | |||
<data name="AgentMI_ProgramDataNotFoundError" xml:space="preserve"> | |||
<value>GetEnvironmentVariable failed to find 'ProgramData'</value> |
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 error message is missing a period at the end to match the style of other resource strings.
<value>GetEnvironmentVariable failed to find 'ProgramData'</value> | |
<value>GetEnvironmentVariable failed to find 'ProgramData'.</value> |
Copilot uses AI. Check for mistakes.
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Description
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.md
and reviewed the following information:ChangeLog.md
file(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
.## Upcoming Release
header in the past tense.ChangeLog.md
if no new release is required, such as fixing test case only.