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

Implement new packages auth, azure and git for passwordless authentication scenarios #789

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

dipti-pai
Copy link
Member

@dipti-pai dipti-pai commented Jul 19, 2024

Changes include partial implementation of RFC-007 for Azure provider.

  • azure sub-package in auth with API to get azure devops access token using workload identity and associated token provider unit tests.
  • Changes to git and gogit package using the API to get credentials invoking provider specific implementation (only Azure is added currently) and associated git credentials/gogit client unit tests.

auth/azure/fake_credential.go Outdated Show resolved Hide resolved
auth/azure/token_provider.go Outdated Show resolved Hide resolved
auth/azure/token_provider.go Outdated Show resolved Hide resolved
auth/git/credentials.go Outdated Show resolved Hide resolved
auth/azure/token_provider.go Outdated Show resolved Hide resolved
auth/git/credentials_test.go Outdated Show resolved Hide resolved
auth/git/credentials.go Outdated Show resolved Hide resolved
auth/git/credentials.go Outdated Show resolved Hide resolved
@dipti-pai
Copy link
Member Author

dipti-pai commented Aug 12, 2024

@darkowlzz I removed all the caching-related code from the PR as I wanted to get thoughts about moving some files around before implementing.

  • With GetCredentials() call moved to gogit client in the new function CloneWithProvider and get/set/delete from cache would also be called from gogit client, would it make sense to not have a separate git package in pkg/auth/git rather move credentials.go to to existing git package at pkg/git and add the new cache.go to pkg/git with methods to get/set/delete entries from cache.
  • AuthOptions (in auth/git/credentials.go right now) will be renamed to ProviderAuthOptions and expanded to hold the cache passed by the caller. This can be moved to pkg/git/options.go which has other AuthOptions defined.
  • CloneWithProvider in pkg/git/gogit/client.go will be expanded with the following logic (pseudo code below)
function CloneWithProvider() {
    if getCredentialsFromCache != nil {
           commit, err = Clone()
           if err == UnAuthorized {
              invalidateCache() 
           } else {
               return commit, err
           }
    }
    
    getCredentials API to fetch new token
    Clone()
    addToCache() if clone was successful
}

@darkowlzz
Copy link
Contributor

@dipti-pai

would it make sense to not have a separate git package in pkg/auth/git rather move credentials.go to to existing git package at pkg/git and add the new cache.go to pkg/git with methods to get/set/delete entries from cache.

A few months ago in a dev meeting, we discussed about this possibility, that maybe the respective clients can have most of the authentication code and very few essential common code in auth package. With the recent discussions around caching in the client itself and looking at the simplicity of the credentials.go code, it appears to me that this would be the obvious design to go with, unless we find some good reason not to. At present, this sounds good to me.

AuthOptions (in auth/git/credentials.go right now) will be renamed to ProviderAuthOptions and expanded to hold the cache passed by the caller. This can be moved to pkg/git/options.go which has other AuthOptions defined.

Looking at the old PR, it seems that the AuthOptions was supposed to contain ProviderOptions, Secret and CacheOptions. I believe the Secret existed to accommodate for some static credential, non-OIDC. Since we are focused on Git OIDC here, I think we can ignore the static secret for now. The remaining ProviderOptions and cache (for caching credential only) are both related to authentication option. I think they can be added within the git.AuthOptions. The provider name can be part of ProviderOptions itself as it is optional too for AuthOptions or as a new field in AuthOptions.
I think the need to have a separate ProviderAuthOptions is because of the new CloneWithProvider(). Below I explain why it may be better to use the existing Clone(). Looking from source-controller's side, in https://github.com/fluxcd/source-controller/blob/v1.3.0/internal/controller/gitrepository_controller.go#L645, after obtaining AuthOptions, based on the provider in the GitRepository spec, we can set the ProviderOptions and cache within AuthOptions, which will be passed to Clone().

Regarding the proposed CloneWithProvider() design and looking at the current implementation of this function, I think we can avoid introducing a separate clone function for providers. It would make things simpler for the caller of clone.
The general idea proposed in the pseudocode provided above about using the cache seems good to me. Considering the pseudocode and the current implementation, a wrapper around the original Clone() seems better to me. As the existing Clone() and all the individual clone calls within it are mostly about the cloning strategy. Renaming the current clone function to clone() and adding the URL validation, caching and credential renewal to a wrapper Clone() seems better at present.
While thinking about it, I arrived at a recursive implementation of the Clone() that can help avoid double the checks we need to perform, but I guess a recursive method would need some extra considerations to break out of a recursion loop if that happens. A linear implementation may be safer and simpler.

git/gogit/client.go Outdated Show resolved Hide resolved
@stefanprodan stefanprodan added the area/git Git and SSH related issues and pull requests label Aug 22, 2024
auth/constants.go Outdated Show resolved Hide resolved
git/gogit/client.go Outdated Show resolved Hide resolved
auth/go.mod Outdated Show resolved Hide resolved
git/gogit/client.go Outdated Show resolved Hide resolved
git/gogit/client.go Outdated Show resolved Hide resolved
git/credentials.go Show resolved Hide resolved
git/gogit/client.go Outdated Show resolved Hide resolved
@dipti-pai dipti-pai force-pushed the pkg-azure-git-wi-auth branch from 9ed437e to 8aa2d25 Compare September 6, 2024 06:39
@dipti-pai
Copy link
Member Author

Manually validated when proxy server configuration is specified in GitRepository .spec.proxySecretRef, the requests to acquire azure devops access token go through the proxy

Relevant logs from source-controller and proxy server are below

{"level":"info","ts":"2024-09-06T07:46:29.767Z","msg":"no changes since last reconcilation: observed revision 'main@sha1:c629e6ee0520bb2c9328addb5229621fa0f31eab'","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"azure-devops","namespace":"default"},"namespace":"default","name":"azure-devops","reconcileID":"bc7870d0-fab9-4c94-aafc-fcd2419a4b76"}
{"level":"info","ts":"2024-09-06T07:47:28.875Z","msg":"no changes since last reconcilation: observed revision 'main@sha1:c629e6ee0520bb2c9328addb5229621fa0f31eab'","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"azure-devops","namespace":"default"},"namespace":"default","name":"azure-devops","reconcileID":"e88474a2-095e-4e69-beb2-9cefb0bce263"}
{"level":"info","ts":"2024-09-06T07:48:29.490Z","msg":"no changes since last reconcilation: observed revision 'main@sha1:c629e6ee0520bb2c9328addb5229621fa0f31eab'","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"azure-devops","namespace":"default"},"namespace":"default","name":"azure-devops","reconcileID":"44368d25-ab9a-47e5-8481-a94150d8be74"}
{"level":"info","ts":"2024-09-06T07:49:31.109Z","msg":"no changes since last reconcilation: observed revision 'main@sha1:c629e6ee0520bb2c9328addb5229621fa0f31eab'","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"azure-devops","namespace":"default"},"namespace":"default","name":"azure-devops","reconcileID":"7301e23e-0e2d-41ae-88dd-9b4949a450d2"}
2024/09/06 07:46:28 [217] INFO: Running 0 CONNECT handlers
2024/09/06 07:46:28 [217] INFO: Accepting CONNECT to login.microsoftonline.com:443
2024/09/06 07:46:29 [218] INFO: Running 0 CONNECT handlers
2024/09/06 07:46:29 [218] INFO: Accepting CONNECT to dev.azure.com:443
2024/09/06 07:47:28 [219] INFO: Running 0 CONNECT handlers
2024/09/06 07:47:28 [219] INFO: Accepting CONNECT to login.microsoftonline.com:443
2024/09/06 07:47:28 [220] INFO: Running 0 CONNECT handlers
2024/09/06 07:47:28 [220] INFO: Accepting CONNECT to dev.azure.com:443
2024/09/06 07:48:28 [221] INFO: Running 0 CONNECT handlers
2024/09/06 07:48:28 [221] INFO: Accepting CONNECT to login.microsoftonline.com:443
2024/09/06 07:48:28 [222] INFO: Running 0 CONNECT handlers
2024/09/06 07:48:28 [222] INFO: Accepting CONNECT to dev.azure.com:443
2024/09/06 07:49:30 [223] INFO: Running 0 CONNECT handlers
2024/09/06 07:49:30 [223] INFO: Accepting CONNECT to login.microsoftonline.com:443
2024/09/06 07:49:30 [224] INFO: Running 0 CONNECT handlers
2024/09/06 07:49:30 [224] INFO: Accepting CONNECT to dev.azure.com:443

git/go.mod Outdated Show resolved Hide resolved
auth/auth.go Outdated Show resolved Hide resolved
git/gogit/client.go Outdated Show resolved Hide resolved
git/credentials.go Outdated Show resolved Hide resolved
git/gogit/client.go Outdated Show resolved Hide resolved
auth/azure/token_provider.go Outdated Show resolved Hide resolved
auth/azure/token_provider.go Outdated Show resolved Hide resolved
@dipti-pai dipti-pai force-pushed the pkg-azure-git-wi-auth branch from 16adfc9 to c8cf7e4 Compare September 6, 2024 18:13
@dipti-pai dipti-pai changed the base branch from azure-git-oidc to main September 9, 2024 16:06
auth/azure/client.go Outdated Show resolved Hide resolved
git/go.mod Outdated Show resolved Hide resolved
git/gogit/client.go Outdated Show resolved Hide resolved
git/credentials_test.go Outdated Show resolved Hide resolved
git/credentials_test.go Outdated Show resolved Hide resolved
@dipti-pai dipti-pai force-pushed the pkg-azure-git-wi-auth branch 2 times, most recently from 2b2fb00 to abee735 Compare September 10, 2024 20:40
git/credentials.go Outdated Show resolved Hide resolved
git/gogit/client.go Outdated Show resolved Hide resolved
@dipti-pai dipti-pai force-pushed the pkg-azure-git-wi-auth branch from abee735 to 6cd9385 Compare September 11, 2024 05:46
git/gogit/client.go Outdated Show resolved Hide resolved
@dipti-pai dipti-pai force-pushed the pkg-azure-git-wi-auth branch from 6cd9385 to 8f8a78e Compare September 12, 2024 20:05
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

Tested the usage of this in source-controller in fluxcd/source-controller#1591 and everything looks good to me.
Thanks for the detailed tests of all the scenarios.

git/credentials_test.go Outdated Show resolved Hide resolved
- Add packages auth, azure to fetch access token using azidentity DefaultAzureCredential API and default ARM scope

- Provide the capability to override the scope of the access token for Azure DevOps.

- Provide the capability to pass proxy settings to the client options if specified.

- Provide the option to specify a fake token credential for unit tests.

- Add ProviderOptions in git AuthOptions to configure the provider options from consumers.

- Use the credentials API to fetch Azure DevOps access token if the provider is Azure from gogit client.

- Add new unit tests for new functionality in azure, git and gogit client.

Signed-off-by: Dipti Pai <[email protected]>
@dipti-pai dipti-pai force-pushed the pkg-azure-git-wi-auth branch from 8f8a78e to 1686996 Compare September 13, 2024 16:21
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @dipti-pai 🥇

@stefanprodan stefanprodan merged commit 62475f1 into fluxcd:main Sep 13, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/git Git and SSH related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants