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.

Sign up for GitHub

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

feat(web): Expose experimental account storage API #1494

Merged
merged 4 commits into from
Jan 7, 2022
Merged

feat(web): Expose experimental account storage API #1494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cade485
feat(web): Expose experimental account storage API
jvz Dec 8, 2021
bdbd1bb
Combine account and credentials endpoints
jvz Jan 7, 2022
9c7d22d
Add docs on AccountDefinition
jvz Jan 7, 2022
0e4a7c1
Add alpha annotations
jvz Jan 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions gate-core/src/main/java/com/netflix/spinnaker/gate/services/internal/ClouddriverService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.fasterxml.jackson.annotation.JsonAnySetter;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.netflix.spinnaker.kork.plugins.SpinnakerPluginDescriptor;
import java.util.ArrayList;
import java.util.Collection;
Expand All @@ -12,6 +13,7 @@
import java.util.Map;
import retrofit.client.Response;
import retrofit.http.Body;
import retrofit.http.DELETE;
import retrofit.http.GET;
import retrofit.http.Headers;
import retrofit.http.POST;
Expand All @@ -31,6 +33,21 @@ public interface ClouddriverService {
@GET("/credentials/{account}")
AccountDetails getAccount(@Path("account") String account);

@GET("/credentials/type/{type}")
List<AccountDefinition> getAccountDefinitionsByType(
@Path("type") String type,
@Query("limit") Integer limit,
@Query("startingAccountName") String startingAccountName);

@POST("/credentials")
AccountDefinition createAccountDefinition(@Body AccountDefinition accountDefinition);

@PUT("/credentials")
AccountDefinition updateAccountDefinition(@Body AccountDefinition accountDefinition);

@DELETE("/credentials/{account}")
void deleteAccountDefinition(@Path("account") String account);

@GET("/task/{taskDetailsId}")
Map getTaskDetails(@Path("taskDetailsId") String taskDetailsId);

Expand Down Expand Up @@ -493,4 +510,39 @@ public void setCloudProvider(String cloudProvider) {
private String cloudProvider;
private final Map<String, Object> details = new HashMap<String, Object>();
}

/**
* Wrapper type for Clouddriver account definitions. Clouddriver account definitions implement
* {@code CredentialsDefinition}, and its type discriminator is present in a property named
* {@code @type}. An instance of an account definition may have fairly different properties than
* its corresponding {@code AccountCredentials} instance. Account definitions must store all the
* relevant properties unchanged while {@link Account} and {@link AccountDetails} may summarize
* and remove data returned from their corresponding APIs. Account definitions must be transformed
* by a {@code CredentialsParser} before their corresponding credentials may be used by
* Clouddriver.
*/
class AccountDefinition {
private final Map<String, Object> details = new HashMap<>();
private String type;

@JsonAnyGetter
public Map<String, Object> details() {
return details;
}

@JsonAnySetter
public void set(String name, Object value) {
details.put(name, value);
}

@JsonProperty("@type")
public String getType() {
return type;
}

@JsonProperty("@type")
public void setType(String type) {
this.type = type;
}
}
}
62 changes: 62 additions & 0 deletions gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/CredentialsController.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,22 @@ import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spinnaker.gate.security.AllowedAccountsSupport
import com.netflix.spinnaker.gate.security.SpinnakerUser
import com.netflix.spinnaker.gate.services.AccountLookupService
import com.netflix.spinnaker.gate.services.internal.ClouddriverService
import com.netflix.spinnaker.gate.services.internal.ClouddriverService.Account
import com.netflix.spinnaker.gate.services.internal.ClouddriverService.AccountDetails
import com.netflix.spinnaker.kork.annotations.Alpha
import com.netflix.spinnaker.security.User
import io.swagger.annotations.ApiOperation
import io.swagger.annotations.ApiParam
import org.springframework.beans.factory.annotation.Autowired
import org.springframework.security.access.prepost.PostFilter
import org.springframework.security.access.prepost.PreAuthorize
import org.springframework.web.bind.annotation.DeleteMapping
import org.springframework.web.bind.annotation.GetMapping
import org.springframework.web.bind.annotation.PathVariable
import org.springframework.web.bind.annotation.PostMapping
import org.springframework.web.bind.annotation.PutMapping
import org.springframework.web.bind.annotation.RequestBody
import org.springframework.web.bind.annotation.RequestHeader
import org.springframework.web.bind.annotation.RequestMapping
import org.springframework.web.bind.annotation.RequestMethod
Expand All @@ -43,6 +53,9 @@ class CredentialsController {
@Autowired
AllowedAccountsSupport allowedAccountsSupport

@Autowired
ClouddriverService clouddriverService

@Autowired
ObjectMapper objectMapper

Expand Down Expand Up @@ -78,4 +91,53 @@ class CredentialsController {
@RequestHeader(value = "X-RateLimit-App", required = false) String sourceApp) {
return getAccountDetailsWithAuthorizedFlag(user).find { it.name == account }
}

@GetMapping('/type/{accountType}')
@ApiOperation('Looks up account definitions by type.')
@PostFilter("hasPermission(filterObject.name, 'ACCOUNT', 'WRITE')")
@Alpha
List<ClouddriverService.AccountDefinition> getAccountsByType(
@ApiParam(value = 'Value of the "@type" key for accounts to search for.', example = 'kubernetes')
@PathVariable String accountType,
@ApiParam('Maximum number of entries to return in results. Used for pagination.')
@RequestParam OptionalInt limit,
@ApiParam('Account name to start account definition listing from. Used for pagination.')
@RequestParam Optional<String> startingAccountName
) {
clouddriverService.getAccountDefinitionsByType(accountType, limit.isPresent() ? limit.getAsInt() : null, startingAccountName.orElse(null))
}

@PostMapping
@ApiOperation('Creates a new account definition.')
@PreAuthorize('isAuthenticated()')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I've changed permissions here to allow any authenticated user to create an account definition. It's on them to specify relevant permissions. Requires WRITE permissions to update and otherwise use the account (same semantics as READ and WRITE permissions on accounts everywhere else).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forget how this was previously, but I suspect we'd like to lock this down more than this. At least WRITE permissions makes sense to me. I suppose other permissions likely need to be in place for spinnaker to be able to use an account, so I can see how opening it up could be ok...but if nothing else it feels like opening up to mistakes, or possibly even a denial of service as extra accounts use resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, I had an admin check on these endpoints. Given that users have to supply their own credentials references (and grant the running Spinnaker access to those secrets), there doesn't seem to be much to secure here. I was thinking perhaps a config setting or table for storing groups or roles that are allowed to create/update/delete resources here, though users will only be able to affect accounts they have permission to write, and they have the ability to grant permissions during create to whoever.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess I need to learn more what's in a credentials reference...but isn't there still a possibility of some malicious user creating a ton of accounts -- even ones that spinnaker doesn't have access to -- and using a bunch of resources. I'm thinking of attempting to cache those accounts and then spamming the logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair concern, though there's already a security setting for which group(s) are required in order to login. I haven't been able to think of a better way to manage the permissions to create accounts, though perhaps that could be a config setting rather than anything dynamic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own usage, I want users to be able to configure their own account credentials without involving the developers to update our config and redeploy anything. I also want to stop using shared account credentials, but that's difficult to do without providing a way for users to configure their own credentials (i.e., I don't have access to the same systems as my users do, so the user is responsible for linking their system to ours either way whether it be via importing a secret or by granting access to my shared credentials to their system). This is also why I'm thinking it might make sense to make configurable as some use cases could be like mine while other people's use cases could be more locked down.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbyron-sf - I work with @jvz, and as jvz stated, we want people to be able to fully manage there accounts. That can be through creation or setting limitations.

One thing jvz and I have talked about is potentially allowing you to add a pluggable authorizer to allow users to specify the limitations that they wants. How does that sound?

Copy link
Contributor

@dbyron-sf dbyron-sf Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be ok...but sounds fancier/more complicated than maybe we need? Would a config flag be enough? I'm assuming fiat, so then it's configuring required permissions there...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, config flag would suffice as well. Generally permissions can get complicated, so having a pluggable authorizer may be nice. However, we can start with a config flag and if things get complicated, we can always change it :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some offline conversations, sounds like we're going to stick with isAuthorized for this PR and move to something that uses fiat as a second step. Is there some way to mark these endpoints as Alpha until they stabilize?

@Alpha
ClouddriverService.AccountDefinition createAccount(
@ApiParam('Account definition body including a discriminator field named "@type" with the account type.')
@RequestBody ClouddriverService.AccountDefinition accountDefinition
) {
clouddriverService.createAccountDefinition(accountDefinition)
}

@PutMapping
@ApiOperation('Updates an existing account definition.')
@PreAuthorize("hasPermission(#definition.name, 'ACCOUNT', 'WRITE')")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was also updated to allow users with WRITE permissions on the account to update it.

@Alpha
ClouddriverService.AccountDefinition updateAccount(
@ApiParam('Account definition body including a discriminator field named "@type" with the account type.')
@RequestBody ClouddriverService.AccountDefinition accountDefinition
) {
clouddriverService.updateAccountDefinition(accountDefinition)
}

@DeleteMapping('/{accountName}')
@ApiOperation(value = 'Deletes an account definition by name.',
notes = 'Deleted accounts can be restored via the update API. Previously deleted accounts cannot be "created" again to avoid conflicts with existing pipelines.')
@PreAuthorize("hasPermission(#definition.name, 'ACCOUNT', 'WRITE')")
@Alpha
void deleteAccount(
@ApiParam('Name of account definition to delete.')
@PathVariable String accountName
) {
clouddriverService.deleteAccountDefinition(accountName)
}
}