Support GitHub Apps Installation authentication

[kfcampbell]

👀 Rendered README changes 👀

The API for Apps initialization looks like:

client, err := pkg.NewApiClient(
    pkg.WithGitHubAppAuthentication("/path/to/pem/file.pem", "client-id", installationIDInteger),
)
if err != nil {
    log.Fatalf("error creating client: %v", err)
}

Future work is planned to make authentication for Apps meta endpoints that require a JWT transparent to the user.

Test coverage:

• Package pkg
  • Coverage: 98.1%
  • Test command: go test -v -coverpkg=./pkg -coverprofile=pkg.cov ./pkg

For comparison:

• Package authentication
  • Coverage: 93.5%
  • Test command: go test -v -coverpkg=./pkg/authentication/... -coverprofile=pkg.cov ./pkg/authentication/...
• Package handlers
  • Coverage: 88.5%
  • Test command: go test -v -coverpkg=./pkg/handlers/... -coverprofile=pkg.cov ./pkg/handlers/...

[github-actions]

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

[jamestelfer]

Apologies for the unasked-for commentary.

I'd love to see App support in this library, but I'm unsure about directly referencing the ghinstallation module. The current module directly references the github.com/google/go-github, making that SDK and indirect reference. It also currently does not support the ClientID style of configuration (though I hope to contribute that soon).

As used also, the only APIs accessible will be those that accept the installation token. APIs that require the App JWT itself will not work.

Could I suggest:

• pulling support into this library
• allowing for usage of either the application JWT directly or an application installation token

There is potential here to have a TokenProvider implementation for installation tokens that uses a separate TokenProvider to generation the application JWTs it needs. This would allow API consumers a straightforward way of providing an alternate App JWT TokenProvider that used an external KMS service to sign the JWT. The TokenProvider is a more flexible extension point to allow for such an implementation than is in the current ghinstallation.Signer API.

[kfcampbell]

@jamestelfer no worries at all, we appreciate and welcome the thoughtfulness! Let's discuss a little further if that's okay since I'm not quite sure I have a handle on what you're proposing.

Great point about the indirect reference to google/go-github -- that's an oversight on my part and something that we should avoid.

When you mention that you'd like to support the ClientID style of configuration, are you thinking of contributing that to ghinstallation or to google/go-github? I definitely agree that we should support the ClientID in this library as well.

I agree that we should support using an application JWT directly for those meta endpoints that don't support an App installation token. What I'm a bit unsure of is the best way to go about doing so: whether that's forking the library or rewriting its functionality ourselves. I'll have to do a little research on our policies before making a decision here.

Finally, can you talk to me a little more about your TokenProvider implementation idea? I don't think I follow. Philosophically, we'd like the go-sdk to be "batteries included" as much as possible and abstract away as many concerns from consumers as we can. Ideally a user wouldn't have to know whether an endpoint needs a JWT or an installation token; if they have an App-authenticated client we should be able to provide the correct auth headers for the endpoint they're calling. It sounds like the approach you're advocating requires the user to know and bring their own JWT and signer, which is a bit at odds with what I'm saying. Am I interpreting that correctly?

[jamestelfer]

When you mention that you'd like to support the ClientID style of configuration, are you thinking of contributing that to ghinstallation or to google/go-github? I definitely agree that we should support the ClientID in this library as well.

I was going to contribute that to ghinstallation. Given that the two are interchangeable, I'm looking to change the internal representation of the ID to a string, and allow that to be set to either the app ID or the client ID. Support for specifying it would be via the options API.

I agree that we should support using an application JWT directly for those meta endpoints that don't support an App installation token.

Ideally this information would be encoded using the x-github extensions to Open API present in the spec, so it's available unambiguously for all generated APIs.

I'll come back to the how on this one.

can you talk to me a little more about your TokenProvider implementation idea?

Fair enough: my description was sketchy at best 😀

Based on what may be a fairly shallow understanding of Kiota, it seems that the abstraction of the TokenProviderOption is the right spot to implement the JWT and token generation, rather than using the RoundTripper at the lower level. (Alternatively, the Kiota abstraction of the AuthenticationProvider could be used directly instead.)

There are implementations required of the TokenProviderOption then: one that implements JWT creation and the other that requests the installation token (using the JWT provider). And there is potentially another: a FilterProvider that selects the right implementation based on the outgoing request URL.¹

So kinda like:

flowchart LR
    A[Client] -->|Authenticate| B[FilterProvider]
    B --> C{URL auth type?}
    C -->|JWT| D[JWTProvider]
    C -->|Token| E[InstallationTokenProvider]
    E -->|create token via| D

Loading

Where the filter provider selects an authenticator based on the URL, the JWT provider creates and sets the JWT as the token, and the Token provider can make a call via the JWT provider to create its token as needed.

Given this separation, the API user need only provide the private key in order to get the behaviour they need, and batteries are included.

Given the options though, the JWT provider could be switched out by the API user for a variant that used a KMS service to sign the JWT, and the user could also ensure that the implementation held the JWT for a short period to avoid lots of KMS API calls. So they don't need to BYO JWT or Signer, but they can.

This is not dissimilar to ghinstallation, however:

• it uses the abstractions present for this library
• the JWT implementation can easily be switched out entirely
• client ID support can be baked in from the get-go

ghinstallation is a great example implementation, but implementing the functionality directly might be the right move for this library.

I hope this better explains what I was getting at, and I really appreciate the conversation!

Footnotes

1. I'm not sure if this is the right place to implement the authenticator choice, I haven't looked at it carefully enough ↩

[kfcampbell]

I was going to contribute that to ghinstallation. Given that the two are interchangeable, I'm looking to change the internal representation of the ID to a string, and allow that to be set to either the app ID or the client ID. Support for specifying it would be via the options API.

Nice! I've forked ghinstallation under my own name and done a little bit of cleanup with the intent to transfer the fork to the octokit organization soon. I've done something similar in kfcampbell/ghinstallation#5, and successfully tested it, which is cool. I'm not super in love with the function names and duplication, so if you can think of a more polished way to do so, I'm all ears (and welcome contributions)!

Ideally this information would be encoded using the x-github extensions to Open API present in the spec, so it's available unambiguously for all generated APIs.

I've also been thinking about the "how" on this one, and I've been hoping to avoid decorating the OpenAPI spec if possible just because of the friction involved, but if there's no other way, I think it would be feasible.

So kinda like:

It was weird to me when I implemented things in the PR that the PAT auth was a TokenProvider and the App auth worked at the RoundTripper level. I rationalized it at the time by thinking that the PAT stayed constant the whole time, whereas the App token needs to be renewed periodically, and the Kiota AuthenticateRequest interface function only gives us a *abs.RequestInformation to work with, we don't have enough information to renew the token. If there's a way to plumb the entire AppsTransport struct into the additionalAuthenticationContext map[string]interface{} map that's also provided in the AuthenticateRequest function, perhaps it would be more feasible to use the TokenProvider interface to do Apps, but I haven't figured out a way to do that yet. Please let me know if you have ideas!

Otherwise, the design of the FilterProvider you've proposed looks good to me.

[jamestelfer]

if you can think of a more polished way to do so

I summarized a proposed approach here: bradleyfalzon/ghinstallation#118. Whether or not this is more polished is an open question 😁 I can put this together in a PR. If you were looking to bring this into the octokit org, would it be an opportunity to consider breaking API changes, or ease transition by keeping the original API?

we don't have enough information to renew the token

TBH, I haven't looked closely at the RequestInformation struct to understand what's needed. It seems like I need to understand more and dive a bit deeper...

[jamestelfer]

I've added my ClientID thoughts as a (very) rough draft PR to ghinstallation: bradleyfalzon/ghinstallation#119

This can be further refined and tests included if the approach seems sound.

[kfcampbell]

would it be an opportunity to consider breaking API changes, or ease transition by keeping the original API?

For our purposes, I'm okay with breaking changes (and I did make them in the fork). Now would be the time to make them.

TBH, I haven't looked closely at the RequestInformation struct to understand what's needed.

Please let me know if you do! I'd be very curious to hear your thoughts.

I've added my ClientID thoughts as a (very) rough draft PR to ghinstallation: bradleyfalzon/ghinstallation#119

Nice! I've subscribed to both the issue and the PR you've created and will stay tuned.

[nickfloyd]

Other than the one nit about nesting this LGTM! ❤️

[nickfloyd]

The fluent syntax here makes me want to consider how we might be able to propagate that to other SDKs - it feels really good.