8000 Add snapshot taker for special `StandardUsernamePasswordCredentials` impls by jamesrobson-secondmind · Pull Request #327 · jenkinsci/credentials-plugin · GitHub
[go: up one dir, main page]
Skip to content

Add snapshot taker for special StandardUsernamePasswordCredentials impls #327

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&r 10000 dquo;, 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
Merged
merged 3 commits into from
Jul 1, 2022
Merged

Add snapshot taker for special StandardUsernamePasswordCredentials impls #327

merged 3 commits into from
Jul 1, 2022

Conversation

jamesrobson-secondmind
Copy link
Contributor

This adds a snapshot taker for StandardUsernamePasswordCredentials. This is needed to fix an issue in hashicorp-vault plugin which currently prevents ssh keys being sent to agents which is being developed in jenkinsci/hashicorp-vault-plugin#218

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@jetersen
Copy link
Member

cc @jglick

@jglick jglick requested a review from jtnord June 23, 2022 19:04
jglick
jglick approved these changes Jun 23, 2022
View reviewed changes
Copy link
Member
@jglick jglick left a comment

Choose a reason for hiding this comment

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

Looks right.

jglick
jglick previously requested changes Jun 23, 2022
View reviewed changes
Copy link
Member
@jglick jglick left a comment

Choose a reason for hiding this comment

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

Ah no, it fails to copy the usernameSecret field.

I would suggest just returning the argument unchanged if it happens to be a UsernamePasswordCredentialsImpl.

@jglick jglick mentioned this pull request Jun 23, 2022
Merged
6 tasks
@jglick jglick changed the title Add snapshot taker for StandardUsernamePasswordCredentials Add snapshot taker for special StandardUsernamePasswordCredentials impls Jun 24, 2022
jglick
jglick reviewed Jun 24, 2022
View reviewed changes
...in/java/com/cloudbees/plugins/credentials/impl/UsernamePasswordCredentialsSnapshotTaker.java Outdated
if (credentials instanceof UsernamePasswordCredentialsImpl) {
return credentials;
}
return new UsernamePasswordCredentialsImpl(credentials.getScope(), credentials.getId(), credentials.getDescription(), credentials.getUsername(), Secret.toString(credentials.getPassword()));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return new UsernamePasswordCredentialsImpl(credentials.getScope(), credentials.getId(), credentials.getDescription(), credentials.getUsername(), Secret.toString(credentials.getPassword()));
UsernamePasswordCredentialsImpl snapshot = new UsernamePasswordCredentialsImpl(credentials.getScope(), credentials.getId(), credentials.getDescription(), credentials.getUsername(), Secret.toString(credentials.getPassword()));
snapshot.setUsernameSecret(credentials.isUsernameSecret());
return snapshot;

just in case other impls now implement this parameter as well.

@jglick jglick added the bug label Jun 24, 2022
@jglick jglick dismissed their stale review June 29, 2022 17:06

Main problem addressed. A minor suggestion remains.

@jamesrobson-secondmind
Copy link
Contributor Author

We have a confirmation of this change working with vault (jenkinsci/hashicorp-vault-plugin#201 (comment)), from commit e331be4. I've tested the following changes my self so we should now be good to go.

@jglick jglick enabled auto-merge July 1, 2022 18:15
@jglick jglick merged commit eb9579f into jenkinsci:master Jul 1, 2022
@jetersen jetersen mentioned this pull request Jul 26, 2022
Closed
@jtnord
Copy link
Member
jtnord commented Aug 31, 2022

noting this seemingly breaks GitHubAppCredentials as it claims support for them and snapshots them 😱

org.jenkinsci.plugins.github_branch_source.GitHubAppCredentials.generateAppInstallationToken(GitHubAppCredentials.java:216)
  at org.jenkinsci.plugins.github_branch_source.GitHubAppCredentials.getToken(GitHubAppCredentials.java:298)
  at org.jenkinsci.plugins.github_branch_source.GitHubAppCredentials.getPassword(GitHubAppCredentials.java:327)
  at com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsSnapshotTaker.snapshot(UsernamePasswordCredentialsSnapshotTaker.java:28)
  at com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsSnapshotTaker.snapshot(UsernamePasswordCredentialsSnapshotTaker.java:9)
  at com.cloudbees.plugins.credentials.CredentialsProvider.snapshot(CredentialsProvider.java:821)

@jglick
Copy link
Member
jglick commented Aug 31, 2022 •
edited
Loading

@jtnord by “breaks” do you mean specifically the token refresh https://github.com/jenkinsci/github-branch-source-plugin/blob/7618247e672d2008fa8dc006ee8e87f4e64ab2c4/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java#L497?

If so, I guess the correct fix is to replace writeReplace with a new CredentialsSnapshotTaker taking precedence over UsernamePasswordCredentialsSnapshotTaker.

credentials-plugin/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java

Lines 808 to 814 in be8bbce

Class bestType = null;
CredentialsSnapshotTaker bestTaker = null;
for (CredentialsSnapshotTaker taker : ExtensionList.lookup(CredentialsSnapshotTaker.class)) {
if (clazz.isAssignableFrom(taker.type()) && taker.type().isInstance(credential)) {
if (bestTaker == null || bestType.isAssignableFrom(taker.type())) {
bestTaker = taker;
bestType = taker.type();
should make this straightforward: no need for https://javadoc.jenkins.io/hudson/Extension.html#ordinal()

@jtnord
Copy link
Member
jtnord commented Aug 31, 2022 •
edited
Loading

@jtnord by “breaks” do you mean specifically the token refresh https://github.com/jenkinsci/github-branch-source-plugin/blob/7618247e672d2008fa8dc006ee8e87f4e64ab2c4/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java#L497?

exactly that.

If so, I guess the correct fix is to replace writeReplace with a new CredentialsSnapshotTaker taking precedence over UsernamePasswordCredentialsSnapshotTaker.

credentials-plugin/src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java

Lines 808 to 814 in be8bbce

Class bestType = null;
CredentialsSnapshotTaker bestTaker = null;
for (CredentialsSnapshotTaker taker : ExtensionList.lookup(CredentialsSnapshotTaker.class)) {
if (clazz.isAssignableFrom(taker.type()) && taker.type().isInstance(credential)) {
if (bestTaker == null || bestType.isAssignableFrom(taker.type())) {
bestTaker = taker;
bestType = taker.type();

should make this straightforward: no need for https://javadoc.jenkins.io/hudson/Extension.html#ordinal()

I'm working on it, there be some dragons I am trying to understand first.

@jglick
Copy link
Member
jglick commented Aug 31, 2022

See discussion in jenkinsci/github-branch-source-plugin#334.

@jtnord jtnord mentioned this pull request Sep 1, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jglick jglick jglick approved these changes

@jtnord jtnord jtnord approved these changes

@jetersen jetersen jetersen approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jamesrobson-secondmind @jetersen @jtnord @jglick
0