Do not enforce GitHub app to comes from the same org as the repo org. The GH app can come from another org as log as it is installed in the org with the target git repo there is no security issue

[olamy]

Signed-off-by: Olivier Lamy [email protected]

Description

A brief summary describing the changes in this pull request. See
JENKINS-XXXXX for further information.

Submitter checklist

• Link to JIRA ticket in description, if appropriate.
• Change is code complete and matches issue description
• Automated tests have been added to exercise the changes
• Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

• Run the changes and verify that the change matches the issue description
• Reviewed the code
• Verified that the appropriate tests have been written or valid explanation given

Documentation changes

• Link to jenkins.io PR, or an explanation for why no doc changes are needed

Users/aliases to notify

[jglick]

This does not look right. I suspect the configuration problem was misdiagnosed. Perhaps the owner field was set incorrectly in the Jenkins credentials? Or could simply have been left blank?

[olamy]

This does not look right. I suspect the configuration problem was misdiagnosed. Perhaps the owner field was set incorrectly in the Jenkins credentials? Or could simply have been left blank?

@jglick no the owner of the GH App Jenkins credentials is not blank but have another org.
Setup is:

• GH App from org a
• Jenkins credentials (GH App ) A with owner field to org b
• GH App installed in org a, b and c
• mbp 1 from org b using the this cred A works
• mbp 2 from org c using cred A not working.

Well I could create cred B with same key but owner org c.
But why? I don't understand the reason of the restriction. As long as the App is installed and validated by owners of orgs, I can't see the possible security issue.

[olamy]

@jglick any comment or it's all good for you?

[netomi]

I also have the feeling this fix is not correct. The plugin seems to be able to support multiple installations of the app and handles that, e.g. in the generateAppInstallationToken, however, the withOwner method seems require that if the owner field is set, you can only use it for repos that have the same owner, i.e. the app must be owned by the owner of the repo.

The fix will return a GitHubAppCredentials instance which has an owner different from the owner of the repo that it should access. In the generateAppInstallationToken method, when we generate the actual installation token to access the repo, it will only work correctly, if the app has exactly a single installation. If there are multiple installations, a different code path is used which will look for installations matching the current owner.

So I believe the correct fix would be something like that:

public synchronized GitHubAppCredentials withOwner(@NonNull String owner) {
    // considering that owner must be non-null
    if (owner.equals(this.owner)) {
        return this;
    }
    // potential add a safe-guard to check if there is an app installation for the owner
    // if you want to fail fast, but it would be caught later on: https://github.com/jenkinsci/github-branch-source-plugin/blob/a1eaf809b29e203dfa9ab62cdf1e414711b81c0e/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java#L238
    .... // existing code
}

[olamy]

@netomi sounds a good catch. would you provide a PR for it?

[netomi]

I can work on a improved fix, but would like to test the case that I described so might take a bit.

[netomi]

so I tested the current fix:

• set up a GitHub app that is owned by org A and install it on org A and B
• setup the GitHubApp credentials in jenkins using the App with A as owner
• setup an Organization Item for org B using this GitHubApp credential

it works, however, it will use tokens that are created for the app installation of org A. As long as the plugin only accesses public information you will not notice a problem as any token can access them, but if the plugin tries to access restricted data you will see failures (e.g. setting up a webhook).

With the fix as outlined above this works as expected, tokens are created for the right app installation based on the required owner organization:

--- a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java
+++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java
@@ -325,7 +325,7 @@ public class GitHubAppCredentials extends BaseStandardCredentials implements Sta
 
     @NonNull
     public synchronized GitHubAppCredentials withOwner(@NonNull String owner) {
-        if (this.owner != null) {
+        if (this.owner != null && this.owner.equals(owner)) {
             return this;
         }
         if (byOwner == null) {

I can easily create a PR to fix that, however I am unsure if I should add a test for this. There is an existing test for this class but it mainly uses mocking which I guess is not of much help in this case.

[olamy]

@netomi sounds like a good fix. Yes it's not easy to create tests in this area. Let''s have a PR first!
Thanks

[jtnord]

This does not look right. I suspect the configuration problem was misdiagnosed. Perhaps the owner field was set incorrectly in the Jenkins credentials? Or could simply have been left blank?

So this was actually doing exactly what it was designed to - the Credential was only being allowed to use the org (owner) that it was configured for. You should just delete the owner if you do not want to restrict the credential usage.

[netomi]

That is the reason why an owner has to be configured when setting up the GitHubAppCredential from the start:

Now the plugin itself deals with multiple installations and its also written in the documentation that this is working in general. However the existing code is in contradiction to that, thus the fixes.

[jglick]

(some more discussion in #745 (comment))

[jtnord]

That is the reason why an owner has to be configured when setting up the GitHubAppCredential from the start:

created https://issues.jenkins.io/browse/JENKINS-72330