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

[RFC-007] Implement GitHub app authentication for git repositories. #1647

Merged
merged 2 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions api/v1/gitrepository_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ const (
// GitProviderAzure provides support for authentication to azure
// repositories using Managed Identity.
GitProviderAzure string = "azure"

// GitProviderGitHub provides support for authentication to git
// repositories using GitHub App authentication
GitProviderGitHub string = "github"
)

const (
Expand Down Expand Up @@ -88,9 +92,9 @@ type GitRepositorySpec struct {
// +optional
SecretRef *meta.LocalObjectReference `json:"secretRef,omitempty"`

// Provider used for authentication, can be 'azure', 'generic'.
// Provider used for authentication, can be 'azure', 'github', 'generic'.
// When not specified, defaults to 'generic'.
// +kubebuilder:validation:Enum=generic;azure
// +kubebuilder:validation:Enum=generic;azure;github
// +optional
Provider string `json:"provider,omitempty"`

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,12 @@ spec:
type: string
provider:
description: |-
Provider used for authentication, can be 'azure', 'generic'.
Provider used for authentication, can be 'azure', 'github', 'generic'.
When not specified, defaults to 'generic'.
enum:
- generic
- azure
- github
type: string
proxySecretRef:
description: |-
Expand Down
4 changes: 2 additions & 2 deletions docs/api/v1/source.md
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ string
</td>
<td>
<em>(Optional)</em>
<p>Provider used for authentication, can be &lsquo;azure&rsquo;, &lsquo;generic&rsquo;.
<p>Provider used for authentication, can be &lsquo;azure&rsquo;, &lsquo;github&rsquo;, &lsquo;generic&rsquo;.
When not specified, defaults to &lsquo;generic&rsquo;.</p>
</td>
</tr>
Expand Down Expand Up @@ -1730,7 +1730,7 @@ string
</td>
<td>
<em>(Optional)</em>
<p>Provider used for authentication, can be &lsquo;azure&rsquo;, &lsquo;generic&rsquo;.
<p>Provider used for authentication, can be &lsquo;azure&rsquo;, &lsquo;github&rsquo;, &lsquo;generic&rsquo;.
When not specified, defaults to &lsquo;generic&rsquo;.</p>
</td>
</tr>
Expand Down
59 changes: 59 additions & 0 deletions docs/spec/v1/gitrepositories.md
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ Supported options are:

- `generic`
- `azure`
- `github`

When provider is not specified, it defaults to `generic` indicating that
mechanisms using `spec.secretRef` are used for authentication.
Expand Down Expand Up @@ -296,6 +297,64 @@ must follow this format:
```
https://dev.azure.com/{your-organization}/{your-project}/_git/{your-repository}
```
#### GitHub

The `github` provider can be used to authenticate to Git repositories using
[GitHub Apps](https://docs.github.com/en/apps/overview).

##### Pre-requisites

- [Register](https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app/registering-a-github-app)
the GitHub App with the necessary permissions and [generate a private
key](https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/managing-private-keys-for-github-apps)
for the app.

- [Install](https://docs.github.com/en/apps/using-github-apps/installing-your-own-github-app)
the app in the organization/account configuring access to the necessary
repositories.

##### Configure GitHub App secret

The GitHub App information is specified in `.spec.secretRef` in the format
specified below:

- Get the App ID from the app settings page at
`https://github.com/settings/apps/<app-name>`.
- Get the App Installation ID from the app installations page at
`https://github.com/settings/installations`. Click the installed app, the URL
will contain the installation ID
`https://github.com/settings/installations/<installation-id>`. For
organizations, the first part of the URL may be different, but it follows the
same pattern.
- The private key that was generated in the pre-requisites.
- (Optional) GitHub Enterprise Server users can set the base URL to
`http(s)://HOSTNAME/api/v3`.

```yaml
apiVersion: v1
kind: Secret
dipti-pai marked this conversation as resolved.
Show resolved Hide resolved
metadata:
name: github-sa
type: Opaque
stringData:
githubAppID: "<app-id>"
githubAppInstallationID: "<app-installation-id>"
githubAppPrivateKey: |
-----BEGIN RSA PRIVATE KEY-----
...
-----END RSA PRIVATE KEY-----
githubAppBaseURL: "<github-enterprise-api-url>" #optional, required only for GitHub Enterprise Server users
```

Alternatively, the Flux CLI can be used to automatically create the secret with
the github app authentication information.

```sh
flux create secret githubapp ghapp-secret \
--app-id=1 \
--app-installation-id=3 \
--app-private-key=~/private-key.pem
```

### Interval

Expand Down
55 changes: 44 additions & 11 deletions internal/controller/gitrepository_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (

securejoin "github.com/cyphar/filepath-securejoin"
"github.com/fluxcd/pkg/auth/azure"
"github.com/fluxcd/pkg/auth/github"
"github.com/fluxcd/pkg/runtime/logger"
"github.com/go-git/go-git/v5/plumbing/transport"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -504,13 +505,8 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch

authOpts, err := r.getAuthOpts(ctx, obj, *u)
if err != nil {
e := serror.NewGeneric(
fmt.Errorf("failed to configure authentication options: %w", err),
sourcev1.AuthenticationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
// Return error as the world as observed may change
return sreconcile.ResultEmpty, e
return sreconcile.ResultEmpty, err
}

// Fetch the included artifact metadata.
Expand Down Expand Up @@ -637,26 +633,63 @@ func (r *GitRepositoryReconciler) getAuthOpts(ctx context.Context, obj *sourcev1
var err error
authData, err = r.getSecretData(ctx, obj.Spec.SecretRef.Name, obj.GetNamespace())
if err != nil {
return nil, fmt.Errorf("failed to get secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.SecretRef.Name, err)
e := serror.NewGeneric(
fmt.Errorf("failed to get secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.SecretRef.Name, err),
sourcev1.AuthenticationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return nil, e
}
}

// Configure authentication strategy to access the source
authOpts, err := git.NewAuthOptions(u, authData)
if err != nil {
return nil, err
e := serror.NewGeneric(
fmt.Errorf("failed to configure authentication options: %w", err),
sourcev1.AuthenticationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return nil, e
}

// Configure provider authentication if specified in spec
if obj.GetProvider() == sourcev1.GitProviderAzure {
switch obj.GetProvider() {
case sourcev1.GitProviderAzure:
authOpts.ProviderOpts = &git.ProviderOptions{
Name: obj.GetProvider(),
Name: sourcev1.GitProviderAzure,
AzureOpts: []azure.OptFunc{
azure.WithAzureDevOpsScope(),
},
}
}
case sourcev1.GitProviderGitHub:
darkowlzz marked this conversation as resolved.
Show resolved Hide resolved
// if provider is github, but secret ref is not specified
if obj.Spec.SecretRef == nil {
e := serror.NewStalling(
fmt.Errorf("secretRef with github app data must be specified when provider is set to github"),
sourcev1.AuthenticationFailedReason,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the way we use AuthenticationFailedReason today is for actual authentication failures which are retried. Using AuthenticationFailedReason with stalled condition seems incorrect. The problem is a misconfiguration, invalid configuration. We have URLInvalidReason, InvalidSTSConfigurationReason, etc which are explicit about the type of failure. In this case, we haven't even attempted authentication but we know that authentication won't work. I think it would be better to introduce a new reason for this, similar to STS configuration, say InvalidProviderConfigurationReason.

I think the same can be used for the case below where the secret contains github app data but the provider is not set, also invalid provider configuration.

Copy link
Contributor

@darkowlzz darkowlzz Dec 19, 2024

Choose a reason for hiding this comment

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

I pushed a separate commit introducing InvalidProviderConfigurationReason and using it above. I'll change it in case of any objection.

Also, added a few tests for checking this reason on failure in the status conditions.
Also, slightly modified and rearranged a test case where secret ref is provided but the secret doesn't exist. This is not specific to github provider, but generic in getAuthOpts().

)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return nil, e
}

authOpts.ProviderOpts = &git.ProviderOptions{
Name: sourcev1.GitProviderGitHub,
GitHubOpts: []github.OptFunc{
github.WithAppData(authData),
},
}
default:
// analyze secret, if it has github app data, perhaps provider should have been github.
if appID := authData[github.AppIDKey]; len(appID) != 0 {
e := serror.NewStalling(
fmt.Errorf("secretRef '%s/%s' has github app data but provider is not set to github", obj.GetNamespace(), obj.Spec.SecretRef.Name),
sourcev1.AuthenticationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return nil, e
}
}
return authOpts, nil
}

Expand Down
103 changes: 95 additions & 8 deletions internal/controller/gitrepository_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import (

kstatus "github.com/fluxcd/cli-utils/pkg/kstatus/status"
"github.com/fluxcd/pkg/apis/meta"
"github.com/fluxcd/pkg/auth/github"
"github.com/fluxcd/pkg/git"
"github.com/fluxcd/pkg/gittestserver"
"github.com/fluxcd/pkg/runtime/conditions"
Expand Down Expand Up @@ -686,46 +687,132 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) {
func TestGitRepositoryReconciler_getAuthOpts_provider(t *testing.T) {
tests := []struct {
name string
url string
secret *corev1.Secret
beforeFunc func(obj *sourcev1.GitRepository)
wantProviderOptsName string
wantErr error
}{
{
name: "azure provider",
url: "https://dev.azure.com/foo/bar/_git/baz",
beforeFunc: func(obj *sourcev1.GitRepository) {
obj.Spec.Provider = sourcev1.GitProviderAzure
},
wantProviderOptsName: sourcev1.GitProviderAzure,
},
{
name: "github provider with no secret ref",
url: "https://github.com/org/repo.git",
beforeFunc: func(obj *sourcev1.GitRepository) {
obj.Spec.Provider = sourcev1.GitProviderGitHub
},
wantProviderOptsName: sourcev1.GitProviderGitHub,
wantErr: errors.New("secretRef with github app data must be specified when provider is set to github"),
},
{
name: "github provider with secret ref that does not exist",
url: "https://github.com/org/repo.git",
beforeFunc: func(obj *sourcev1.GitRepository) {
obj.Spec.Provider = sourcev1.GitProviderGitHub
obj.Spec.SecretRef = &meta.LocalObjectReference{
Name: "githubAppSecret",
}
},
wantErr: errors.New("failed to get secret '/githubAppSecret': secrets \"githubAppSecret\" not found"),
},
{
name: "github provider with github app data in secret",
url: "https://example.com/org/repo",
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "githubAppSecret",
},
Data: map[string][]byte{
github.AppIDKey: []byte("123"),
github.AppInstallationIDKey: []byte("456"),
github.AppPrivateKey: []byte("abc"),
},
},
beforeFunc: func(obj *sourcev1.GitRepository) {
obj.Spec.Provider = sourcev1.GitProviderGitHub
obj.Spec.SecretRef = &meta.LocalObjectReference{
Name: "githubAppSecret",
}
},
wantProviderOptsName: sourcev1.GitProviderGitHub,
},
{
name: "generic provider with github app data in secret",
url: "https://example.com/org/repo",
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "githubAppSecret",
},
Data: map[string][]byte{
github.AppIDKey: []byte("123"),
},
},
beforeFunc: func(obj *sourcev1.GitRepository) {
obj.Spec.Provider = sourcev1.GitProviderGeneric
obj.Spec.SecretRef = &meta.LocalObjectReference{
Name: "githubAppSecret",
}
},
wantErr: errors.New("secretRef '/githubAppSecret' has github app data but provider is not set to github"),
},
{
name: "generic provider",
url: "https://example.com/org/repo",
beforeFunc: func(obj *sourcev1.GitRepository) {
obj.Spec.Provider = sourcev1.GitProviderGeneric
},
},
{
url: "https://example.com/org/repo",
name: "no provider",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)
clientBuilder := fakeclient.NewClientBuilder().
WithScheme(testEnv.GetScheme()).
WithStatusSubresource(&sourcev1.GitRepository{})

if tt.secret != nil {
clientBuilder.WithObjects(tt.secret)
}

obj := &sourcev1.GitRepository{}
r := &GitRepositoryReconciler{}
url, _ := url.Parse("https://dev.azure.com/foo/bar/_git/baz")
r := &GitRepositoryReconciler{
EventRecorder: record.NewFakeRecorder(32),
Client: clientBuilder.Build(),
features: features.FeatureGates(),
patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"),
}

url, err := url.Parse(tt.url)
g.Expect(err).ToNot(HaveOccurred())

if tt.beforeFunc != nil {
tt.beforeFunc(obj)
}
opts, err := r.getAuthOpts(context.TODO(), obj, *url)

g.Expect(err).ToNot(HaveOccurred())
g.Expect(opts).ToNot(BeNil())
if tt.wantProviderOptsName != "" {
g.Expect(opts.ProviderOpts).ToNot(BeNil())
g.Expect(opts.ProviderOpts.Name).To(Equal(tt.wantProviderOptsName))
if tt.wantErr != nil {
g.Expect(err).To(HaveOccurred())
g.Expect(err.Error()).To(ContainSubstring(tt.wantErr.Error()))
} else {
g.Expect(opts.ProviderOpts).To(BeNil())
g.Expect(err).ToNot(HaveOccurred())
g.Expect(opts).ToNot(BeNil())
if tt.wantProviderOptsName != "" {
g.Expect(opts.ProviderOpts).ToNot(BeNil())
g.Expect(opts.ProviderOpts.Name).To(Equal(tt.wantProviderOptsName))
} else {
g.Expect(opts.ProviderOpts).To(BeNil())
}
}
})
}
Expand Down
Loading