Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make GitHubAppCredentials#writeReplace protected for subclasses #810

Merged
merged 1 commit into from
Oct 23, 2024
Merged

Make GitHubAppCredentials#writeReplace protected for subclasses #810

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ long getTokenStaleEpochSeconds() {
* <li>The agent need not be able to contact GitHub.
* </ul>
*/
private Object writeReplace() {
protected Object writeReplace() {
Copy link
Member

@jtnord jtnord Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

TBH this all seems a bit messy to me.
Why would this just not be in a SnapshotTaker for GitHubAppCredentials?
The refresh can make a call to the controller if on an agent or make a local call if it is on the controller... (then it is just the DelegatingGitHubAppCredentials created by the SnapShotTaker that needs to have a readResolve to set a channel (but only if used on an agent, and then making a local call if on a controller).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm I see what you mean. When playing around though, you quickly notice that the DelegatingGitHubAppCredentials does not extend GitHubAppCredentials and that's maybe why things started to get messy. It does not seem like you can snapshot a GitHubAppCredentials to a DelegatingGitHubAppCredentials with the current design ? Maybe it requires some superclass that hold the "source" for refreshing the token. One source implementation would have appId, privateKey, apiUri and owner and the other the encrypted form of those (what DelegatingGitHubAppCredentials has).

FYI, my proposed change is to fix the serialization in an upstream implementation. Without changing anything upstream. Maybe we can address the snapshotting problem as part of a different PR. I can create a JENKINS issue for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://issues.jenkins.io/browse/JENKINS-73787

Copy link
Member

@jtnord jtnord Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

DelegatingGitHubAppCredentials does not extend GitHubAppCredentials and that's maybe why things started to get messy. It does not seem like you can snapshot a GitHubAppCredentials to a DelegatingGitHubAppCredentials with the current design ?

snapshot should return (an implementation of) the interface type (because credential types should be written using interfaces as per the design doc) which in this case would be StandardUsernamePasswordCredentials so AFAICT without writing code this should be possible.
if code really does need a specific type for a GitHubApp credential (which is likely) then the GitHubAppCredential should not be a concrete type but an interface with an implementaion. (doc)

Copy link
Member

Choose a reason for hiding this comment

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

(for the avoidance of doubt I am not blocking this PR, but I do not feel like the addition of debt onto the debt makes me want to approve it either)

Copy link
Member

Choose a reason for hiding this comment

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

if code really does need a specific type for a GitHubApp credential (which is likely)

No, it is just used as an implementation of StandardUsernamePasswordCredentials that happens to create a “password” on demand.

Copy link
Member

@dwnusbaum dwnusbaum Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

TL;DR: Unless I am missing something obvious, this PR seems like the simplest way to fix the issue without causing more problems.

The motivation behind this change is to fix the behavior of a CloudBees-specific subclass of GitHubAppCredentials that comes from a plugin that implements a special CredentialsProvider that stores the relevant secrets externally (e.g. the private key). Right now, those credentials may not be used on agents directly.

The main problem is that we need multiple levels of dynamic behavior for GitHub app credentials:

  • We need to support dynamic contextualization of owner via Credentials.forRun and Connector.lookupScanCredentials
    • This requires us to use an instance of GitHubAppCredentials right up until the actual password is going to be used
    • For CloudBees, this means that any CredentialsSnapshotTaker for a GitHubAppCredentials must translate to a type that is still a GitHubAppCredentials (which is not the case for DelegatingGitHubAppCredentials), otherwise owner inference totally breaks when credentials are sent from CloudBees CI Operations Center to controllers (this kind of problem was the motivation behind [JENKINS-73388] Allow alternative implementation for GitHub App credentials #796)
  • We want to support dynamic refresh of the token from agents without the agents having access to the GitHub app secrets directly
    • This requires us to replace GitHubAppCredentials with a type like DelegatingGitHubAppCredentials when serializing the credentials to an agent

Making writeReplace protected here allows the CloudBees-specific type to use a CredentialsSnapshotTaker to produce an instance of GitHubAppCredentials where the external secrets have been retrieved, which does not break owner inference for the snapshot, and does not prevent DelegatingGitHubAppCredentials from being used for the credentials and the snapshot on agents.

If we try to use CredentialsSnapshotTaker here and delete writeReplace, things get more complicated. We need a way to tell in our CredentialsSnapshotTaker implementation whether the snapshot is happening because the credentials are being sent to an agent (the only case for OSS Jenkins) or is because the credentials are being sent from OC to controllers (only for CloudBees). This is not just a problem for the special GitHubAppCredentials subtype with CloudBees CI, it also applies to usage of plain GitHubAppCredentials in CloudBees CI (e.g. #616 is somewhat related to this kind of problem). The problem though is that Channel.current() returns null from inside of CredentialsSnapshotTaker.snapshot (at least when checking testAgentRefresh here), so I don't really see any simple way to make this work without writeReplace.

Now, are there bigger changes that could be made to avoid this issue? Maybe. For example, if DelegatingAppCredentials was itself a subclass of GitHubAppCredentials and supported owner inference, then we could always translate directly to it in CredentialsSnapshotTaker. I am not sure if that can be done safely though given the need for agents to be able to refresh the token dynamically. Additionally, the CloudBees-specific subtype of GitHubAppCredentials could maybe be deleted with a redesign of the relevant plugin, but we'd still have CloudBees-specific issues with usage of regular GitHubAppCredentials if we tried to unify GitHubAppCredentialsSnapshotTaker and GitHubAppCredentials.writeReplace.

if (
/* XStream */ Channel.current() == null) {
return this;
Expand Down