feat: add mTLS certificate-based Git authentication support for TestWorkflows#6871
feat: add mTLS certificate-based Git authentication support for TestWorkflows#6871dhimanAbhi wants to merge 2 commits intokubeshop:mainfrom
Conversation
Signed-off-by: Abhishek Dhiman <abhi2002dhiman@gmail.com>
Signed-off-by: Abhishek Dhiman <abhi2002dhiman@gmail.com>
|
@greptile |
Greptile SummaryAdded comprehensive mTLS certificate-based Git authentication support for TestWorkflows, enabling Testkube to clone repositories requiring mutual TLS authentication. Key Changes:
Issues Found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant API
participant Processor
participant Resolver
participant Toolkit
participant Git
User->>API: Submit TestWorkflow with mTLS certs
API->>Resolver: Extract credentials from spec
Resolver->>Resolver: Process CaCertFrom, ClientCertFrom, ClientKeyFrom
Resolver->>API: Return externalized secrets
API->>Processor: Process TestWorkflow
Processor->>Processor: Build clone command with cert args
Processor->>Toolkit: Execute clone with -c, -e, -k flags
Toolkit->>Toolkit: Create temp files for certs/keys
Toolkit->>Toolkit: Build git config args (http.sslCAInfo, http.sslCert, http.sslKey)
Toolkit->>Git: Execute git clone with cert config
Git->>Git: Perform TLS handshake with certificates
Git-->>Toolkit: Clone complete
Toolkit->>Toolkit: Cleanup temp cert files
Toolkit-->>Processor: Success
Processor-->>API: Workflow execution complete
Last reviewed commit: e62cdc2 |
|
|
||
| if err := os.WriteFile(keyPath, []byte(key), 0400); err != nil { | ||
| _ = os.Remove(keyPath) | ||
| return func() {}, "", fmt.Errorf("writing SSH key: %w", err) |
There was a problem hiding this comment.
error message says "writing SSH key" but this function is generic and used for certificates too
| return func() {}, "", fmt.Errorf("writing SSH key: %w", err) | |
| return func() {}, "", fmt.Errorf("writing key file: %w", err) |
| if opts.ClientCert != "" && opts.ClientKey != "" { | ||
| cleanupclientCertFile, clientCertFilePath, err := CreateTempFile(opts.ClientCert, "client-cert-*") | ||
| if err != nil { | ||
| return nil, cleanupFuncs, fmt.Errorf("creating temp file for Client Certificate: %w", err) | ||
| } | ||
| certAuthArgs = append(certAuthArgs, "-c", fmt.Sprintf("http.sslCert=%s", clientCertFilePath)) | ||
| cleanupFuncs = append(cleanupFuncs, cleanupclientCertFile) | ||
|
|
||
| cleanupclientKeyFile, clientKeyFilePath, err := CreateTempFile(opts.ClientKey, "client-key-*") | ||
| if err != nil { | ||
| return nil, cleanupFuncs, fmt.Errorf("creating temp file for Client Key: %w", err) | ||
| } | ||
| certAuthArgs = append(certAuthArgs, "-c", fmt.Sprintf("http.sslKey=%s", clientKeyFilePath)) | ||
| cleanupFuncs = append(cleanupFuncs, cleanupclientKeyFile) | ||
| } |
There was a problem hiding this comment.
if only clientCert is provided without clientKey (or vice versa), git will fail but no validation error occurs
add validation:
} else if opts.ClientCert != "" || opts.ClientKey != "" {
return nil, cleanupFuncs, fmt.Errorf("both clientCert and clientKey must be provided together")
}
vsukhin
left a comment
There was a problem hiding this comment.
thank you @dhimanAbhi please check my and greptile comments. Also did you test it in k8s cluster? Is size still good enough for client CRDs?
| // authorization type for the credentials | ||
| AuthType testsv3.GitAuthType `json:"authType,omitempty" expr:"template"` | ||
| // plain text CA certificate to verify repository TLS connection | ||
| CaCert string `json:"caCert,omitempty"` |
There was a problem hiding this comment.
why didn't you add expr metatag there?
| // plain text CA certificate to verify repository TLS connection | ||
| CaCert string `json:"caCert,omitempty"` | ||
| // external CA certificate to verify repository TLS connection | ||
| CaCertFrom *corev1.EnvVarSource `json:"caCertFrom,omitempty" expr:"force"` |
| SshKey string `json:"sshKey,omitempty"` | ||
| SshKeyFrom *EnvVarSource `json:"sshKeyFrom,omitempty"` | ||
| AuthType *ContentGitAuthType `json:"authType,omitempty"` | ||
| // plain text CA certificate to verify repository TLS connection |
There was a problem hiding this comment.
I don't see corresponding OpenAPI spec changes
|
@dhimanAbhi waiting for your feedback on review questions |
|
Hi @olensmar, thanks for the reminder. I got pulled into another task earlier, but I’ll address the requested changes and update this PR by the end of the week. |
Pull request description
This PR adds full support for certificate-based Git authentication (mTLS) in Testkube TestWorkflows.
This addresses the missing client-certificate functionality mentioned in #4588 , enabling Testkube to clone Git repositories that require mutual TLS authentication.
New Fields Introduced
caCert: CA certificate that Testkube should trust when establishing TLS connections to the Git server.caCertFrom: External CA certificate that Testkube should trust when establishing TLS connections to the Git server.clientCert: Client certificate that the Testkube presents during TLS handshake.clientCertFrom: External Client certificate that the Testkube presents during TLS handshake.clientKey: Private key that corresponds to clientCertclientKeyFrom: External Private key that corresponds to clientCertThese fields map to git options:
-c http.sslCAInfo=<"path">
-c http.sslCert=<"path">
-c http.sslKey=<"path">
Example Testworkflow Usage
Proof Manifests
I tested this feature by setting up a local Gitea Git server and placing an Nginx reverse proxy in front of it to enforce full mTLS. The server certificate and the client certificate used by Testkube were both signed by the same CA. I created a test repository containing a Cypress test suite (copied from test/cypress/cypress-12 in the official Testkube repo), and my TestWorkflow was able to successfully authenticate with mTLS and fetch the repository contents using the new certificate fields.
Checklist (choose whats happened)
Breaking changes
Changes
Fixes