Skip to content
Open
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
9 changes: 9 additions & 0 deletions api/v1alpha1/task_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,15 @@ type TaskSpec struct {
// PodOverrides allows customizing the agent pod configuration.
// +optional
PodOverrides *PodOverrides `json:"podOverrides,omitempty"`

// Repositories limits the GitHub App installation token to these
// repository names. When set and the workspace uses GitHub App
// authentication, the generated GITHUB_TOKEN can only access the
// listed repositories. Ignored when the workspace uses a PAT.
// Repository names are relative to the installation owner
// (e.g., "my-repo", not "org/my-repo").
// +optional
Repositories []string `json:"repositories,omitempty"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Because a taks is already scoped to a workspace which lists a repository is there a reason to duplicate that information into the taskSpawner rather than use the workspace repo to scope the token?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good question. The workspace does list a repo, but the installation token the GitHub App generates has access to every repo the App is installed on — not just the workspace repo. So without explicit scoping, an agent working on `org/frontend` can also read/write `org/infrastructure`.

Auto-deriving from the workspace repo would cover the common case well. The explicit `repositories` field is for cases where they diverge:

  • Agent needs access to additional repos (cross-repo dependency updates)
  • Workspace points to a fork but the agent also needs upstream API access
  • Orchestrator wants to enforce per-user access boundaries (e.g., a chatbot restricting the token to repos the triggering user can reach)

Happy to add auto-scoping from the workspace as the default, with `repositories` as an opt-in override for the above cases. Would that work for you?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I won't speak for @gjkim42 but I think using the existing repo field to scope the token, then expand to: #842 when that lands.

The upstream access is also solved by ensuring the token has access to remotes on the workspace.

Orchestrator wants to enforce per-user access boundaries (e.g., a chatbot restricting the token to repos the triggering user can reach)

This one is trickier.

In general I think we have the fields needed to scope the tokens already. We may want to put it behind a configuration flag to enable or disable (for backwards compatibility and can remove that in V1, for example).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed — removed Repositories from both TaskSpec and TaskTemplate. The controller now auto-derives the repo list from workspace.Spec.Repo + workspace.Spec.Remotes[].URL and passes them to the installation token API.

Net result: -70 lines, no new API surface. Cross-repo access via readOnlyWorkspaces (#842) can extend the token scope when it lands.

}

// TaskStatus defines the observed state of Task.
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion internal/controller/task_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,10 @@ func (r *TaskReconciler) resolveGitHubAppToken(ctx context.Context, task *kelosv
}
}

tokenResp, err := tc.GenerateInstallationToken(ctx, creds)
opts := &githubapp.TokenOptions{
Repositories: task.Spec.Repositories,
}
tokenResp, err := tc.GenerateInstallationToken(ctx, creds, opts)
if err != nil {
return nil, fmt.Errorf("generating installation token: %w", err)
}
Expand Down
26 changes: 23 additions & 3 deletions internal/githubapp/token.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package githubapp

import (
"bytes"
"context"
"crypto"
"crypto/rand"
Expand Down Expand Up @@ -51,6 +52,14 @@ type TokenResponse struct {
ExpiresAt time.Time
}

// TokenOptions configures optional scoping for installation tokens.
type TokenOptions struct {
// Repositories limits the token to these repository names.
// Names are relative to the installation owner (e.g., "my-repo",
// not "org/my-repo").
Repositories []string `json:"repositories,omitempty"`
}

// TokenClient generates GitHub App installation tokens.
type TokenClient struct {
BaseURL string
Expand Down Expand Up @@ -125,14 +134,25 @@ func parsePrivateKey(pemBytes []byte) (*rsa.PrivateKey, error) {
}

// GenerateInstallationToken exchanges GitHub App credentials for an installation token.
func (tc *TokenClient) GenerateInstallationToken(ctx context.Context, creds *Credentials) (*TokenResponse, error) {
// When opts is non-nil and contains Repositories, the token is scoped to those repositories.
func (tc *TokenClient) GenerateInstallationToken(ctx context.Context, creds *Credentials, opts *TokenOptions) (*TokenResponse, error) {
jwt, err := generateJWT(creds)
if err != nil {
return nil, fmt.Errorf("generating JWT: %w", err)
}

url := fmt.Sprintf("%s/app/installations/%s/access_tokens", tc.baseURL(), creds.InstallationID)
req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, nil)

var body io.Reader
if opts != nil && len(opts.Repositories) > 0 {
payload, err := json.Marshal(opts)
if err != nil {
return nil, fmt.Errorf("marshaling token options: %w", err)
}
body = bytes.NewReader(payload)
}

req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, body)
Comment thread
omercnet marked this conversation as resolved.
if err != nil {
return nil, fmt.Errorf("creating request: %w", err)
}
Expand Down Expand Up @@ -242,7 +262,7 @@ func (tp *TokenProvider) Token(ctx context.Context) (string, error) {
return tp.token, nil
}

resp, err := tp.client.GenerateInstallationToken(ctx, tp.creds)
resp, err := tp.client.GenerateInstallationToken(ctx, tp.creds, nil)
if err != nil {
// Fall back to cached token if it has not actually expired yet
if tp.token != "" && now.Before(tp.expiresAt) {
Expand Down
122 changes: 120 additions & 2 deletions internal/githubapp/token_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"encoding/json"
"encoding/pem"
"fmt"
"io"
"net/http"
"net/http/httptest"
"testing"
Expand Down Expand Up @@ -230,7 +231,7 @@ func TestGenerateInstallationToken(t *testing.T) {
Client: server.Client(),
}

resp, err := tc.GenerateInstallationToken(context.Background(), creds)
resp, err := tc.GenerateInstallationToken(context.Background(), creds, nil)
if err != nil {
t.Fatalf("GenerateInstallationToken: %v", err)
}
Expand Down Expand Up @@ -417,8 +418,125 @@ func TestGenerateInstallationToken_Error(t *testing.T) {
Client: server.Client(),
}

_, err = tc.GenerateInstallationToken(context.Background(), creds)
_, err = tc.GenerateInstallationToken(context.Background(), creds, nil)
if err == nil {
t.Error("expected error for 401 response")
}
}

func TestGenerateInstallationToken_WithRepositories(t *testing.T) {
_, keyPEM := generateTestKey(t)

expiresAt := time.Now().Add(1 * time.Hour).UTC().Truncate(time.Second)

var receivedBody map[string]interface{}
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
body, err := io.ReadAll(r.Body)
if err != nil {
t.Fatalf("reading request body: %v", err)
}
if len(body) > 0 {
if err := json.Unmarshal(body, &receivedBody); err != nil {
t.Fatalf("unmarshaling request body: %v", err)
}
}

w.WriteHeader(http.StatusCreated)
json.NewEncoder(w).Encode(map[string]interface{}{
"token": "ghs_scoped_token",
"expires_at": expiresAt.Format(time.RFC3339),
})
}))
defer server.Close()

creds, err := ParseCredentials(map[string][]byte{
"appID": []byte("12345"),
"installationID": []byte("67890"),
"privateKey": keyPEM,
})
if err != nil {
t.Fatalf("ParseCredentials: %v", err)
}

tc := &TokenClient{
BaseURL: server.URL,
Client: server.Client(),
}

opts := &TokenOptions{
Repositories: []string{"my-repo", "other-repo"},
}
resp, err := tc.GenerateInstallationToken(context.Background(), creds, opts)
if err != nil {
t.Fatalf("GenerateInstallationToken: %v", err)
}

if resp.Token != "ghs_scoped_token" {
t.Errorf("Token = %q, want %q", resp.Token, "ghs_scoped_token")
}

// Verify the request body contained the repositories
repos, ok := receivedBody["repositories"]
if !ok {
t.Fatal("request body missing 'repositories' field")
}
repoList, ok := repos.([]interface{})
if !ok {
t.Fatalf("repositories is not an array: %T", repos)
}
if len(repoList) != 2 {
t.Errorf("repositories length = %d, want 2", len(repoList))
}
if repoList[0] != "my-repo" || repoList[1] != "other-repo" {
t.Errorf("repositories = %v, want [my-repo other-repo]", repoList)
}
}

func TestGenerateInstallationToken_NilOpts(t *testing.T) {
_, keyPEM := generateTestKey(t)

expiresAt := time.Now().Add(1 * time.Hour).UTC().Truncate(time.Second)

var requestBodyEmpty bool
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
body, _ := io.ReadAll(r.Body)
requestBodyEmpty = len(body) == 0

w.WriteHeader(http.StatusCreated)
json.NewEncoder(w).Encode(map[string]interface{}{
"token": "ghs_unscoped",
"expires_at": expiresAt.Format(time.RFC3339),
})
}))
defer server.Close()

creds, err := ParseCredentials(map[string][]byte{
"appID": []byte("12345"),
"installationID": []byte("67890"),
"privateKey": keyPEM,
})
if err != nil {
t.Fatalf("ParseCredentials: %v", err)
}

tc := &TokenClient{BaseURL: server.URL, Client: server.Client()}

// nil opts should send no body (backward compatible)
_, err = tc.GenerateInstallationToken(context.Background(), creds, nil)
if err != nil {
t.Fatalf("GenerateInstallationToken: %v", err)
}
if !requestBodyEmpty {
t.Error("expected empty request body for nil opts")
}

// Empty repositories should also send no body
requestBodyEmpty = false
_, err = tc.GenerateInstallationToken(context.Background(), creds, &TokenOptions{})
if err != nil {
t.Fatalf("GenerateInstallationToken with empty opts: %v", err)
}
if !requestBodyEmpty {
t.Error("expected empty request body for empty opts")
}
}
11 changes: 11 additions & 0 deletions internal/manifests/charts/kelos/templates/crds/task-crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,17 @@ spec:
prompt:
description: Prompt is the task prompt to send to the agent.
type: string
repositories:
description: |-
Repositories limits the GitHub App installation token to these
repository names. When set and the workspace uses GitHub App
authentication, the generated GITHUB_TOKEN can only access the
listed repositories. Ignored when the workspace uses a PAT.
Repository names are relative to the installation owner
(e.g., "my-repo", not "org/my-repo").
items:
type: string
type: array
ttlSecondsAfterFinished:
description: |-
TTLSecondsAfterFinished limits the lifetime of a Task that has finished
Expand Down
11 changes: 11 additions & 0 deletions internal/manifests/install-crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,17 @@ spec:
prompt:
description: Prompt is the task prompt to send to the agent.
type: string
repositories:
Comment thread
omercnet marked this conversation as resolved.
Outdated
description: |-
Repositories limits the GitHub App installation token to these
repository names. When set and the workspace uses GitHub App
authentication, the generated GITHUB_TOKEN can only access the
listed repositories. Ignored when the workspace uses a PAT.
Repository names are relative to the installation owner
(e.g., "my-repo", not "org/my-repo").
items:
type: string
type: array
ttlSecondsAfterFinished:
description: |-
TTLSecondsAfterFinished limits the lifetime of a Task that has finished
Expand Down
Loading