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

[ISSUE] Secret scope ACL is set to MANAGE for all users by default and no way to override #322

Closed
jordanjennings opened this issue Sep 14, 2020 · 12 comments · Fixed by #326
Assignees
Labels
bug Something isn't working
Milestone

Comments

@jordanjennings
Copy link

Terraform Version

Terraform v0.13.2
+ provider registry.terraform.io/databrickslabs/databricks v0.2.4

Affected Resource(s)

  • databricks_secret_scope

Terraform Configuration Files

resource "databricks_secret_scope" "test" {
  name = "test"
}

Expected Behavior

A secret scope is created with only the user whose databricks authentication is being used having MANAGE access

Actual Behavior

All users are given MANAGE access to the secret scope

Steps to Reproduce

  1. terraform apply

The underlying issue appears to be that the Databricks API is somewhat limited and requires that initial_manage_principal be omitted OR be set to users. Surprisingly it does not allow a caller to specify a specific desired manage principal, even if you set the principal to match the calling user.

In the provider code I see that there's a default value of Users set for this field: https://github.com/databrickslabs/terraform-provider-databricks/blob/master/access/resource_secret_scope.go#L83

There should be no default for this field if it's not present, because the API sets the value to the current user only if the parameter is not present in the API call. Any other value is rejected by the API with a 400 error code.

@jordanjennings jordanjennings added the bug Something isn't working label Sep 14, 2020
@nfx nfx added this to the v0.3.0 milestone Sep 14, 2020
@stikkireddy
Copy link
Contributor

The only supported principal for this option is the group users, which contains all users in the workspace. If initial_manage_principal is not specified, the initial ACL with MANAGE permission applied to the scope is assigned to the API request issuer’s user identity. From the api documentation from the databricks api: https://docs.databricks.com/dev-tools/api/latest/secrets.html. You have two options one is initial_acl will be "users" or no initial managed principal then it will be user used to create the secret. Those are the only two options provided by the API.

To get around this you can create an acl and associate that with the scope and then there will be an actual ACL that is tied to the scope. Let me know if that addresses your concerns/question. @jordanjennings

@jordanjennings
Copy link
Author

If initial_manage_principal is not specified, the initial ACL with MANAGE permission applied to the scope is assigned to the API request issuer’s user identity.

@stikkireddy The workaround does not remove the users ACL with MANAGE access.

So for example, with this terraform:

resource "databricks_secret_scope" "test" {
  name = "test"
}

I end up with an ACL like:

Principal    Permission
-----------  ------------
users        MANAGE

And with what I think you're suggesting as a workaround, like this:

resource "databricks_secret_scope" "test" {
  name = "test"
}

resource "databricks_secret_acl" "test_acl" {
  principal = "some.user@email.com"
  permission = "MANAGE"
  scope = databricks_secret_scope.test.name
}

I end up with this ACL:

Principal                      Permission
-----------------------------  ------------
users                          MANAGE
some.user@email.com            MANAGE

As far as I can see there is no way to remove users from the ACL.

Note that in a previous version of the provider I was able to provide an empty string like this:

resource "databricks_secret_scope" "test" {
  name = "test"
  initial_manage_principal = ""
}

However, older versions of the provider had numerous other bugs so rolling back isn't a viable option. Let me know if I can provide more details to clarify anything.

@nfx
Copy link
Contributor

nfx commented Sep 16, 2020

@jordanjennings which cloud are you using?

@jordanjennings
Copy link
Author

@nfx Running on Azure Databricks

@alexott
Copy link
Contributor

alexott commented Sep 17, 2020

@jordanjennings what would you prefer - disabling Users via setting the empty string explicitly, or maybe it's better to have a separate boolean variable, like, set_initial_principal ?

@jordanjennings
Copy link
Author

jordanjennings commented Sep 17, 2020

@alexott I would prefer not to specify the value at all, because then I would reasonably expect it to do whatever is the default behavior for the API.

That said, assuming that the underlying API isn't going to change, I would suggest a design change to this resource to be more clear and have something like:

resource "databricks_secret_scope" "test" {
  name = "test"
  allow_all_users_to_manage = false # optional field that defaults to false
}

My order of preference for how to fix this issue:

  1. Design change as noted above
  2. Keep initial_manage_principal but omitting the field from the resource also omits it from the API call
  3. Set initial_manage_principal to empty string to indicate that it should be omitted from the API call and default to the calling user

@alexott
Copy link
Contributor

alexott commented Sep 17, 2020

@jordanjennings thank you for your preferences - I'll implement first option tomorrow. I'm not sure about setting this field to false by default because it will change existing behavior

@nfx
Copy link
Contributor

nfx commented Sep 17, 2020

allow_all_users_to_manage is semantically not possible. if databricks_secret_acl is created for all users/users, then we'd need to have a check for other ACLs to keep this property reflecting the reality.

i'll prefer to remove default value of initial_manage_principal on resource schema, which is more expected behaviour for terraform. respective documentation on databricks_secret_acl or databricks_secret should be modified as well. So this is a combination of options 2 & 3 from previous comments.

@jordanjennings
Copy link
Author

@nfx Good point, since option 1 is not viable then just to be overly clear:

Option 2 (strongly preferred):

resource "databricks_secret_scope" "test" {
  name = "test"
}

Option 3:

resource "databricks_secret_scope" "test" {
  name = "test"
  initial_manage_principal = ""
}

@nfx
Copy link
Contributor

nfx commented Sep 17, 2020

option 2 is more in line with long-term vision on provider.

@nfx nfx modified the milestones: v0.3.0, 0.2.6 Sep 27, 2020
@nfx nfx added the Small Size label Sep 30, 2020
@nfx nfx closed this as completed in #326 Oct 2, 2020
@nfx
Copy link
Contributor

nfx commented Oct 5, 2020

@jordanjennings released in 0.2.7

@marc88
Copy link

marc88 commented Nov 17, 2023

Hello,

I have an ACL on a scope as follows. This does not state that the default 'users' group has any privilege on the resource. Hence, I assumed it should be a 'DENY' for 'users' by default.

{ "items": [ { "principal": "user123", "permission": "MANAGE" } ] }

But, surprisingly, users other than 'user123' are also able to READ and use secrets from this scope. The other users have a workspace admin privileges (Entitlements: Workspace access, Databricks SQL access, Allow unrestricted cluster creation, allow-instance-pool-create).
Also, users that have only Workspace access and Databricks SQL access are all able to access this scope.

Question,

  1. How is it possible when there is no ACL granting privileges to the scope, for 'users'?
  2. There is no way in the databricks REST API documentation, to 'DENY' them this privilege. Is there a workaround? Or maybe a way to set a DENY ACL?

Regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants