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

Add AuthN support using Nested Cred #30

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

gouthamMN
Copy link

JIRA link:
https://issues.redhat.com/browse/ARO-11172

What does this PR do?
This PR adds following interfaces to support AuthN operations on nested credentials as outlined in the DDR.
By moving the authentication logic here, we can ensure consistency for authenticating (i.e. setting regional endpoint, disabling instance metadata discovery, etc.)

  • Interfaces:
    • GetNestedCredentialsObjectUserAssignedIdentities
    • GetCredentialsObjectUserAssignedIdentities
    • (c CredentialsObject) GetCredential()
    • (n NestedCredentialsObject) GetCredential()

Testing:
Update the existing unit tests.


// Constructor for Nested Credentials Object UserAssignedIdentities
func NewNestedCredentialsObjectUAIdentities(c swagger.NestedCredentialsObject, cloud string) (*NestedCredentialsObject, error) {
return &NestedCredentialsObject{NestedCredentialsObject: c, cloud: cloud}, nil

Choose a reason for hiding this comment

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

I'm surprised to see an exported member field set by a constructor - either omit the constructor and export all the members or hide them and close over object initialization with the constructor.

Copy link
Author

Choose a reason for hiding this comment

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

Updated to omit the constructor and export all the members.

// Clients can use the credential to get a token for the user-assigned identity
func (u UserAssignedIdentities) GetCredential(requestedResourceID string) (*azidentity.ClientCertificateCredential, error) {
func (c CredentialsObject) GetCredential(requestedResourceID string) (*azidentity.ClientCertificateCredential, error) {

Choose a reason for hiding this comment

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

Let's go ahead and implement this interface instead:

// TokenCredential represents a credential capable of providing an OAuth token.
// Exported as azcore.TokenCredential.
type TokenCredential interface {
	// GetToken requests an access token for the specified set of scopes.
	GetToken(ctx context.Context, options TokenRequestOptions) (AccessToken, error)
}

We know that reloading the value from disk is critically important and every client will need to do it. A method off of a credentials object that returns a static *azidentity.ClientCertificateCredential is insufficient.

Copy link

@stevekuznetsov stevekuznetsov Jan 3, 2025

Choose a reason for hiding this comment

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

(The higher-level construct should take a path as input and can use the code you have here as the implementation for loading the credential. Look at the azidentity.WorkloadIdentityCredential as prior art here. )

Copy link
Author

@gouthamMN gouthamMN Jan 6, 2025

Choose a reason for hiding this comment

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

I already see an implementation of GetToken() func for Object azidentity.ClientCertificateCredential under azidentity.client_certificate_credential. Client could directly use that to get token? The implementation logic is exactly similar to azidentity.WorkloadIdentityCredential. Doesn't this suffice the ask?

Choose a reason for hiding this comment

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

We chatted about this out-of-band but just for posterity - no, it doesn't, since the code here is loading the credential once, and what clients will need is to refresh it from disk when it changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants