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

[DDO-3868] Faster break-glass propagation #655

Merged
merged 11 commits into from
Sep 9, 2024

Conversation

jack-r-warren
Copy link
Member

@jack-r-warren jack-r-warren commented Sep 6, 2024

Three-pronged approach:

1. Composite/plural grants are now supported. More plainly, this means that the following now does what you'd expect:

{
  "name": "my-fun-fancy-role",
  "grantsProdFirecloudGroup": "first-group-to-grant@firecloud.org, second-group-to-grant@firecloud.org"
}

This plural grant support is implemented for all existing propagated-grant fields just as comma separation (with whitespace trimming) so it's backwards compatible. This can potentially help propagation in that I think group nesting may take longer than direct group membership.

If we add Firecloud account creation or GitHub org membership in the future, plural grants might not make much sense (suppose the grant might just be indicated by a boolean, not a string). That's okay, we'd just configure the propagator like this:

// Hypothetical! This engine and grant type don't exist yet
&propagatorImpl[bool, ..., ...]{
  configKey: "devFirecloudAccount",
  getGrants: func(role models.Role) []*bool { return []*bool{ role.GrantsDevFirecloudAccount } },
  engine:    &propagation_engines.ImaginaryHypotheticalFirecloudAccountEngine{},
}

2. Can now grant dev/qa/prod Firecloud accounts roles/owner on Google Cloud folders (new propagation engine + new role grant fields). More plainly, the following is now supported:

{
  "name": "my-fun-fancy-role",
  "grantsProdFirecloudFolderOwner": "folders/abc123, folders/def456"
}

The folders can be in any organization -- the reference to Firecloud just describes what accounts will be given the access. Sherlock's SA will need resourcemanager.folders.get/setIamPolicy on the folders.

When Sherlock targets a folder it assumes no other system will give user @firecloud.org accounts roles/owner. Service accounts, groups, users from other domains, or user @firecloud.org accounts with roles other than roles/owner will be unaffected. Identiteam has confirmed that no Terra system ever grants anyone roles/owner so we've got multiple layers between us and a conflict.

3. Propagators now run in parallel for a given role. The sequencing and parallelization of different propagators is easily changed from the role propagation package's init function.

Right now, full parallelization is fine, but this design explicitly allows for sequential steps still so we can have any future Firecloud account creation or GitHub org membership propagator run first.

Testing

Tests written for all of this. For parts 1 and 3 especially the existing tests provide excellent regression coverage.

Part 2 can be enabled in dev before prod, and even in prod we can target it at dev or qa Firecloud before prod.

Risk

Moderate. 1 and 3 are under the hood changes that regression testing fully captures.

2 is risky as Sherlock will begin modifying IAM directly instead of indirectly. The overall design of the system isn't changing -- whereas Sherlock currently modifies IAM by adding people to a group that has an IAM grant, Sherlock will begin modifying IAM by adding people to it directly.

The code isn't zero risk but the vast majority of the logic is already inherited by the propagation system. I'd judge that the larger risk comes from the potential for super-admins setting incorrect configuration. Putting a folder grant on the wrong role would be bad... but it's entirely possible for someone to typo the group in a Google IAM panel too. Somewhere in the stack we have to have the danger button.

@jack-r-warren jack-r-warren requested a review from a team as a code owner September 6, 2024 20:57
Copy link

github-actions bot commented Sep 6, 2024

What's Changed


GET /api/roles/v3
Parameters:

Added: grantsDevFirecloudFolderOwner in query

Added: grantsProdFirecloudFolderOwner in query

Added: grantsQaFirecloudFolderOwner in query

Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    Changed items (object):

    • Added property grantsDevFirecloudFolderOwner (string)

    • Added property grantsProdFirecloudFolderOwner (string)

    • Added property grantsQaFirecloudFolderOwner (string)

POST /api/roles/v3
Request:

Changed content type : application/json

  • Added property grantsDevFirecloudFolderOwner (string)

  • Added property grantsProdFirecloudFolderOwner (string)

  • Added property grantsQaFirecloudFolderOwner (string)

Return Type:

Changed response : 201 Created

Created

  • Changed content type : application/json

    • Added property grantsDevFirecloudFolderOwner (string)

    • Added property grantsProdFirecloudFolderOwner (string)

    • Added property grantsQaFirecloudFolderOwner (string)

GET /api/roles/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Added property grantsDevFirecloudFolderOwner (string)

    • Added property grantsProdFirecloudFolderOwner (string)

    • Added property grantsQaFirecloudFolderOwner (string)

DELETE /api/roles/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Added property grantsDevFirecloudFolderOwner (string)

    • Added property grantsProdFirecloudFolderOwner (string)

    • Added property grantsQaFirecloudFolderOwner (string)

PATCH /api/roles/v3/{selector}
Request:

Changed content type : application/json

  • Added property grantsDevFirecloudFolderOwner (string)

  • Added property grantsProdFirecloudFolderOwner (string)

  • Added property grantsQaFirecloudFolderOwner (string)

Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Added property grantsDevFirecloudFolderOwner (string)

    • Added property grantsProdFirecloudFolderOwner (string)

    • Added property grantsQaFirecloudFolderOwner (string)

GET /api/clusters/v3
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    Changed items (object):

    • Changed property requiredRoleInfo (object)

      • Added property grantsDevFirecloudFolderOwner (string)

      • Added property grantsProdFirecloudFolderOwner (string)

      • Added property grantsQaFirecloudFolderOwner (string)

POST /api/clusters/v3
Return Type:

Changed response : 201 Created

Created

  • Changed content type : application/json

    • Changed property requiredRoleInfo (object)

      • Added property grantsDevFirecloudFolderOwner (string)

      • Added property grantsProdFirecloudFolderOwner (string)

      • Added property grantsQaFirecloudFolderOwner (string)

GET /api/clusters/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property requiredRoleInfo (object)

      • Added property grantsDevFirecloudFolderOwner (string)

      • Added property grantsProdFirecloudFolderOwner (string)

      • Added property grantsQaFirecloudFolderOwner (string)

DELETE /api/clusters/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property requiredRoleInfo (object)

      • Added property grantsDevFirecloudFolderOwner (string)

      • Added property grantsProdFirecloudFolderOwner (string)

      • Added property grantsQaFirecloudFolderOwner (string)

PATCH /api/clusters/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property requiredRoleInfo (object)

      • Added property grantsDevFirecloudFolderOwner (string)

      • Added property grantsProdFirecloudFolderOwner (string)

      • Added property grantsQaFirecloudFolderOwner (string)

GET /api/environments/v3
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    Changed items (object):

    • Changed property requiredRoleInfo (object)

      • Added property grantsDevFirecloudFolderOwner (string)

      • Added property grantsProdFirecloudFolderOwner (string)

      • Added property grantsQaFirecloudFolderOwner (string)

POST /api/environments/v3
Return Type:

Changed response : 201 Created

Created

  • Changed content type : application/json

    • Changed property requiredRoleInfo (object)

      • Added property grantsDevFirecloudFolderOwner (string)

      • Added property grantsProdFirecloudFolderOwner (string)

      • Added property grantsQaFirecloudFolderOwner (string)

GET /api/environments/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property requiredRoleInfo (object)

      • Added property grantsDevFirecloudFolderOwner (string)

      • Added property grantsProdFirecloudFolderOwner (string)

      • Added property grantsQaFirecloudFolderOwner (string)

DELETE /api/environments/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property requiredRoleInfo (object)

      • Added property grantsDevFirecloudFolderOwner (string)

      • Added property grantsProdFirecloudFolderOwner (string)

      • Added property grantsQaFirecloudFolderOwner (string)

PATCH /api/environments/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property requiredRoleInfo (object)

      • Added property grantsDevFirecloudFolderOwner (string)

      • Added property grantsProdFirecloudFolderOwner (string)

      • Added property grantsQaFirecloudFolderOwner (string)

POST /api/changesets/procedures/v3/apply
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    Changed items (object):

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

GET /api/changesets/procedures/v3/chart-release-history/{chart-release}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    Changed items (object):

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

POST /api/changesets/procedures/v3/plan
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    Changed items (object):

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

Changed response : 201 Created

Created

  • Changed content type : application/json

    Changed items (object):

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

POST /api/changesets/procedures/v3/plan-and-apply
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    Changed items (object):

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

Changed response : 201 Created

Created

  • Changed content type : application/json

    Changed items (object):

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

GET /api/changesets/procedures/v3/version-history/{version-type}/{chart}/{version}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    Changed items (object):

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

GET /api/changesets/v3
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    Changed items (object):

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

GET /api/changesets/v3/{id}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

GET /api/chart-releases/v3
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    Changed items (object):

    • Changed property clusterInfo (object)

      • Changed property requiredRoleInfo (object)

        • Added property grantsDevFirecloudFolderOwner (string)

        • Added property grantsProdFirecloudFolderOwner (string)

        • Added property grantsQaFirecloudFolderOwner (string)

POST /api/chart-releases/v3
Return Type:

Changed response : 201 Created

Created

  • Changed content type : application/json

    • Changed property clusterInfo (object)

      • Changed property requiredRoleInfo (object)

        • Added property grantsDevFirecloudFolderOwner (string)

        • Added property grantsProdFirecloudFolderOwner (string)

        • Added property grantsQaFirecloudFolderOwner (string)

GET /api/chart-releases/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property clusterInfo (object)

      • Changed property requiredRoleInfo (object)

        • Added property grantsDevFirecloudFolderOwner (string)

        • Added property grantsProdFirecloudFolderOwner (string)

        • Added property grantsQaFirecloudFolderOwner (string)

DELETE /api/chart-releases/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property clusterInfo (object)

      • Changed property requiredRoleInfo (object)

        • Added property grantsDevFirecloudFolderOwner (string)

        • Added property grantsProdFirecloudFolderOwner (string)

        • Added property grantsQaFirecloudFolderOwner (string)

PATCH /api/chart-releases/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property clusterInfo (object)

      • Changed property requiredRoleInfo (object)

        • Added property grantsDevFirecloudFolderOwner (string)

        • Added property grantsProdFirecloudFolderOwner (string)

        • Added property grantsQaFirecloudFolderOwner (string)

GET /api/database-instances/v3
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    Changed items (object):

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

PUT /api/database-instances/v3
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

Changed response : 201 Created

Created

  • Changed content type : application/json

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

POST /api/database-instances/v3
Return Type:

Changed response : 201 Created

Created

  • Changed content type : application/json

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

GET /api/database-instances/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

DELETE /api/database-instances/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

PATCH /api/database-instances/v3/{selector}
Return Type:

Changed response : 200 OK

OK

  • Changed content type : application/json

    • Changed property chartReleaseInfo (object)

      • Changed property clusterInfo (object)

        • Changed property requiredRoleInfo (object)

          • Added property grantsDevFirecloudFolderOwner (string)

          • Added property grantsProdFirecloudFolderOwner (string)

          • Added property grantsQaFirecloudFolderOwner (string)

Copy link

github-actions bot commented Sep 6, 2024

Published image from a38f377 (merge b88fb89):

us-central1-docker.pkg.dev/dsp-artifact-registry/sherlock/sherlock:v1.5.49-b88fb89
us-central1-docker.pkg.dev/dsp-devops-super-prod/sherlock/sherlock:v1.5.49-b88fb89

Copy link

codecov bot commented Sep 6, 2024

Codecov Report

Attention: Patch coverage is 56.32653% with 107 lines in your changes missing coverage. Please review.

Project coverage is 69.05%. Comparing base (5e16704) to head (a38f377).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...opagation_engines/google_workspace_folder_owner.go 22.77% 78 Missing ⚠️
sherlock/internal/role_propagation/boot.go 0.00% 17 Missing ⚠️
sherlock/internal/role_propagation/propagate.go 0.00% 4 Missing and 2 partials ⚠️
...ernal/role_propagation/parallelizing_propagator.go 92.85% 1 Missing and 1 partial ⚠️
sherlock/internal/role_propagation/propagator.go 0.00% 2 Missing ⚠️
...pagation/role_propagation_mocks/mock_propagator.go 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #655      +/-   ##
==========================================
- Coverage   69.22%   69.05%   -0.18%     
==========================================
  Files         284      287       +3     
  Lines       12843    13021     +178     
==========================================
+ Hits         8891     8992     +101     
- Misses       3059     3129      +70     
- Partials      893      900       +7     
Files with missing lines Coverage Δ
sherlock/internal/api/sherlock/roles_v3.go 95.12% <100.00%> (+0.38%) ⬆️
sherlock/internal/models/role.go 63.46% <ø> (ø)
sherlock/internal/models/test_data.go 98.20% <100.00%> (+<0.01%) ⬆️
...ation/propagator_calculate_alignment_operations.go 100.00% <100.00%> (ø)
...le_propagation/propagator_get_and_filter_grants.go 100.00% <100.00%> (ø)
.../internal/role_propagation/propagator_propagate.go 100.00% <100.00%> (ø)
...role_propagation/split_string_pointer_on_commas.go 100.00% <100.00%> (ø)
...ernal/role_propagation/parallelizing_propagator.go 92.85% <92.85%> (ø)
sherlock/internal/role_propagation/propagator.go 0.00% <0.00%> (-100.00%) ⬇️
...pagation/role_propagation_mocks/mock_propagator.go 51.89% <66.66%> (+25.31%) ⬆️
... and 3 more

Copy link

sonarcloud bot commented Sep 9, 2024

@jack-r-warren jack-r-warren merged commit 8d29a01 into main Sep 9, 2024
20 checks passed
@jack-r-warren jack-r-warren deleted the DDO-3868-faster-propagation branch September 9, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants