-
Notifications
You must be signed in to change notification settings - Fork 214
Add support for mTLS to GitHub App transport #1860
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
Conversation
aae442d
to
371c49f
Compare
Great work on this PR! 🎉 Following up on our discussion about this implementation, I just wanted to share that fluxcd/pkg#1001 was recently merged which adds GitHub App support to This could simplify the implementation here by using the unified authentication pattern instead of manual secret parsing. The new API provides: // Extract both GitHub App auth and TLS config from a single secret
targetURL := fmt.Sprintf("%s://%s", u.Scheme, u.Host)
authMethods, err := secrets.AuthMethodsFromSecret(ctx, secret, secrets.WithTargetURL(targetURL), secrets.WithSystemCertPool())
if err != nil {
return err
}
// Configure GitHub App authentication
var appOpts []github.OptFunc
if authMethods.HasGitHubAppData() {
appOpts = append(appOpts, github.WithAppData(authMethods.GitHubAppData))
}
// Configure TLS if available
if authMethods.HasTLS() {
appOpts = append(appOpts, github.WithTLSConfig(authMethods.TLS))
}
// Add other options (proxy, cache, etc.)
if proxyURL != nil {
appOpts = append(appOpts, github.WithProxyURL(proxyURL))
}
username, password, err := github.GetCredentials(ctx, appOpts...) Would you be interested in refactoring to use this unified approach? I'd be happy to help with the migration if needed! |
Yes, thank you @cappyzawa, I have done what you’ve suggested. Just working on the documentation part… |
cb95019
to
6dab2c8
Compare
|
6dab2c8
to
5000fd3
Compare
@stefanprodan @matheuscscp @cappyzawa Could I please, get feedback on the documentation? |
Good documentation addition! The ca.crt usage explanation for GitHub Enterprise Server users is clear and helpful. The YAML example properly shows how to include the certificate, and it's placed in the right section of the docs. Thanks for making this feature more accessible to Enterprise users! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a test case for the scenario where provider: github
is set with a secretRef containing basic auth data (no GitHub App data). This would improve test coverage for the new mandatory GitHub App validation:
{
name: "github provider with basic auth secret (no github app data) should fail",
url: "https://github.com/org/repo.git",
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "basic-auth-secret",
},
Data: map[string][]byte{
"username": []byte("user"),
"password": []byte("ghp_token"), // GitHub PAT
},
},
beforeFunc: func(obj *sourcev1.GitRepository) {
obj.Spec.Provider = sourcev1.GitProviderGitHub
obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "basic-auth-secret"}
},
wantErr: "secretRef with github app data must be specified when provider is set to github",
},
This ensures the !authMethods.HasGitHubAppData()
validation is properly tested.
5000fd3
to
9840ec1
Compare
Done - 9840ec1 I added the test cases to both |
9840ec1
to
19f0781
Compare
19f0781
to
7156479
Compare
return nil, err | ||
} | ||
if !authMethods.HasGitHubAppData() { | ||
e := serror.NewStalling( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While reviewing this specific line, I noticed that we are stalling in a few places in this controller where we should not, so I fixed it here:
This is also a place where we should not stall. The secret can be updated and the controller will only know if it does not stall.
Stalling means adding the Stalled
condition in the CR status and not enqueueing any more reconciliation requests. When this is done, the same CR is only reconciled again if the watcher receives an event. But if the secret with the GitHub App info is updated and fixed, we will not get any watch events, as the controller is not watching secrets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thank you!... I will replace it with serror.NewGeneric(....)
7156479
to
e685664
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good @abhijith-darshan, thanks for all the work here!!
To merge we need one last important thing to be done: we need to test this e2e with a real GitHub Enterprise Server. Would you be able to do that for us and post here some logs/screenshots?
Also, we can only merge this if the feature is implemented in image-automation-controller as well, so we'll have to wait for the IAC PR to be ready for review and tested, to merge the 2 PRs at the same time 🙏 |
Yes of course I can do that. At SAP we have multi-tenant GitHub Enterprise Servers.
I have already tested it for both servers. I will post screenshots / Video and Logs. But I will confirm first with our Open Source program office if we can expose domain URLs or redact it (just to be compliant) |
I had a draft open on IAC controller. I just need to carry over the review changes to it. I will do that soon.. 🖖🏻 |
Please do redact any sensitive information, even if your company says it's okay not to! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @abhijith-darshan 🏅
@@ -357,6 +357,8 @@ 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`. | |||
- (Optional) If GitHub Enterprise Server uses a private CA, include its bundle (root and any intermediates) in `ca.crt`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also document tls.ctl
and tls.key
here, according to this comment:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 46522f9
Scenario - GitHub Enterprise Server requiring CAScreenshotLogsflux % kc logs -f deployment/source-controller -n source-system -c manager
{"level":"info","ts":"2025-08-12T19:33:17.311Z","logger":"setup","msg":"caching of Helm index files is disabled"}
{"level":"info","ts":"2025-08-12T19:33:17.312Z","logger":"setup","msg":"starting manager"}
{"level":"info","ts":"2025-08-12T19:33:17.312Z","logger":"controller-runtime.metrics","msg":"Starting metrics server"}
{"level":"info","ts":"2025-08-12T19:33:17.312Z","logger":"controller-runtime.metrics","msg":"Serving metrics server","bindAddress":":8080","secure":false}
{"level":"info","ts":"2025-08-12T19:33:17.312Z","msg":"starting server","name":"health probe","addr":"[::]:9440"}
{"level":"info","ts":"2025-08-12T19:33:17.413Z","logger":"runtime","msg":"attempting to acquire leader lease source-system/source-controller-leader-election..."}
{"level":"info","ts":"2025-08-12T19:33:17.418Z","logger":"runtime","msg":"successfully acquired lease source-system/source-controller-leader-election"}
{"level":"info","ts":"2025-08-12T19:33:17.418Z","msg":"Starting EventSource","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","source":"kind source: *v1.GitRepository"}
{"level":"info","ts":"2025-08-12T19:33:17.418Z","msg":"Starting EventSource","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart","source":"kind source: *v1.Bucket"}
{"level":"info","ts":"2025-08-12T19:33:17.418Z","msg":"Starting EventSource","controller":"helmrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmRepository","source":"kind source: *v1.HelmRepository"}
{"level":"info","ts":"2025-08-12T19:33:17.418Z","msg":"Starting EventSource","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart","source":"kind source: *v1.HelmRepository"}
{"level":"info","ts":"2025-08-12T19:33:17.418Z","msg":"Starting EventSource","controller":"ocirepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"OCIRepository","source":"kind source: *v1.OCIRepository"}
{"level":"info","ts":"2025-08-12T19:33:17.418Z","msg":"Starting EventSource","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart","source":"kind source: *v1.HelmChart"}
{"level":"info","ts":"2025-08-12T19:33:17.418Z","msg":"Starting EventSource","controller":"bucket","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"Bucket","source":"kind source: *v1.Bucket"}
{"level":"info","ts":"2025-08-12T19:33:17.418Z","msg":"Starting EventSource","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart","source":"kind source: *v1.GitRepository"}
{"level":"info","ts":"2025-08-12T19:33:17.418Z","logger":"setup","msg":"starting file server"}
{"level":"info","ts":"2025-08-12T19:33:17.519Z","msg":"Starting Controller","controller":"helmrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmRepository"}
{"level":"info","ts":"2025-08-12T19:33:17.519Z","msg":"Starting workers","controller":"helmrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmRepository","worker count":2}
{"level":"info","ts":"2025-08-12T19:33:17.519Z","msg":"Starting Controller","controller":"bucket","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"Bucket"}
{"level":"info","ts":"2025-08-12T19:33:17.520Z","msg":"Starting Controller","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository"}
{"level":"info","ts":"2025-08-12T19:33:17.520Z","msg":"Starting workers","controller":"bucket","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"Bucket","worker count":2}
{"level":"info","ts":"2025-08-12T19:33:17.520Z","msg":"Starting workers","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","worker count":2}
{"level":"info","ts":"2025-08-12T19:33:17.520Z","msg":"Starting Controller","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart"}
{"level":"info","ts":"2025-08-12T19:33:17.520Z","msg":"Starting workers","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart","worker count":2}
{"level":"info","ts":"2025-08-12T19:33:17.520Z","msg":"Starting Controller","controller":"ocirepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"OCIRepository"}
{"level":"info","ts":"2025-08-12T19:33:17.520Z","msg":"Starting workers","controller":"ocirepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"OCIRepository","worker count":2}
{"level":"info","ts":"2025-08-12T19:34:40.752Z","msg":"metadata.finalizers: \"finalizers.fluxcd.io\": prefer a domain-qualified finalizer name to avoid accidental conflicts with other finalizer writers","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"git-repository-internal","namespace":"flux-system"},"namespace":"flux-system","name":"git-repository-internal","reconcileID":"19450807-552d-4e86-b9c1-191bcfb3f5f0"}
{"level":"error","ts":"2025-08-12T19:34:41.688Z","msg":"Reconciler error","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"git-repository-internal","namespace":"flux-system"},"namespace":"flux-system","name":"git-repository-internal","reconcileID":"b76822cf-e85b-4b67-b479-238aaf288563","error":"failed to configure authentication options: could not refresh installation id 8383's token: could not get access_tokens from GitHub API for installation ID 8383: tls: failed to verify certificate: x509: certificate signed by unknown authority"}
{"level":"error","ts":"2025-08-12T19:34:43.317Z","msg":"Reconciler error","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"git-repository-internal","namespace":"flux-system"},"namespace":"flux-system","name":"git-repository-internal","reconcileID":"4a58b88f-8def-4ea6-94cb-48ca87aa5bc1","error":"failed to configure authentication options: could not refresh installation id 8383's token: could not get access_tokens from GitHub API for installation ID 8383: tls: failed to verify certificate: x509: certificate signed by unknown authority"}
{"level":"error","ts":"2025-08-12T19:34:46.452Z","msg":"Reconciler error","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"git-repository-internal","namespace":"flux-system"},"namespace":"flux-system","name":"git-repository-internal","reconcileID":"5d57ee39-fe96-4016-ac28-f1eaf9f960a8","error":"failed to configure authentication options: could not refresh installation id 8383's token: could not get access_tokens from GitHub API for installation ID 8383: tls: failed to verify certificate: x509: certificate signed by unknown authority"}
{"level":"info","ts":"2025-08-12T19:34:55.284Z","msg":"stored artifact for commit 'Merge pull request #1 from teaser/20250808_ADD_ima...'","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"git-repository-internal","namespace":"flux-system"},"namespace":"flux-system","name":"git-repository-internal","reconcileID":"35fc89ea-5011-4ec5-8dd1-8fca2f447d68"}
{"level":"info","ts":"2025-08-12T19:34:55.512Z","msg":"no changes since last reconcilation: observed revision 'main@sha1:7e38979160dc031052601629faf30b7cd46f5684'","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"git-repository-internal","namespace":"flux-system"},"namespace":"flux-system","name":"git-repository-internal","reconcileID":"bd120989-9934-4425-884c-c5dba19214fd"}
{"level":"info","ts":"2025-08-12T19:39:32.578Z","msg":"garbage collected artifacts for deleted resource","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"git-repository-internal","namespace":"flux-system"},"namespace":"flux-system","name":"git-repository-internal","reconcileID":"4e0889fe-85e0-470f-95e8-af2fd3b2d9be"} The above scenario initially applies a
Scenario - GitHub Enterprise Server No CA Requiredflux % kc logs -f deployment/source-controller -n source-system -c manager
{"level":"info","ts":"2025-08-12T20:28:09.253Z","logger":"setup","msg":"caching of Helm index files is disabled"}
{"level":"info","ts":"2025-08-12T20:28:09.254Z","logger":"setup","msg":"starting manager"}
{"level":"info","ts":"2025-08-12T20:28:09.254Z","logger":"controller-runtime.metrics","msg":"Starting metrics server"}
{"level":"info","ts":"2025-08-12T20:28:09.254Z","logger":"controller-runtime.metrics","msg":"Serving metrics server","bindAddress":":8080","secure":false}
{"level":"info","ts":"2025-08-12T20:28:09.254Z","msg":"starting server","name":"health probe","addr":"[::]:9440"}
{"level":"info","ts":"2025-08-12T20:28:09.355Z","logger":"runtime","msg":"attempting to acquire leader lease source-system/source-controller-leader-election..."}
{"level":"info","ts":"2025-08-12T20:28:09.359Z","logger":"runtime","msg":"successfully acquired lease source-system/source-controller-leader-election"}
{"level":"info","ts":"2025-08-12T20:28:09.359Z","msg":"Starting EventSource","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","source":"kind source: *v1.GitRepository"}
{"level":"info","ts":"2025-08-12T20:28:09.359Z","msg":"Starting EventSource","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart","source":"kind source: *v1.Bucket"}
{"level":"info","ts":"2025-08-12T20:28:09.359Z","msg":"Starting EventSource","controller":"helmrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmRepository","source":"kind source: *v1.HelmRepository"}
{"level":"info","ts":"2025-08-12T20:28:09.359Z","msg":"Starting EventSource","controller":"ocirepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"OCIRepository","source":"kind source: *v1.OCIRepository"}
{"level":"info","ts":"2025-08-12T20:28:09.360Z","msg":"Starting EventSource","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart","source":"kind source: *v1.HelmChart"}
{"level":"info","ts":"2025-08-12T20:28:09.360Z","msg":"Starting EventSource","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart","source":"kind source: *v1.HelmRepository"}
{"level":"info","ts":"2025-08-12T20:28:09.360Z","msg":"Starting EventSource","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart","source":"kind source: *v1.GitRepository"}
{"level":"info","ts":"2025-08-12T20:28:09.360Z","msg":"Starting EventSource","controller":"bucket","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"Bucket","source":"kind source: *v1.Bucket"}
{"level":"info","ts":"2025-08-12T20:28:09.360Z","logger":"setup","msg":"starting file server"}
{"level":"info","ts":"2025-08-12T20:28:09.460Z","msg":"Starting Controller","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository"}
{"level":"info","ts":"2025-08-12T20:28:09.460Z","msg":"Starting Controller","controller":"helmrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmRepository"}
{"level":"info","ts":"2025-08-12T20:28:09.460Z","msg":"Starting workers","controller":"helmrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmRepository","worker count":2}
{"level":"info","ts":"2025-08-12T20:28:09.460Z","msg":"Starting Controller","controller":"ocirepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"OCIRepository"}
{"level":"info","ts":"2025-08-12T20:28:09.460Z","msg":"Starting workers","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","worker count":2}
{"level":"info","ts":"2025-08-12T20:28:09.460Z","msg":"Starting workers","controller":"ocirepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"OCIRepository","worker count":2}
{"level":"info","ts":"2025-08-12T20:28:09.460Z","msg":"Starting Controller","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart"}
{"level":"info","ts":"2025-08-12T20:28:09.460Z","msg":"Starting workers","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart","worker count":2}
{"level":"info","ts":"2025-08-12T20:28:09.460Z","msg":"Starting Controller","controller":"bucket","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"Bucket"}
{"level":"info","ts":"2025-08-12T20:28:09.460Z","msg":"Starting workers","controller":"bucket","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"Bucket","worker count":2}
{"level":"info","ts":"2025-08-12T20:29:00.632Z","msg":"metadata.finalizers: \"finalizers.fluxcd.io\": prefer a domain-qualified finalizer name to avoid accidental conflicts with other finalizer writers","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"git-repository-tools","namespace":"flux-system"},"namespace":"flux-system","name":"git-repository-tools","reconcileID":"cd52c749-bb3e-4e00-b5ee-fda6f4702e28"}
{"level":"info","ts":"2025-08-12T20:29:02.041Z","msg":"stored artifact for commit '(chore): generate random pass for tekton postgres ...'","controller":"gitrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"GitRepository","GitRepository":{"name":"git-repository-tools","namespace":"flux-system"},"namespace":"flux-system","name":"git-repository-tools","reconcileID":"396e4241-5f90-4c65-8c6f-2721cb49491f"} ScreenshotThe above scenario applies a |
this commit ensures that if ca.crt or caFile is available in the github app secret, a tls config with user provided certs is appended to system cert pool and passed to the underlying http transport Signed-off-by: abhijith-darshan <[email protected]> (chore): update target URL for TLSConfigFromSecret this commit ensures that the target URL for runtime/secrets.TLSConfigFromSecret has the scheme and host Signed-off-by: abhijith-darshan <[email protected]> (chore): adds test scenarios this commit adds test scenarios for mTLS GitHub app in reconcile source auth strategy Signed-off-by: abhijith-darshan <[email protected]> (chore): use runtime/secrets authMethods this commit ensures that GitHubApp secret resolution happens via pkg/runtime/secrets Signed-off-by: abhijith-darshan <[email protected]> (chore): update docs Signed-off-by: abhijith-darshan <[email protected]> (chore): adds github app data check this commit ensures that when provider is github and no github app data is present in the secret, it will error out with invalid configuration Signed-off-by: abhijith-darshan <[email protected]> (chore): removes getProxyOpts helper func this commit removes the helper method getProxyOpts and uses the standardized pkg/runtime/secrets APIs to get proxy options. Signed-off-by: abhijith-darshan <[email protected]> (chore): removes getProxyOpts test Signed-off-by: abhijith-darshan <[email protected]> (chore): improves test coverage Signed-off-by: abhijith-darshan <[email protected]> (chore): do not stall on missing github app data Signed-off-by: abhijith-darshan <[email protected]> (chore): adds a note on mTLS configuration in docs This commit mentions in the docs that if tls.crt and tls.key is part of the secret then mutual TLS configuration will be automatically enabled and should be used optionally. Signed-off-by: abhijith-darshan <[email protected]>
e685664
to
46522f9
Compare
@abhijith-darshan thanks for testing! Please port these changes to image-automation-controller so we can merge both. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🎉
Great work addressing all the review feedback!
@stefanprodan @matheuscscp - The PR on IAC is ready - fluxcd/image-automation-controller#947 |
If
ca.crt
orcaFile
is available in the GitHub App secret, atls
config with user provided certs is appended to system cert pool and passed to the underlying GitHub App transport.closes #1858