Skip to content

Avoid caching the credential, instead get the credential value whenever it is required through the request filter #1712

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
Merged
krisstern merged 8 commits into from
Oct 24, 2024
Merged

Avoid caching the credential, instead get the credential value whenever it is required through the request filter #1712

krisstern merged 8 commits into from
Oct 24, 2024

Conversation

@arechavarria
Copy link
Contributor

@arechavarria arechavarria commented Oct 22, 2024 •
edited by krisstern
Loading

Fixes #963.

Description

This pull request introduces a new GitlabCredentialResolver class to manage the retrieval of credentials for GitLab connections more centrally and securely within the Jenkins GitLab plugin. The previous logic for handling API tokens has been refactored out of GitLabConnection and related classes into this new resolver class, improving code readability and maintainability.

Key changes include:

  • Creation of the GitlabCredentialResolver class to handle credential resolution based on Item context and credential IDs.
  • Refactoring GitLabClientBuilder and its implementations to accept a GitlabCredentialResolver instead of a plain API token string.
  • Modifying ApiHeaderTokenFilter to utilize GitlabCredentialResolver for injecting API tokens into HTTP requests.
  • Updates to unit tests to accommodate the new method of resolving credentials, ensuring compatibility with the new design.
  • Fix for AWS Secret Manager Issue: Addressed an issue where, if AWS Secret Manager is used with a rotation mechanism, the plugin encounters problems because the client is cached and always uses the token with which it was initially created. This change ensures that credentials are retrieved each time they are used, allowing the plugin to work correctly with rotated tokens.

Testing done

  • Unit Tests: Updated existing tests to use GitlabCredentialResolver, ensuring they pass with the refactored code. This includes modifying tests like AutodetectingGitLabClientTest and TestUtility.
  • Manual Testing: Verified the following scenarios manually:
    • Connection to GitLab using both global API tokens and job-specific credentials.
    • Ensured that the new GitlabCredentialResolver properly tracks credentials when associated with specific Jenkins items.
    • Verified that API requests include the correct PRIVATE_TOKEN header for both global and job-specific tokens.
    • Tested the resolution of credentials in environments with AWS Secret Manager to confirm that rotated tokens are correctly retrieved and used.
  • Automated Test Execution: Ran all existing tests to ensure that there are no regressions introduced by this refactoring.

Submitter checklist

  • 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

Aaron Echavarria added 3 commits October 21, 2024 19:49
Avoid caching the credential, instead get the credential value whenev…
96ed882 
…er it is required through the request filter (ApiHeaderTokenFilter)
Fix tests
e555b42
spotless
3ee4c4d
@arechavarria arechavarria requested a review from a team as a code owner October 22, 2024 04:20
@github-actions github-actions bot added the tests This PR adds/removes/updates test cases label Oct 22, 2024
@krisstern krisstern added the bug For changelog: Minor bug. Will be listed after features label Oct 22, 2024
@krisstern
Copy link
Member

krisstern commented Oct 22, 2024

#1711 (comment):

What's not clear? It turns out there is a related issue, when for example aws secret manager is used, there are credentials that are rotated, however the plugin caches the client and the same token is always used, which causes that when the credential rotates the plugin starts to generate errors with Gitlab, because it continues using the same token

@krisstern
Copy link
Member

krisstern commented Oct 22, 2024

Hi @arechavarria Thanks for the PR! But where is the item for the "Link to relevant issues in GitHub or Jira" you have indicated in the checklist that there is one?

@arechavarria arechavarria mentioned this pull request Oct 22, 2024
Closed
@arechavarria
Copy link
Contributor Author

arechavarria commented Oct 22, 2024

Hi @arechavarria Thanks for the PR! But where is the item for the "Link to relevant issues in GitHub or Jira" you have indicated in the checklist that there is one?

Hello, with pleasure

I added the relationship with the issue:
#963

@krisstern krisstern requested a review from daniel-beck October 22, 2024 06:22
@krisstern
Copy link
Member

krisstern commented Oct 22, 2024

Hi @daniel-beck would it be able for you to take a look also and see if this PR would have any security issue(s) if merged as is?

krisstern
krisstern requested changes Oct 23, 2024
View reviewed changes
Copy link
Member

@krisstern krisstern left a comment

Choose a reason for hiding this comment

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

I have a gut feeling we should be more careful about retricting any external use of the two methods concerned?

arechavarria and others added 2 commits October 23, 2024 07:54
@arechavarria @krisstern
Update src/main/java/com/dabsquared/gitlabjenkins/connection/GitlabCr…
3d1a653 
…edentialResolver.java

Co-authored-by: Kris Stern <krisstern@outlook.com>
@arechavarria @krisstern
Update src/main/java/com/dabsquared/gitlabjenkins/connection/GitlabCr…
df4124f 
…edentialResolver.java

Co-authored-by: Kris Stern <krisstern@outlook.com>
arechavarria and others added 2 commits October 23, 2024 10:37
@arechavarria @krisstern
Update src/main/java/com/dabsquared/gitlabjenkins/connection/GitlabCr…
415727b 
…edentialResolver.java

Co-authored-by: Kris Stern <krisstern@outlook.com>
@arechavarria @krisstern
Update src/main/java/com/dabsquared/gitlabjenkins/connection/GitlabCr…
ba99d0b 
…edentialResolver.java

Co-authored-by: Kris Stern <krisstern@outlook.com>
@krisstern
Copy link
Member

krisstern commented Oct 23, 2024

Hi @arechavarria I will let this PR sit here for 24 to 48 hours. If there is no objection from the community within that time frame, I will proceed to merge it.

krisstern
krisstern approved these changes Oct 23, 2024
View reviewed changes
Copy link
Member

@krisstern krisstern left a comment

Choose a reason for hiding this comment

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

LGTM

@arechavarria
Copy link
Contributor Author

arechavarria commented Oct 23, 2024

Thanks @krisstern

@krisstern krisstern merged commit 0ff4703 into jenkinsci:master Oct 24, 2024
@arechavarria arechavarria deleted the feature/avoid_caching_private_token branch October 24, 2024 16:07
@arechavarria
Copy link
Contributor Author

arechavarria commented Oct 24, 2024

@krisstern I ask you at what point the release is managed so that the plugin is available in the marketplace?

@krisstern
Copy link
Member

krisstern commented Oct 24, 2024

Hi @arechavarria we have another PR #1713 pending to be merged soon. So we will need to wait until afterwards when a new release will be cut.

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

Reviewers

@krisstern krisstern krisstern approved these changes

@daniel-beck daniel-beck Awaiting requested review from daniel-beck

Assignees

No one assigned

Labels

bug For changelog: Minor bug. Will be listed after features hacktoberfest-accepted tests This PR adds/removes/updates test cases

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

add ability to flush the cached connection

2 participants

@arechavarria @krisstern