-
-
Notifications
You must be signed in to change notification settings - Fork 582
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
Enhance badge API to require authorization #4059
Enhance badge API to require authorization #4059
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
Forgot readying the PR for the frontend, will remain a draft until then |
6bf1e60
to
2790e1c
Compare
Remove frontend elements for this switch, as badges API get authenticated access. Downstream change of DependencyTrack/dependency-track#4059 Signed-off-by: Kirill.Sybin <[email protected]>
@SaberStrat |
Replace enabling of unauthenticaed access to badges via admin config checkbox with an api authentication with a new dedicated permission "VIEW_BADGES". Signed-off-by: Kirill.Sybin <[email protected]>
Signed-off-by: Kirill.Sybin <[email protected]>
Signed-off-by: Kirill.Sybin <[email protected]>
Add new badge permission to tests. Remove tests for badge disabling. Add tests testing authentication, permission and ACL access. Signed-off-by: Kirill.Sybin <[email protected]>
Signed-off-by: Kirill.Sybin <[email protected]>
Allows API authentication via URI query param for badge requests as an alternative to header authentication because typical use cases for badges do not easily allow header injection. Requires stevespringett/Alpine#641 Signed-off-by: Kirill.Sybin <[email protected]>
Update tests to focus on API authentication via URI query parameter, but keep some tests that test header authentication as that remains an option. Requires stevespringett/Alpine#641 Signed-off-by: Kirill.Sybin <[email protected]>
2790e1c
to
8c40c9a
Compare
Not sure how to repro the error locally. The unit tests are successful. |
Just a flaky test. I restarted CI workflow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, and I agree with your assessment of intentionally making this a breaking change due to the security implications of the current implementation.
In the original issue, Steve suggested to create a default team with the VIEW_BADGE
permission. I think we should follow that. You can add a new default team here:
dependency-track/src/main/java/org/dependencytrack/persistence/DefaultObjectGenerator.java
Lines 138 to 173 in edb95ce
private void loadDefaultPersonas() { | |
try (QueryManager qm = new QueryManager()) { | |
if (!qm.getManagedUsers().isEmpty() && !qm.getTeams().isEmpty()) { | |
return; | |
} | |
LOGGER.info("Adding default users and teams to datastore"); | |
LOGGER.debug("Creating user: admin"); | |
ManagedUser admin = qm.createManagedUser("admin", "Administrator", "admin@localhost", | |
new String(PasswordService.createHash("admin".toCharArray())), true, true, false); | |
LOGGER.debug("Creating team: Administrators"); | |
final Team sysadmins = qm.createTeam("Administrators", false); | |
LOGGER.debug("Creating team: Portfolio Managers"); | |
final Team managers = qm.createTeam("Portfolio Managers", false); | |
LOGGER.debug("Creating team: Automation"); | |
final Team automation = qm.createTeam("Automation", true); | |
final List<Permission> fullList = qm.getPermissions(); | |
LOGGER.debug("Assigning default permissions to teams"); | |
sysadmins.setPermissions(fullList); | |
managers.setPermissions(getPortfolioManagersPermissions(fullList)); | |
automation.setPermissions(getAutomationPermissions(fullList)); | |
qm.persist(sysadmins); | |
qm.persist(managers); | |
qm.persist(automation); | |
LOGGER.debug("Adding admin user to System Administrators"); | |
qm.addUserToTeam(admin, sysadmins); | |
admin = qm.getObjectById(ManagedUser.class, admin.getId()); | |
admin.setPermissions(qm.getPermissions()); | |
qm.persist(admin); | |
} | |
} |
As this is a breaking change: |
@SaberStrat Thoughts on @savek-cc's remarks? Would you be willing to refactor your work to enable backward-compatibility? |
Maybe a different approach: Could we (in order to prepare for the preparation) create an AuthKey in DT 4.11 and include that in todays API requests (they would be ignored for now) and then give that team/API-key the new permission with DT 4.12? |
I don't think it should be an either or (authenticated OR unauthenticated) - but rather an override to allow access without an API key if none is provided. If one is provided, it should be checked as currently implemented. So the change is as already implemented - but there is an override (e.g. as env-var) that makes it not only accept valid authKeys - but also the empty one (as in none supplied). What are your thoughts? |
You mean as a configuration or property for the application like these here https://docs.dependencytrack.org/getting-started/configuration/? So basically turning the current switch from an either-or into an enabler for unauthentic access and moving it from the UI into a property? Unless I misunderstood, it would be a neat middle ground for everyone involved. Gives users a grace period, I don’t have to do as much extra coding (I think) and the setting is hidden a bit more, thus advertising the authentication-way more/authentication-less way less. |
Correct - but even if that override is enabled, your code should still check the AuthKey - just on the "no auth key present" path there is an override to grant access anyway IF that config var is set. Everything in the UI stays as currently implemented in your PR. Code changes should(?) be minimal and not require you to rework a lot. This way, projects can already start using (and testing - as an invalid key would still be rejected) the new authentication. @nscuro your thoughts? |
Wouldn't it be easier to just leave the existing Off: Disable unauthenticated, allow authenticated+authorized Users who already have it enabled won't have to do anything when upgrading to v4.12. For v4.13, we remove this toggle and only allow authenticated access. I think moving the toggle from UI to env vars in v4.12 will just be confusing. |
Looking at the current documentation https://docs.dependencytrack.org/integrations/badges/ I too think that this is the best idea. The current option is specifically about enabling unauthenticated access (pre 4.12)- and there is no need to disable badges if the only access is authenticated (4.13+). |
Then I’m modifying this PR and issue and its frontend counterpart and create a separate pair for the removal for 4.13? |
@SaberStrat Yeah I think that would be the easiest. |
Add a default team for viewing badges for new DBs. Signed-off-by: Kirill.Sybin <[email protected]>
@SaberStrat Do you have a rough idea as to how soon you'd be able to complete this work? No pressure, just curious WRT planning the v4.12 release. |
@nscuro Estimate by feeling: good case this week, bad case by end of next week. What’s the deadline for v4.12? |
We were hoping for early next week, since the next community meeting is on Wednesday. |
I'm actually struggling to come up with a way to toggle authentication on and off for a resource at runtime. DT is using alpine-server to apply an authentication filter for incoming requests, which has the method annotation Previously, the 'Badges-way' was implemented like this: deactivate authentication in general for all methods of the resource with that annotation (static), and the ConfigProperty/general setting toggle made the resource methods return an empty body (dynamic) Now what we want is for the toggle to basically do what the annotation did, but let the user control it, at runtime, i.e. dynamically. I'd either need to inject and query state of a ConfigProperty either in alpine-server or somewhere in DT where Alpine classes plug into DT. Or maybe work with Reflections and modify the annotation at runtime? Either way, my thoughts lead down complicated paths with lots of effort, that I'm not sure is justifiable for a temporary deprecation. I'd be thankful for suggestions of justifiable implementations. Otherwise, I'm at the point where @savek-cc's idea of adding a |
I think you could simply implement the permission check "manually" in the method body, instead of relying on the annotation. There is a You can access the Yes it's a bit of duplicated code, but ultimately it's not a lot, and it's only in this specific endpoint, and only for v4.12. So I believe this is a reasonable option. |
To make the removal of unauthenticated access to badges not be a breaking change after all, the enable badges config property is kept in after all, but repurposed into a setting to enable unauthenticated access to the badges resource. If it is disabled, then the badges api remains accessible to authenticated and authorized requests. Signed-off-by: Kirill.Sybin <[email protected]>
Signed-off-by: Kirill.Sybin <[email protected]>
Update documentation for globally configurable unauthenticated access to badges. Signed-off-by: Kirill.Sybin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @SaberStrat! Could you please look into fixing the failing tests? Happy to merge once those are resolved.
Error: Failures:
Error: DefaultObjectGeneratorTest.testContextInitialized:38->testLoadDefaultPersonas:96 expected:<3> but was:<4>
Error: DefaultObjectGeneratorTest.testLoadDefaultPersonas:96 expected:<3> but was:<4>
Thank you for the help! It makes sense and goes in a similar direction that ACL is implemented in the badge resource. |
Signed-off-by: Kirill.Sybin <[email protected]>
Can't repro the test failure locally. |
Likely flaky. Triggered a re-run of the workflow. |
Seems like the CPAN registry is slow to respond, causing the test failure. Will merge, since the failing test is unrelated to this change. |
Description
Enables Dependency-Track to offer badges in a secure manner and change the
badge
API from an opt-in-able, unauthenticated one into one requiring authentication with the new permissionVIEW_BADGES
, either via HTTP header or URI query parameter.In the currently implementation, this as a breaking change (see Additional Details.)
Addressed Issue
Closes #3596
Additional Details
As a first for DT's API, badges will allow authentication via URI query parameter, as badges are often retrieved from/embedded in clients that do not easily support an injection of headers.
The securing of badges behind API authentication and ACL can be done in two ways, either as a breaking change or gracefully with backwards compatibility: implement authentication with a new permission and
A grace period is nice. But because the previous way of offering badges represents a security flaw, I would've preferred to make users switch to authenticated badges requests right away.
Downstream changes
DependencyTrack/frontend#967
Upstream requirements
stevespringett/Alpine#641
Checklist
This PR fixes a defect, and I have provided tests to verify that the fix is effectiveThis PR introduces changes to the database model, and I have added corresponding update logic