Skip to content
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

Acquire token for IMDS SAMI #500

Merged
merged 35 commits into from
Sep 25, 2024
Merged

Conversation

4gust
Copy link
Collaborator

@4gust 4gust commented Sep 2, 2024

Add ManagedIdentity Class and acquireToken Function for SystemAssigned Identity

Overview

This PR introduces a new ManagedIdentity class in the Go codebase, specifically designed to handle operations related to managed identities in Azure. The primary feature added in this PR is the acquireToken function within the ManagedIdentity class, which currently supports only SystemAssigned managed identities.

Changes

  • ManagedIdentity:

    • Created a new class named ManagedIdentity to encapsulate managed identity operations.
    • The class is intended to support different types of managed identities (SystemAssigned, UserAssigned) in the future, but for now, it only supports SystemAssigned identities.
  • acquireToken Function:

    • Implemented an acquireToken function in the ManagedIdentity class.
    • This function is responsible for acquiring an access token for the SystemAssigned managed identity.
    • The function makes a call to the Azure Instance Metadata Service (IMDS) endpoint to fetch the token.

Implementation Details

  • SystemAssigned Identity:

    • The acquireToken function currently assumes that the managed identity is SystemAssigned.
    • It retrieves the access token using the endpoint: http://127.0.0.1:40342/metadata/identity/oauth2/token.
  • Error Handling:

    • The function includes basic error handling to manage cases where the token cannot be acquired.
    • If the HTTP request to the IMDS endpoint fails, the function returns an error with appropriate context.

Testing

  • Unit Tests:
    • Added unit tests for the acquireToken function to ensure it behaves as expected when fetching tokens for a SystemAssigned identity.
    • Tests include scenarios for successful token acquisition and error cases (e.g., network failure).

Checklist

  • Code compiles successfully.
  • Unit tests added and pass locally. (more work to be done here)

Notes

  • This is the initial implementation of the ManagedIdentity class with limited functionality. Please review and suggest improvements, especially concerning the design for future extensibility.

4gust added 5 commits August 27, 2024 23:53
Added a simple version of getting token and printing it
reformatting code.
Added tests and implementation for SAMI IMDS
Reverted changes in the test app
Formatting changes
Added method for UAMI
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Show resolved Hide resolved
apps/managedidentity/managedidentity_test.go Outdated Show resolved Hide resolved
@bgavrilMS bgavrilMS changed the title Acquire token for IMDB SAMI Acquire token for IMDS SAMI Sep 4, 2024
Updated the some code and cleaned up some comments and print statement
Updated the key for the resource

Co-authored-by: Charles Lowell <[email protected]>
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, some more tests are needed I think, particularly for getting error responses from IMDS (like 400 error with details about the error)

Updated the token from url function to a reaquest based function
Updated test to fail not only return error
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity_test.go Outdated Show resolved Hide resolved
@chlowell chlowell added the enhancement New feature or request label Sep 11, 2024
Removed printing token
Variable name updated.
apps/internal/mock/mock.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity_test.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity_test.go Outdated Show resolved Hide resolved
apps/tests/devapps/managedidentity_sample.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity_test.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity_test.go Show resolved Hide resolved
apps/managedidentity/managedidentity_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@AndyOHart AndyOHart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
Updated the MI identity for UAMI with "UserAssigned" as prefix
@chlowell chlowell added enhancement New feature or request and removed enhancement New feature or request labels Sep 23, 2024
@4gust 4gust requested a review from chlowell September 24, 2024 10:36
apps/managedidentity/managedidentity.go Outdated Show resolved Hide resolved
}
if err != nil {
t.Fatal("client New() error while creating client")
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker but this else is still unnecessary. What you have is like

if err != nil {
    os.Exit(1)
} else {
    // this block never executes when err != nil, so you don't
    // need the else to prevent it executing in that case
}

}
mockClient.AppendResponse(mock.WithHTTPStatusCode(http.StatusOK), mock.WithBody(responseBody), mock.WithCallback(func(r *http.Request) { url = r.URL.String() }))
client, err := New(SystemAssigned(), WithHTTPClient(&mockClient))
mockClient.AppendResponse(mock.WithHTTPStatusCode(http.StatusOK), mock.WithBody(responseBody))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why delete the callback you had here? Don't you want to validate the request? Now you have this test doing

url := testCase.endpoint
// ... nothing modifies the value of url ...
if !strings.HasPrefix(url, testCase.endpoint) {
	t.Fatal("this line will never execute")
}

and you're really only testing that the client can unmarshal the response. That's important, but so is the client's request

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to check alot more values in the request.

} else {
if client.miType.value() != tt.id.value() {
t.Fatal("client New() did not assign a correct value to type.")
}
}
})
}

}
func TestCreateIMDSAuthRequest(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested merging this into another test, not simply deleting it; the coverage is important

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added that test to Test_SystemAssigned_Returns_Token_Success

@4gust 4gust requested a review from chlowell September 24, 2024 21:00
apps/managedidentity/managedidentity_test.go Outdated Show resolved Hide resolved
Comment on lines 39 to 40
// acquireTokenClientCertificate()
// // this time the token comes from the cache!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 do you intend to make this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I Was testing the api on the VM so, to get these changes there pushed this, now reverted.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarCloud

@4gust 4gust merged commit b6ec2ee into andyohart/managed-identity Sep 25, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants