Skip to content

Conversation

@laurayco
Copy link
Collaborator

@laurayco laurayco commented Jun 10, 2025

Original intent: There's already mitigation in the UI, however this would not stop a nefarious actor who sends a POST request without the UI; this is intended to cover that case.

The need for GitHub app support for cloning usurps these mitigations, so this PR now builds off that idea and incorporating the GitHub app's auth, with additional design considerations for similar auth patterns for other services (e.g. a GitLab auth provider).

The sequence for cloning with auth from a GitHub app installation is:

sequenceDiagram
    participant concord-agent
    participant concord-server
    participant token-cache

    concord-server->>+token-cache: get token for repo
    token-cache-->>token-cache: cached installation token for repo
    token-cache-->>token-cache: validate token expiration
    token-cache-->>+GitHub: or Generate new token
    GitHub-->>-token-cache: new 1-hour installation client token
    token-cache->>-concord-server: installation token
    concord-server->>+concord-server: clone repo
    concord-server->>+concord-agent: dispatch workflow
    concord-agent->>+concord-server: lookup token
    concord-server->>+token-cache: retrieve token
    token-cache-->>token-cache: cached installation token for repo
    token-cache-->>token-cache: validate token expiration
    token-cache-->>+GitHub: or Generate new token
    GitHub-->>-token-cache: new 1-hour installation client token
    token-cache->>-concord-server: installation token
    concord-server->>+concord-agent: installation token
    concord-agent->>+concord-agent: clone repo
    concord-agent->>+concord-agent: start workflow runner
Loading

@laurayco laurayco added security Security issues wip Work in progress, do not merge labels Jul 1, 2025
@benbroadaway benbroadaway changed the title add a whitelist for repo URI schemes to prevent abuse. server, agent: GitHub app installation support for repository cloning Sep 22, 2025
Copy link
Collaborator

@ibodrov ibodrov left a comment

Choose a reason for hiding this comment

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

Still 👀 through it, got a couple of Qs

}

/**
* Refresh repositories by their IDs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment doesn't match the method.


@Value.Immutable
@Value.Style(jdkOnly = true, redactedMask = "**redacted**")
interface CacheKey {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
interface CacheKey {
record CacheKey(URI repoUri, int weight)

maybe? Calculate the weight when the key is created. Get rid of immutables and there won't be anything secret to print in the first place.

@Value.Immutable
@Value.Style(jdkOnly = true)
@JsonDeserialize(as = ImmutableGitHubAppAuthConfig.class)
public interface GitHubAppAuthConfig extends MappingAuthConfig {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be a record as well? Do the sanity check in the constructor?

@ibodrov
Copy link
Collaborator

ibodrov commented Oct 27, 2025

Any chance to extract git.allowedSchemes into a separate PR?

@brig
Copy link
Contributor

brig commented Oct 27, 2025

I’d even suggest splitting the server and agent parts into separate PRs...


<sql>
insert into ROLE_PERMISSIONS values (
(select ROLE_ID from ROLES where ROLE_NAME = 'concordAdmin'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think only agents are going to need this permission? If so then should we create an agent role?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

security Security issues

Development

Successfully merging this pull request may close these issues.

6 participants