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.

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

feat: Extend authentication to support Bearer token for many resources #7277

Merged
merged 9 commits into from
Jan 1, 2025

Conversation

aikebah
Copy link
Collaborator

@aikebah aikebah commented Dec 31, 2024

Description of Change

Builds on top of the changes of #7255 that added pre-emptive basic authentication, so should be merged after that.

Adds HTTP Bearer authentication configuration possibility for

  • NVD API Datafeed
  • Known Exploited Vulnerabilities mirror site
  • User suppression files
  • Hosted Suppressions File mirror site
  • RetireJS Repo mirror site
  • Central Content URL mirror site
  • Central Search URL mirror site

Adds HTTP Basic authentication configuration possibility for:

  • Known Exploited Vulnerabilities (CLI/Maven)
  • Hosted Suppressions File mirror site (Ant/CLI/Maven)
  • User suppression files (Ant/CLI)
  • Central Search URL mirror site (CLI)

Related issues

Have test cases been added to cover the new functionality?

no - as testcases would require live endpoints that host the various files behind a HTTP Basic- or HTTP Bearer authentication.

@boring-cyborg boring-cyborg bot added ant changes to ant cli changes to the cli core changes to core documentation site documentation maven changes to the maven plugin utils changes to utils labels Dec 31, 2024
@aikebah
Copy link
Collaborator Author

aikebah commented Dec 31, 2024

Awaiting merge of #7255 before looking into the reported merge conflicts

@aikebah
Copy link
Collaborator Author

aikebah commented Dec 31, 2024

@jeremylong let me know if you'd rather have this merged into the branch of #7255 to then merge both in one go into main

@jeremylong
Copy link
Owner

I've approved #7255 - feel free to merge it. See #7255 (comment)

…URLs

- Add configurability for hostedSuppressionsFile user and password
- Add configurability for knownExploitedVulnerabilities user and password

Also sanitized the ant tasks:
- hostedSuppressionUrl is irrelevant for Purge, so pushed down to Update
- retireJs URL/User/Password/ForceUpdate/Enabled and KnownExploited URL/Enabled are also relevant for Update, so pulled up from Check
- retireJsForceUpdate brought in line with documentation (original code wrongly used retireJsAnalyzerForceUpdate deviating from the documentation)
- Over time methods in Check slipped in-between a "copied from PathConvert comment region", moved those out and transformed the comments into an IntelliJ-and-VS-Code compatible folding region
- Removed the obsolete getters for the Task configuration parameters
- nvdApiDelay changed into an Integer so that we only set a non-default value when specified in the task
- Moved value sanity checks from populateSettings to the setter of the given property
- Added configurability of Known Exploited valid for hours
* add NVD_API_DATAFEED_BEARER_TOKEN
* add KEV_BEARER_TOKEN
* add SUPPRESSION_FILE_BEARER_TOKEN
* add HOSTED_SUPPRESSIONS_BEARER_TOKEN
* add ANALYZER_RETIREJS_REPO_JS_BEARER_TOKEN
* add CENTRAL_CONTENT_BEARER_TOKEN
* add ANALYZER_CENTRAL_BEARER_TOKEN
* update Javadoc to document the either bearer- or basic-auth aspect of the authentication property keys
…cached/mirrored resources

* Add the missing suppressionFile User/Password basic auth
* Add suppressionFile Bearer token auth
* Add retireJS Bearer token auth
* Add KEV Bearer token auth
* Add NVD Datafeed Bearer token auth
* Add HostedSuppressions Bearer token auth

* Synchronize documentation with implementation
…cached/mirrored resources

* add suppressionFile auth, both basic and bearer
* add RetireJS bearer auth and fixup documentation of the basic auth flags
* add KEV auth, both basic and bearer
* add NVD datafeed bearer auth
* add hostedSuppressions auth, both basic and bearer
* add central search auth, both basic (was missing in CliParser) and bearer
…r cached/mirrored resources

* add NVD datafeed bearer auth + extend serverId usage to bearer when only password
* add KEV auth, both basic and bearer
* add suppressionFile bearer auth + extend serverId usage to bearer when only password
* add hosted suppressions auth, both basic and bearer
* add retireJsUrl bearer auth + extend serverId usage to bearer when only password
* don't handover member fields as instance method parameters
* re-order parameters in preparation of adding bearer authentication key
@aikebah aikebah force-pushed the feat/hc5-extending-auth branch from cc04f55 to 9c5dac9 Compare December 31, 2024 12:10
@aikebah aikebah marked this pull request as ready for review December 31, 2024 12:11
jeremylong
jeremylong previously approved these changes Dec 31, 2024
Copy link
Owner

@jeremylong jeremylong left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for moving some of the Ant configuration from Check to Update where it should have been.

@jeremylong
Copy link
Owner

Looks like an NPE in the test?

@aikebah
Copy link
Collaborator Author

aikebah commented Dec 31, 2024

Not sure how that suddenly surfaced, was convinced I've run a full build even with the latest fixes, but apparently not. The fixup of the last commit is what currently makes it break.

Looking into its root cause (testcase fails to .configure(Settings settings) the Downloader before triggering code that uses parts of the Downloader that access the settings).

I think the Downloader singleton initialisation within the various bootstraps would best be changed to a Downloader cached by the Engine and initialized during Engine construction. In practice it would be the same as a singleton (as there would typically only a single engine instance), but it would allow for correct behaviour in case of multiple engine instances with different settings running in the JVM and would require no special care in the testcases.

@aikebah
Copy link
Collaborator Author

aikebah commented Dec 31, 2024

diff --git a/core/src/test/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzerTest.java b/core/src/test/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzerTest.java
index 36009f597..33c656ba9 100644
--- a/core/src/test/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzerTest.java
+++ b/core/src/test/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzerTest.java
@@ -32,6 +32,7 @@ import org.owasp.dependencycheck.Engine.Mode;
 import org.owasp.dependencycheck.analyzer.exception.AnalysisException;
 import org.owasp.dependencycheck.dependency.Dependency;
 import org.owasp.dependencycheck.exception.InitializationException;
+import org.owasp.dependencycheck.utils.Downloader;
 import org.owasp.dependencycheck.utils.Settings;
 import org.owasp.dependencycheck.utils.Settings.KEYS;
 import org.owasp.dependencycheck.xml.suppression.SuppressionRule;
@@ -152,6 +153,7 @@ public class AbstractSuppressionAnalyzerTest extends BaseTest {
         getSettings().setString(KEYS.SUPPRESSION_FILE, path);
         final AbstractSuppressionAnalyzerImpl fileAnalyzer = new AbstractSuppressionAnalyzerImpl();
         fileAnalyzer.initialize(getSettings());
+        Downloader.getInstance().configure(getSettings());
         Engine engine = new Engine(Mode.EVIDENCE_COLLECTION, getSettings());
         fileAnalyzer.prepare(engine);
         int count = AbstractSuppressionAnalyzer.getRuleCount(engine);

Would solve the symptom, but I think it's better to solve it by going from singleton to managed-by-engine downloader instance. @jeremylong WDYT?

@aikebah
Copy link
Collaborator Author

aikebah commented Dec 31, 2024

I experimented a bit to take the root-cause way and make the downloader managed by the engine, but encountered so many places where the downloader (or the engine) would need to passed downwards on calls (and/or cached in local fields) that I think fixing up the testcase to initialize the singleton is the better approach.

Copy link
Owner

@jeremylong jeremylong left a comment

Choose a reason for hiding this comment

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

LGTM

@jeremylong jeremylong merged commit 70b40e4 into main Jan 1, 2025
9 checks passed
@jeremylong jeremylong deleted the feat/hc5-extending-auth branch January 1, 2025 13:08
@jeremylong jeremylong added this to the 12.0.0 milestone Jan 1, 2025
@aikebah
Copy link
Collaborator Author

aikebah commented Jan 1, 2025

@jeremylong Have you already started integrating this (new configuration parameters) into the gradle plugin? Or shall I make a start with updating that?

@jeremylong
Copy link
Owner

jeremylong commented Jan 1, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ant changes to ant cli changes to the cli core changes to core documentation site documentation maven changes to the maven plugin tests test cases utils changes to utils
Projects
None yet
2 participants