feat(api-tokens): API token expiry and revoke-by-key endpoint#14932
feat(api-tokens): API token expiry and revoke-by-key endpoint#14932svader0 wants to merge 9 commits into
Conversation
- Add token_expiry DateTimeField to UserContactInfo (was missing from initial commit despite migration referencing it) - Register ApiTokenViewSet at api-tokens/ in urls.py (was missing from initial commit despite ViewSet existing in views.py) - Add unit tests for list, retrieve, revoke, expiry enforcement, and default-expiry-on-reset behaviours
|
This pull request triggers multiple critical findings from the configured codepaths analyzer, indicating sensitive edits across core files such as
🔴 Configured Codepaths Edit in
|
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/api_v2/views.py (drs_e0bcfe7f)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/db_migrations/0269_usercontactinfo_token_expiry.py (drs_850baef5)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/models.py (drs_0aa4b6d6)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/templates/dojo/api_v2_key.html (drs_0d836839)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/urls.py (drs_d1116fbb)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/user/authentication.py (drs_bbd8f2de)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/user/views.py (drs_f521fc46)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/models.py (drs_b492fa69)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/urls.py (drs_76d499ca)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/user/authentication.py (drs_a6fb3017)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/user/authentication.py (drs_637a190b)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/api_v2/views.py (drs_f690e06f)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/user/authentication.py (drs_b4eef600)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/models.py (drs_0b272573)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/urls.py (drs_8620e6e2)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/api_v2/serializers.py (drs_0ed544d0)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/api_v2/views.py (drs_32b9ac12)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/urls.py (drs_2a60837c)
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🟡 Token auto-creation bypasses expiry policy in dojo/user/views.py (drs_34ed9c3e)
| Vulnerability | Token auto-creation bypasses expiry policy |
|---|---|
| Description | In dojo/user/views.py, the api_v2_key view automatically creates an API token for users who do not have one. This creation process does not initialize the token_expiry field in the user's UserContactInfo. As a result, newly created tokens via this path have token_expiry set to None (or effectively uninitialized). The ExpiringTokenAuthentication middleware in dojo/user/authentication.py only enforces expiration if uci.token_expiry is set and in the past; if it is None, the token remains valid indefinitely, allowing users to effectively bypass any configured expiration policy. |
django-DefectDojo/dojo/user/views.py
Lines 104 to 105 in ebbbc31
We've notified @mtesauro.
Comment to provide feedback on these findings.
Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]
Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing
All finding details can be found in the DryRun Security Dashboard.
|
Thank you for the PR. At first glance there seem to be various functionalities in this PR. I can imagine having an expiry datetime/period for tokens is good to have and not to hard to maintain. But I'm unsure of the other items like allowing super users to manage other users tokens etc. Do you have good example of use cases for each feature introduced by the PR? |
|
Thanks for taking a look! I definitely agree about the expiry date/time period. After thinking about it for a while, it seems that the practical use-case of the token-management stuff was a little too overstated, so I made a few changes to slim it down. Token expiry: Like you said, it's fairly easy implementation and maintenance, and having an expiration date on API keys is pretty standard behavior. If a key were to get leaked, having an expiration date will reduce risk. I think this should stay as-is. Token Management: The thinking was that if a token was compromised, as a superuser you may want to revoke it immediately without issuing a replacement. I was originally following the train of logic set about by the issue I was responding to, where you'd want a superuser to be able to have a list of existing tokens and then be able to revoke them selectively. But I think that in trying to solve that problem, I accidentally made things a little convoluted here, so I went ahead and simplified the solution. Now there is one new endpoint in this PR, If you think these two additions are too distinct to be in one PR, I am happy to open up a second and split them up. |
|
So this PR is hard to review not because of the implementation, but because it needs some careful thinking about API design, features, security, etc. My feeling is that it cannot be merged as-is. I have asked Claude also to review it and that review below contains some of my concerns. My suggestion would be to switch to django-restknow (or another library if it exists) and do it right. Might be a good time right now while we're "in transition" to v3. I'm going to have to ask @Maffooch and @mtesauro to lean in here as well, and will discuss it with them. |
|
Thanks for working on this — token lifecycle management is a real gap. I reviewed the full diff; some of this is blocking-ish (correctness gaps), and there's a bigger-picture question at the end about foundation. CorrectnessWhat's solid:
Gaps:
Missing features
Foundation: should this be built on django-rest-knox?This is the bigger question. django-rest-knox provides natively almost everything being hand-rolled here: per-token expiry (on the token itself, no The catch is that adopting it is a breaking, multi-release migration, not a tweak to this PR: DefectDojo's whole ecosystem uses DRF's So my suggestion is not to retrofit Knox here. Either:
The decoupled-expiry and bypass-path issues above are essentially symptoms of forcing per-token semantics onto DRF's single-plaintext-token model — which is exactly what Knox exists to replace. Happy to help scope the Knox migration if the team wants to go that route. |
|
Makes a lot of sense. You're right in that this is more of a shotgun-surgery type of fix; I wasn't quite sure how drastic we wanted to go. Happy to help with implementing a more robust solution depending on what the higher-ups decide. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
@valentijnscholten thanks for the careful consideration here. I think you're right that this would be a better API v3 ONLY change. There have been no expirations on API v2 for many years, so this would be a big shift. I would feel better if this new mechanism only impacted v3 so that the correct expectation can be set from the get go |
This PR addresses issue/feature request #14890.
NOTE: This PR description has been updated away from the original commit, which was significantly different.
Description
Currently, DefectDojo has no mechanism for managing API tokens via the API. In case a token gets leaked or needs to be revoked, the only existing solution is to call
reset_api_token, which simply rotates the token. Superusers also have no way to see what tokens exist, who owns them, or when they were issued. This PR is an attempt to close the gap a little.This PR introduces a per-user token expiry. A superuser can set a
token_expiryon a specific user via the existinguser_contact_infosendpoint. Expired tokens are rejected at authentication time. In addition to this, a new instance-wide environment variableDD_API_TOKEN_DEFAULT_EXPIRY_DAYS(talk about a mouthful...) applies a default lifetime to all generated tokens. The default is 0, a.k.a. no expiry, so there should be no issues with existing deployments unless they choose to opt into the change.This PR also introduces one new endpoint
POST /api/v2/api-tokens/revoke/, where you supply a key via JSON payload, and it gets revoked. Currently, you can only rotate a user's key by user lookup, you can't eradicate a specific key itself. This would solve the use case where, "uh oh!", a token gets leaked, and I want to eradicate it immediately and programmatically without tracking down its owner and rotating it.Documentation
docs/content/automation/api/api-v2-docs.mdupdated with new endpoint, expiry behavior, and theDD_API_TOKEN_DEFAULT_EXPIRY_DAYSvariable.Checklist
This checklist is for your information.
dev.dev.bugfixbranch.