Add file based Azure Arc detection #955
Conversation
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityClient.java
Outdated
Show resolved
Hide resolved
@bgavrilMS : I originally didn't add tests because it is very difficult to modify static/final variables in a test and our mocking framework doesn't allow mocks of low-level Java features like file manipulation, and the PR over in .NET didn't have them so I figured y'all ran into similar issues too. However, in the latest commit I removed the 'final' modifier from the final paths and added some package-private setters, allowing the tests to create temp directories to test the new behavior. We have some similar used-for-tests-only setters for a few other fields that otherwise could be private, and this is basically the only way to have meaningful tests for this file detection. |
| @@ -26,13 +26,20 @@ class AzureArcManagedIdentitySource extends AbstractManagedIdentitySource{ | |||
| private static final String FILE_EXTENSION = ".key"; | |||
| private static final int MAX_FILE_SIZE_BYTES = 4096; | |||
| private static final String WWW_AUTHENTICATE_HEADER = "WWW-Authenticate"; | |||
| private static final String FALLBACK_IDENTITY_ENDPOINT = "http://127.0.0.1:40342/metadata/identity/oauth2/token"; | |||
There was a problem hiding this comment.
| private static final String FALLBACK_IDENTITY_ENDPOINT = "http://127.0.0.1:40342/metadata/identity/oauth2/token"; | |
| private static final String FALLBACK_AZURE_ARC_IDENTITY_ENDPOINT = "http://127.0.0.1:40342/metadata/identity/oauth2/token"; |
There was a problem hiding this comment.
it's a nit, you can choose to ignore
|
|
||
| if (StringHelper.isNullOrBlank(identityEndpoint)) { | ||
| LOG.info("[Managed Identity] Azure Arc was detected through file based detection but the environment variables were not found. Defaulting to known azure arc endpoint."); | ||
| identityEndpoint = FALLBACK_IDENTITY_ENDPOINT; | ||
| } | ||
|
|
||
| String imdsEndpoint = environmentVariables.getEnvironmentVariable(Constants.IMDS_ENDPOINT); |
There was a problem hiding this comment.
Don't you also need to check that imdsEndpoint doesn't exist before using FALLBACK_IDENTITY_ENDPOINT?
There was a problem hiding this comment.
and if neither exists, also set imdsEndpoint = "N/A: himds executable exists";
| private static String WINDOWS_HIMDS_FILEPATH = "%Programfiles%\\AzureConnectedMachineAgent\\himds.exe"; | ||
| private static String LINUX_HIMDS_FILEPATH = "/opt/azcmagent/bin/himds"; |
There was a problem hiding this comment.
nit: should you define a custom object instead? This is what we do in msal-node:
type FilePathMap = {
win32: string;
linux: string;
};
export const SUPPORTED_AZURE_ARC_PLATFORMS: FilePathMap = {
win32: `${process.env["ProgramData"]}\\AzureConnectedMachineAgent\\Tokens\\`,
linux: "/var/opt/azcmagent/tokens/",
};
export const AZURE_ARC_FILE_DETECTION: FilePathMap = {
win32: `${process.env["ProgramFiles"]}\\AzureConnectedMachineAgent\\himds.exe`,
linux: "/opt/azcmagent/bin/himds",
};
|
|
||
| String osName = System.getProperty("os.name").toLowerCase(); | ||
|
|
||
| if (osName.contains("windows")) { |
There was a problem hiding this comment.
Can "windows" and "linux" be saved as constants in a constants file? Surely they are used elsewhere, or will be? Otherwise, if you implement my suggestion above, you could get the file path via:
// get the expected Windows or Linux file path of the himds executable
const fileDetectionPath: string =
AZURE_ARC_FILE_DETECTION[process.platform as keyof FilePathMap];
|
You might find msal-node's implementation of this feature interesting. It can be found in this file. |
Fixes the issue described in #846, and is similar to the fix in MSAL .NET: AzureAD/microsoft-authentication-library-for-dotnet#4856
This PR adds file-based detection of Azure Arc, in addition to the current detection based on environment variables. If the files are found but the identity endpoint variable is not set, we default to a known endpoint.