Project

General

Profile

Actions

Idea #20663

closed

Configuration to limit resources managed by arvados-login-sync

Added by Brett Smith over 1 year ago. Updated over 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Deployment
Start date:
06/22/2023
Due date:
Story points:
2.0
Release relationship:
Auto

Description

arvados-login-sync is willing to modify the following resources:

  • User accounts
  • Group additions
  • Group removals
  • User SSH authorized keys
  • User Arvados API credentials

This is very convenient if you have no other system management tools, but many organizations have other ways to manage these resources, and do not want arvados-login-sync interfering with their existing stacks. It should be possible to configure arvados-login-sync to manage a subset of these resources.

Proposed solution:

  • Arvados configuration grows a setting that lists dedicated keys representing what resources arvados-login-sync is allowed to manage. The default is all of them, for backwards compatibility.
  • This configuration is published through the API server.
  • arvados-login-sync reads this configuration from the API server.
  • If the configuration says arvados-login-sync is not allowed to manage a resource, then it does not take any action that might modify that resource. It assumes that another tool is managing that.
  • If arvados-login-sync cannot do a management task that it should do but cannot because of missing system resources, it logs that fact and does not make any changes associated with that user account. For example, if arvados-login-sync is configured to manage SSH keys and API tokens, and these resources exist for a user+VM but that user does not exist on the VM, it logs the fact that it cannot manage a nonexistent user and skips it.

Looking ahead a bit: the reason to make this configuration available through the API is so that client tools can adapt their management interfaces to match. For example, if we do this, there can be a follow-up story so that Workbench 2 does not present management UI for resources that are not managed by arvados-login-sync.


Subtasks 1 (0 open1 closed)

Task #20670: Review 20663-login-sync-configResolvedLucas Di Pentima06/22/2023Actions
Actions #1

Updated by Peter Amstutz over 1 year ago

  • Story points set to 2.0
  • Target version changed from Future to To be scheduled
Actions #2

Updated by Peter Amstutz over 1 year ago

  • Target version changed from To be scheduled to Development 2023-07-05 sprint
Actions #3

Updated by Peter Amstutz over 1 year ago

  • Assigned To set to Brett Smith
Actions #4

Updated by Brett Smith over 1 year ago

  • Status changed from New to In Progress
Actions #5

Updated by Brett Smith over 1 year ago

20663-login-sync-config @ ee35d22df94f1745f97c17f3171e8663fa2e375e - developer-run-tests: #3716

Note I would like to do some manual testing to make sure there are no obvious regressions, etc. But hopefully that wouldn't require any major changes.

Actions #6

Updated by Lucas Di Pentima over 1 year ago

Re-running failed test pipeline: developer-run-tests-remainder: #3905

Actions #7

Updated by Brett Smith over 1 year ago

Brett Smith wrote in #note-5:

Note I would like to do some manual testing to make sure there are no obvious regressions, etc. But hopefully that wouldn't require any major changes.

After too much wrangling with RVM and Gems this is done, it works as expected.

Actions #8

Updated by Lucas Di Pentima over 1 year ago

Some questions:

  • Knowing that this is out of scope, but just in case you think is simple enough to add: How about a config knob Users.SyncIgnoredGroups to allow the site admin team to avoid some potentially problematic groups (like 'root', 'sudo', etc) in case the Arvados admin team is made of different people?
  • Haven't looked at it in detail but I'm seeing that there's some (not much, unfortunately) testing on login-sync, do you think it could be expanded to test this new behaviors?
  • IIRC you asked about API versioning. Not sure if we want it for this branch but just in case, you could change the timestamp at arvados/services/api/app/controllers/arvados/v1/schema_controller.rb:L39.
Actions #9

Updated by Brett Smith over 1 year ago

20663-login-sync-config @ 9b4f22418bc26d57e4b9d4a0ba9ef3c4e34a2e51 - developer-run-tests: #3718

Lucas Di Pentima wrote in #note-8:

  • Knowing that this is out of scope, but just in case you think is simple enough to add: How about a config knob Users.SyncIgnoredGroups to allow the site admin team to avoid some potentially problematic groups (like 'root', 'sudo', etc) in case the Arvados admin team is made of different people?

Sure, added in the most recent commit.

  • Haven't looked at it in detail but I'm seeing that there's some (not much, unfortunately) testing on login-sync, do you think it could be expanded to test this new behaviors?

I don't think it's worth it, unfortunately. arvados-login-sync isn't written to be testable: it's a giant oneshot script that does all of its system operations directly. The good way to fix this would be to modify the script to split up functionality so that it becomes testable. The alternative is to write a huge amount of test scaffolding.

I am guessing at some point semi-soon-ish we will want to rewrite this service in either Go or Python, just because those are our first-class SDKs. I think that would be a good time to make the component testable too.

  • IIRC you asked about API versioning. Not sure if we want it for this branch but just in case, you could change the timestamp at arvados/services/api/app/controllers/arvados/v1/schema_controller.rb:L39.

I don't think it's necessary this time around. The changes in the branch can handle talking to an older API server no problem: it has defaults for all the configuration that match its original behavior.

Actions #10

Updated by Brett Smith over 1 year ago

Brett Smith wrote in #note-9:

Lucas Di Pentima wrote in #note-8:

  • Knowing that this is out of scope, but just in case you think is simple enough to add: How about a config knob Users.SyncIgnoredGroups to allow the site admin team to avoid some potentially problematic groups (like 'root', 'sudo', etc) in case the Arvados admin team is made of different people?

Sure, added in the most recent commit.

Per standup discussion, updated the default value for this setting to tighten security. Now 20663-login-sync-config @ aad254eb85755d41927fe1809cb52c65bb8aac20 - developer-run-tests: #3719

Actions #11

Updated by Lucas Di Pentima over 1 year ago

This LGTM, thanks!

Actions #12

Updated by Brett Smith over 1 year ago

  • Status changed from In Progress to Resolved
Actions #13

Updated by Peter Amstutz over 1 year ago

  • Release set to 66
Actions

Also available in: Atom PDF