MAAS

piston3 token delete cascades

Bug #1870171 reported by Freddy
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
MAAS
Fix Released
Medium
Lee Trager
MAAS 2.8.0b1
2.3
Fix Committed
Medium
Lee Trager
MAAS 2.3.x
2.4
Fix Committed
Medium
Lee Trager
MAAS 2.4.x
2.6
Fix Released
Medium
Lee Trager
2.7
Fix Released
Medium
Lee Trager
MAAS 2.7.1b1

Bug Description

https://git.launchpad.net/maas/tree/src/maasserver/models/node.py#n1101

The current configured behavior is to CASCADE deletes on the token_id column for maasserver_node. This results in nodes potentially being deleted on the event of a token being deleted.

There are several thousand maas nodes in the installation that we manage. We have started to enforce key rotation and it has resulted in nodes being removed from our maas installation, which negatively impacts our users.

Can the token foreign key on_delete attribute be updated to either
- SET_NULL, to keep the node row but set the token_id attribute to null. or,
- PROTECT, to raise an error if there are any node objects related to the token_id

We do not use the juju functionality that requires linking a token to a node, so I can't say which would be better. However, I can't believe that silently deleting nodes out of maas is the best solution.

More info on ForeignKey setup: https://docs.djangoproject.com/en/2.1/ref/models/fields/#django.db.models.ForeignKey.on_delete

Related branches

~ltrager/maas:lp1870171_2_2.3
Lee Trager (community): Approve
Diff: 46 lines (+11/-3)
2 files modified
src/maasserver/models/tests/test_userprofile.py (+7/-2)
src/maasserver/models/userprofile.py (+4/-1)
~ltrager/maas:lp1870171_2_2.4
Lee Trager (community): Approve
Diff: 46 lines (+11/-3)
2 files modified
src/maasserver/models/tests/test_userprofile.py (+7/-2)
src/maasserver/models/userprofile.py (+4/-1)
~ltrager/maas:lp1870171_2_2.6
Lee Trager (community): Approve
Diff: 46 lines (+11/-3)
2 files modified
src/maasserver/models/tests/test_userprofile.py (+7/-2)
src/maasserver/models/userprofile.py (+4/-1)
~ltrager/maas:lp1870171_2
Björn Tillenius: Approve
MAAS Lander: Approve
Diff: 46 lines (+11/-3)
2 files modified
src/maasserver/models/tests/test_userprofile.py (+7/-2)
src/maasserver/models/userprofile.py (+4/-1)
~ltrager/maas:lp1870171_2.3
Lee Trager (community): Approve
Diff: 564 lines (+153/-120)
10 files modified
src/maasserver/api/account.py (+1/-1)
src/maasserver/api/machines.py (+21/-9)
src/maasserver/api/tests/test_api.py (+15/-2)
src/maasserver/api/tests/test_machine.py (+40/-16)
src/maasserver/api/tests/test_machines.py (+13/-48)
src/maasserver/models/node.py (+10/-7)
src/maasserver/models/tests/test_node.py (+29/-32)
src/maasserver/models/tests/test_userprofile.py (+9/-1)
src/maasserver/models/userprofile.py (+7/-2)
src/maasserver/node_action.py (+8/-2)
~ltrager/maas:lp1870171_2.4
Lee Trager (community): Approve
Diff: 557 lines (+147/-120)
10 files modified
src/maasserver/api/account.py (+1/-1)
src/maasserver/api/machines.py (+21/-9)
src/maasserver/api/tests/test_api.py (+15/-2)
src/maasserver/api/tests/test_machine.py (+40/-16)
src/maasserver/api/tests/test_machines.py (+13/-48)
src/maasserver/models/node.py (+10/-7)
src/maasserver/models/tests/test_node.py (+29/-32)
src/maasserver/models/tests/test_userprofile.py (+9/-1)
src/maasserver/models/userprofile.py (+7/-2)
src/maasserver/node_action.py (+2/-2)
~ltrager/maas:lp1870171_2.6
Lee Trager (community): Approve
Diff: 820 lines (+361/-128)
13 files modified
src/maasserver/api/account.py (+1/-1)
src/maasserver/api/machines.py (+12/-9)
src/maasserver/api/tests/test_api.py (+15/-2)
src/maasserver/api/tests/test_machine.py (+40/-16)
src/maasserver/api/tests/test_machines.py (+23/-54)
src/maasserver/models/node.py (+11/-7)
src/maasserver/models/tests/test_node.py (+29/-32)
src/maasserver/models/tests/test_userprofile.py (+9/-1)
src/maasserver/models/userprofile.py (+7/-2)
src/maasserver/node_action.py (+2/-2)
src/maasserver/tests/test_node_action.py (+1/-2)
src/maasserver/websockets/handlers/tests/test_token.py (+115/-0)
src/maasserver/websockets/handlers/token.py (+96/-0)
~ltrager/maas:lp1870171_2.7
Lee Trager (community): Approve
Diff: 556 lines (+71/-99)
13 files modified
src/maasserver/api/account.py (+1/-1)
src/maasserver/api/machines.py (+5/-6)
src/maasserver/api/tests/test_api.py (+12/-2)
src/maasserver/api/tests/test_machine.py (+0/-4)
src/maasserver/api/tests/test_machines.py (+8/-52)
src/maasserver/models/node.py (+1/-5)
src/maasserver/models/tests/test_node.py (+4/-19)
src/maasserver/models/tests/test_userprofile.py (+9/-1)
src/maasserver/models/userprofile.py (+5/-1)
src/maasserver/node_action.py (+2/-2)
src/maasserver/tests/test_node_action.py (+1/-3)
src/maasserver/websockets/handlers/tests/test_token.py (+11/-2)
src/maasserver/websockets/handlers/token.py (+12/-1)
~ltrager/maas:lp1870171
Alberto Donato (community): Approve
MAAS Lander: Approve
Diff: 435 lines (+33/-101)
8 files modified
src/maasserver/api/machines.py (+5/-6)
src/maasserver/api/tests/test_machine.py (+0/-4)
src/maasserver/api/tests/test_machines.py (+8/-52)
src/maasserver/migrations/maasserver/0206_remove_node_token.py (+12/-0)
src/maasserver/models/node.py (+1/-15)
src/maasserver/models/tests/test_node.py (+4/-19)
src/maasserver/node_action.py (+2/-2)
src/maasserver/tests/test_node_action.py (+1/-3)
Alberto Donato (ack)
Changed in maas:
milestone: none → 2.8.0b1
status: New → Triaged
importance: Undecided → High
importance: High → Medium
Revision history for this message
Lee Trager (ltrager) wrote :

MAAS doesn't expect others to be modifying the database. The OAUTH keys stored are passed to the deployed machine and used to communicate back to MAAS. By deleting them the machine won't be able to authenticate and communicate back with MAAS. Removing the current keys could break any ephemeral environment or deployment.

MAAS does remove keys when they are no longer needed however the key needs to remain the same as long as the machine is deployed. This is because cloud-init accesses the metadata service on every boot. We *may* be able to change that but we'd have to do some investigation.

Revision history for this message
Freddy (fwieffering) wrote :

To be clear - I am not modifying the database. In the mass ui, when you remove an API key from a user any nodes associated with that key are removed from maas, because the mass api is configured to cascade the deleted token.

I understand that the token has a purpose. I just don't want to inadvertently result in a number of nodes being silently removed from maas. There could be either a warning that there are linked nodes or the action could be forbidden if there are linked nodes.

Revision history for this message
Björn Tillenius (bjornt) wrote :

Yes, this is something that we need to fix.

I think Lee and Freddy are talking about different tokens. The token that Freddy is talking about is Node.token, which get set whenever a machine is acquired using the API.

It seems that the only use for that key is to for the list-allocated API call, which lists all the machines that were deployed using the current API key.

Revision history for this message
Björn Tillenius (bjornt) wrote :

I confirmed this bug. I deployed a machine using the API, and then I went to the UI and deleted the user's API key that was used to deploy the machine. The result was the the deployed machine was deleted as well.

Adam Collard (adam-collard)
Changed in maas:
assignee: nobody → Lee Trager (ltrager)
Revision history for this message
Lee Trager (ltrager) wrote :

My mistake, it seems Nodes have two tokens associated with them. One is used as I described above. The second seems to be a left over from something else. When a machine is allocated, only on the API, it sets an association between the user's token and the Node. Only the API uses this to figure out which nodes have been allocated. The nodes are already filtered by owner and state so it isn't needed. I'll create a branch to remove it.

MAAS Lander (maas-lander)
Changed in maas:
status: Triaged → Fix Committed
Revision history for this message
Björn Tillenius (bjornt) wrote :

I reopened all the backport tasks, since they still need some work. It would be good to try to hook into a lower layer.

One thing that I tested was to deploy a machine with user A using the API in 2.7.0. Then I upgraded to 2.7.1 and deleted user A using the API, passing in transfer_resources_to=B.

The result was that user A and the machine that was deployed were both deleted.

Revision history for this message
Lee Trager (ltrager) wrote :

Thanks for catching that. It looks like the transfer_resources_to code never changed the OAUTH token from the previous user. I confirmed this code can only be used when deleting a user so there is no risk of the previous user maintaining access to a transferred node.

Alberto Donato (ack)
Changed in maas:
status: Fix Committed → Fix Released
Revision history for this message
Lee Trager (ltrager) wrote :

The fix has landed in MAAS <= 2.7.

Revision history for this message
Freddy (fwieffering) wrote :

will you be releasing a patched version of 2.6 + 2.7 with this change?

Revision history for this message
Lee Trager (ltrager) wrote :

Yes, I have back ported this to 2.7, 2.6, 2.4, and 2.3.

Revision history for this message
Freddy (fwieffering) wrote :

I saw that but haven't seen anything pushed to the PPAs for 2.6 / 2.7 and wanted to confirm that will come.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.