add support for github app auth

[DrFaust92]

Closes #247

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of pkg/config/config.go

@@ -37,6 +37,11 @@ type ServerConfig struct {
 	VcsToken     string `mapstructure:"vcs-token"`
 	VcsType      string `mapstructure:"vcs-type"`
 
+	//github
+	GithubPrivateKey     string `mapstructure:"github-private-key"`
+	GithubAppID          int64  `mapstructure:"github-app-id"`
+	GithubInstallationID int64  `mapstructure:"github-installation-id"`
+
 	// webhooks
 	EnsureWebhooks bool   `mapstructure:"ensure-webhooks"`
 	WebhookSecret  string `mapstructure:"webhook-secret"`

Feedback & Suggestions:

1. Security Concern: Storing sensitive information like GithubPrivateKey directly in the configuration can be risky. Consider using environment variables or a secrets management service to handle sensitive data securely.

   GithubPrivateKey string `mapstructure:"github-private-key" env:"GITHUB_PRIVATE_KEY"`

   This way, you can use a library like github.com/kelseyhightower/envconfig to load the environment variables.

2. Documentation: Add comments to explain the purpose of the new GitHub-related fields. This will help future developers understand why these fields were added and how they should be used.

3. Validation: Ensure that the new fields are validated properly. For example, check that GithubAppID and GithubInstallationID are positive integers and that GithubPrivateKey is not empty.

   if cfg.GithubAppID <= 0 {
       return cfg, errors.New("invalid GitHub App ID")
   }
   if cfg.GithubInstallationID <= 0 {
       return cfg, errors.New("invalid GitHub Installation ID")
   }
   if cfg.GithubPrivateKey == "" {
       return cfg, errors.New("GitHub Private Key cannot be empty")
   }

4. Consistency: Ensure that the naming conventions are consistent. For example, consider using GitHub instead of Github to match the official branding.

---

😼 Mergecat review of docs/usage.md

@@ -46,6 +46,9 @@ The full list of supported environment variables is described below:
 |`KUBECHECKS_ENABLE_PREUPGRADE`|Enable preupgrade checks.|`true`|
 |`KUBECHECKS_ENSURE_WEBHOOKS`|Ensure that webhooks are created in repositories referenced by argo.|`false`|
 |`KUBECHECKS_FALLBACK_K8S_VERSION`|Fallback target Kubernetes version for schema / upgrade checks.|`1.23.0`|
+|`KUBECHECKS_GITHUB_APP_ID`|Github App ID.|`0`|
+|`KUBECHECKS_GITHUB_INSTALLATION_ID`|Github Installation ID.|`0`|
+|`KUBECHECKS_GITHUB_PRIVATE_KEY`|Github App Private Key.||
 |`KUBECHECKS_KUBERNETES_CLUSTERID`|Kubernetes Cluster ID, must be specified if kubernetes-type is eks.||
 |`KUBECHECKS_KUBERNETES_CONFIG`|Path to your kubernetes config file, used to monitor applications.||
 |`KUBECHECKS_KUBERNETES_TYPE`|Kubernetes Type One of eks, or local.|`local`|

Feedback & Suggestions:

1. Security Concern: The KUBECHECKS_GITHUB_PRIVATE_KEY should be handled with care. Consider adding a note about securely managing this key, such as using Kubernetes secrets or a secret management tool.
2. Default Values: The default values for KUBECHECKS_GITHUB_APP_ID and KUBECHECKS_GITHUB_INSTALLATION_ID are set to 0. If 0 is not a valid value, it might be better to leave these fields empty to avoid confusion.
3. Consistency: Ensure that the descriptions for the new environment variables are as detailed as the existing ones. For example, specifying the format or any constraints for KUBECHECKS_GITHUB_PRIVATE_KEY.

---

😼 Mergecat review of go.mod

@@ -12,6 +12,7 @@ require (
 	github.com/aws/aws-sdk-go-v2/service/eks v1.46.0
 	github.com/aws/aws-sdk-go-v2/service/sts v1.30.1
 	github.com/aws/smithy-go v1.20.3
+	github.com/bradleyfalzon/ghinstallation/v2 v2.6.0
 	github.com/cenkalti/backoff/v4 v4.3.0
 	github.com/chainguard-dev/git-urls v1.0.2
 	github.com/creasty/defaults v1.7.0
@@ -106,7 +107,6 @@ require (
 	github.com/blang/semver/v4 v4.0.0 // indirect
 	github.com/bmatcuk/doublestar/v4 v4.6.0 // indirect
 	github.com/bombsimon/logrusr/v2 v2.0.1 // indirect
-	github.com/bradleyfalzon/ghinstallation/v2 v2.6.0 // indirect
 	github.com/bufbuild/protocompile v0.6.0 // indirect
 	github.com/cespare/xxhash/v2 v2.3.0 // indirect
 	github.com/chai2010/gettext-go v1.0.2 // indirect

Feedback & Suggestions:

1. Duplicate Dependency: The diff adds github.com/bradleyfalzon/ghinstallation/v2 v2.6.0 as a direct dependency while removing it as an indirect dependency. This change is fine if you intend to use this package directly in your code. However, ensure that this change is intentional and that the package is indeed required directly.
2. Dependency Management: If github.com/bradleyfalzon/ghinstallation/v2 v2.6.0 is not needed directly, it would be better to keep it as an indirect dependency to avoid unnecessary clutter in the require block.

---

😼 Mergecat review of cmd/root.go

@@ -45,7 +45,7 @@ func init() {
 				zerolog.LevelDebugValue,
 				zerolog.LevelTraceValue,
 			).
-			withDefault("info").
+			withDefault(zerolog.LevelInfoValue).
 			withShortHand("l"),
 	)
 	boolFlag(flags, "persist-log-level", "Persists the set log level down to other module loggers.")
@@ -55,6 +55,9 @@ func init() {
 			withChoices("github", "gitlab").
 			withDefault("gitlab"))
 	stringFlag(flags, "vcs-token", "VCS API token.")
+	stringFlag(flags, "github-private-key", "Github App Private Key.")
+	int64Flag(flags, "github-app-id", "Github App ID.")
+	int64Flag(flags, "github-installation-id", "Github Installation ID.")
 	stringFlag(flags, "argocd-api-token", "ArgoCD API token.")
 	stringFlag(flags, "argocd-api-server-addr", "ArgoCD API Server Address.",
 		newStringOpts().
@@ -119,7 +122,10 @@ func setupLogOutput() {
 
 	// Default level is info, unless debug flag is present
 	levelFlag := viper.GetString("log-level")
-	level, _ := zerolog.ParseLevel(levelFlag)
+	level, err := zerolog.ParseLevel(levelFlag)
+	if err != nil {
+		log.Error().Err(err).Msg("Invalid log level")
+	}
 
 	zerolog.SetGlobalLevel(level)
 	log.Debug().Msg("Debug level logging enabled.")

Feedback & Suggestions:

1. Log Level Default Value:

   • The change from "info" to zerolog.LevelInfoValue is a good improvement for consistency. 👍

2. New GitHub Flags:

   • Adding flags for github-private-key, github-app-id, and github-installation-id is a useful addition. Ensure these values are securely managed and not exposed in logs or error messages. 🔒

3. Error Handling for Log Level Parsing:

   • Adding error handling for zerolog.ParseLevel is a great improvement. However, consider exiting the application if the log level is invalid, as continuing with an undefined log level might lead to unexpected behavior.
   • Suggested change:

     if err != nil {
         log.Error().Err(err).Msg("Invalid log level")
         os.Exit(1) // Exit the application if the log level is invalid
     }

4. Security Considerations:

   • Ensure that sensitive information such as github-private-key is not logged or exposed in any way. Consider adding validation to check for the presence of these flags and handle errors appropriately.

5. Performance Considerations:

   • No specific performance issues identified in the diff. The changes are primarily related to configuration and logging.

---

😼 Mergecat review of pkg/vcs/github_client/client.go

@@ -9,7 +9,8 @@ import (
 	"strconv"
 	"strings"
 
-	"github.com/chainguard-dev/git-urls"
+	"github.com/bradleyfalzon/ghinstallation/v2"
+	giturls "github.com/chainguard-dev/git-urls"
 	"github.com/google/go-github/v62/github"
 	"github.com/pkg/errors"
 	"github.com/rs/zerolog/log"
@@ -46,34 +47,51 @@ func CreateGithubClient(cfg config.ServerConfig) (*Client, error) {
 		err            error
 		googleClient   *github.Client
 		shurcoolClient *githubv4.Client
+		githubClient   *http.Client
 	)
 
-	// Initialize the GitLab client with access token
-	t := cfg.VcsToken
-	if t == "" {
-		log.Fatal().Msg("github token needs to be set")
+	if (cfg.GithubAppID == 0 || cfg.GithubInstallationID == 0 || cfg.GithubPrivateKey == "") && cfg.VcsToken == "" {
+		log.Fatal().Msg("Either GitHub token or GitHub App credentials (App ID, Installation ID, Private Key) must be set")
 	}
-	log.Debug().Msgf("Token Length - %d", len(t))
+
+	// Initialize the GitHub client with app key if provided
+	if cfg.GithubAppID != 0 && cfg.GithubInstallationID != 0 && cfg.GithubPrivateKey != "" {
+		appTransport, err := ghinstallation.New(http.DefaultTransport, cfg.GithubAppID, cfg.GithubInstallationID, []byte(cfg.GithubPrivateKey))
+		if err != nil {
+			log.Fatal().Err(err).Msg("failed to create github app transport")
+		}
+		githubClient = &http.Client{Transport: appTransport}
+	}
+
 	ctx := context.Background()
-	ts := oauth2.StaticTokenSource(
-		&oauth2.Token{AccessToken: t},
-	)
-	tc := oauth2.NewClient(ctx, ts)
+
+	// Initialize the GitHub client with access token if app key is not provided
+	if githubClient == nil {
+		vscToken := cfg.VcsToken
+		if vscToken != "" {
+			log.Debug().Msgf("Token Length - %d", len(vscToken))
+			ts := oauth2.StaticTokenSource(
+				&oauth2.Token{AccessToken: vscToken},
+			)
+			githubClient = oauth2.NewClient(ctx, ts)
+		}
+	} else {
+		log.Fatal().Msg("github client is not initialized")
+	}
 
 	githubUrl := cfg.VcsBaseUrl
 	githubUploadUrl := cfg.VcsUploadUrl
 	// we need both urls to be set for github enterprise
 	if githubUrl == "" || githubUploadUrl == "" {
-		googleClient = github.NewClient(tc) // If this has failed, we'll catch it on first call
+		googleClient = github.NewClient(githubClient) // If this has failed, we'll catch it on first call
 
-		// Use the client from shurcooL's githubv4 library for queries.
-		shurcoolClient = githubv4.NewClient(tc)
+		shurcoolClient = githubv4.NewClient(githubClient)
 	} else {
-		googleClient, err = github.NewClient(tc).WithEnterpriseURLs(githubUrl, githubUploadUrl)
+		googleClient, err = github.NewClient(githubClient).WithEnterpriseURLs(githubUrl, githubUploadUrl)
 		if err != nil {
 			log.Fatal().Err(err).Msg("failed to create github enterprise client")
 		}
-		shurcoolClient = githubv4.NewEnterpriseClient(githubUrl, tc)
+		shurcoolClient = githubv4.NewEnterpriseClient(githubUrl, githubClient)
 	}
 
 	user, _, err := googleClient.Users.Get(ctx, "")

Feedback & Suggestions:

1. Error Handling for GitHub Client Initialization: The current logic will always trigger a fatal log if neither the GitHub App credentials nor the VcsToken are provided. However, if the githubClient is not initialized, it should also trigger a fatal log. The current else block for githubClient == nil should be an else if to ensure proper error handling.

   +	} else if githubClient == nil {
   +		log.Fatal().Msg("github client is not initialized")

2. Redundant Error Check: The check for githubClient == nil after attempting to initialize it with the app key or token is redundant. This should be handled more cleanly by ensuring the initialization logic is correct.

   +	if githubClient == nil {
   +		log.Fatal().Msg("github client is not initialized")

3. Logging Improvements: The log message "github client is not initialized" should be more descriptive to indicate the specific reason why the initialization failed.

   +		log.Fatal().Msg("Failed to initialize GitHub client with either App credentials or VcsToken")

4. Security Consideration: Ensure that sensitive information such as GithubPrivateKey is handled securely and not logged. The current implementation does not log this, which is good, but it's worth mentioning to maintain this practice.

5. Code Duplication: The logic for initializing the GitHub client with either the app key or token is duplicated. This can be refactored into a helper function to reduce redundancy and improve readability.

---

By addressing these points, the code will be more robust, secure, and maintainable. 🛠️

---

Dependency Review

Click to read mergecats review!

No suggestions found

[djeebus]

Looks good to me! I'll run some tests locally, but (assuming all looks good) we'll get this merged. Thanks!

[DrFaust92]

djeebus any news?

[djeebus]

News! Sorry about the delay. Had to make some changes to get it to work, but #288 should have it implemented. If you get a chance, mind testing it out, see if it works for you?

[djeebus]

Closing in order to focus on #288