Skip to content

Commit

Permalink
feat(docker): Add RBAC support to Docker accounts (#6187)
Browse files Browse the repository at this point in the history
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
jvz and mergify[bot] authored Apr 17, 2024
1 parent 7b2f4c8 commit 4d786f7
Show file tree
Hide file tree
Showing 6 changed files with 250 additions and 4 deletions.
3 changes: 3 additions & 0 deletions clouddriver-docker/clouddriver-docker.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,7 @@ dependencies {
testImplementation "org.spockframework:spock-core"
testImplementation "org.spockframework:spock-spring"
testImplementation "org.springframework:spring-test"
testImplementation "org.springframework.boot:spring-boot-starter-test"
testImplementation "org.springframework.security:spring-security-test"
testImplementation "io.spinnaker.kork:kork-core"
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package com.netflix.spinnaker.clouddriver.docker.registry.config

import com.fasterxml.jackson.annotation.JsonTypeName
import com.netflix.spinnaker.credentials.definition.CredentialsDefinition
import com.netflix.spinnaker.fiat.model.resources.Permissions
import groovy.transform.EqualsAndHashCode
import groovy.transform.ToString

Expand Down Expand Up @@ -68,6 +69,8 @@ class DockerRegistryConfigurationProperties {
String catalogFile
// Allow filter the repositories by a regular expression
String repositoriesRegex
// Permissions for using this account
Permissions.Builder permissions = new Permissions.Builder()
}

List<ManagedAccount> accounts = []
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import com.netflix.spinnaker.clouddriver.docker.registry.provider.DockerRegistry
import com.netflix.spinnaker.clouddriver.docker.registry.security.DockerRegistryNamedAccountCredentials
import com.netflix.spinnaker.clouddriver.security.AccountCredentialsProvider
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.RequestMapping
import org.springframework.web.bind.annotation.RequestMethod
import org.springframework.web.bind.annotation.RequestParam
Expand All @@ -39,6 +41,7 @@ class DockerRegistryImageLookupController {
AccountCredentialsProvider accountCredentialsProvider

@RequestMapping(value = "/tags", method = RequestMethod.GET)
@PreAuthorize("hasPermission(#account, 'ACCOUNT', 'READ')")
List<String> getTags(@RequestParam('account') String account, @RequestParam('repository') String repository) {
def credentials = (DockerRegistryNamedAccountCredentials) accountCredentialsProvider.getCredentials(account)
if (!credentials) {
Expand All @@ -62,6 +65,7 @@ class DockerRegistryImageLookupController {
}

@RequestMapping(value = '/find', method = RequestMethod.GET)
@PostFilter("hasPermission(filterObject['account'], 'ACCOUNT', 'READ')")
List<Map> find(LookupOptions lookupOptions) {
def account = lookupOptions.account ?: ""

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.client.DockerOkC
import com.netflix.spinnaker.clouddriver.docker.registry.api.v2.client.DockerRegistryClient
import com.netflix.spinnaker.clouddriver.docker.registry.exception.DockerRegistryConfigException
import com.netflix.spinnaker.clouddriver.security.AbstractAccountCredentials
import com.netflix.spinnaker.fiat.model.Authorization
import com.netflix.spinnaker.fiat.model.resources.Permissions
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
import groovy.transform.CompileStatic
import groovy.util.logging.Slf4j
Expand Down Expand Up @@ -53,6 +55,7 @@ class DockerRegistryNamedAccountCredentials extends AbstractAccountCredentials<D
List<String> skip
String catalogFile
String repositoriesRegex
Permissions permissions
DockerOkClientProvider dockerOkClientProvider

Builder() {}
Expand Down Expand Up @@ -177,6 +180,11 @@ class DockerRegistryNamedAccountCredentials extends AbstractAccountCredentials<D
return this
}

Builder permissions(Permissions permissions) {
this.permissions = permissions
this
}

Builder dockerOkClientProvider(DockerOkClientProvider dockerOkClientProvider) {
this.dockerOkClientProvider = dockerOkClientProvider
return this
Expand Down Expand Up @@ -205,6 +213,8 @@ class DockerRegistryNamedAccountCredentials extends AbstractAccountCredentials<D
catalogFile,
repositoriesRegex,
insecureRegistry,
null,
permissions,
dockerOkClientProvider)
}
}
Expand Down Expand Up @@ -255,6 +265,7 @@ class DockerRegistryNamedAccountCredentials extends AbstractAccountCredentials<D
repositoriesRegex,
insecureRegistry,
null,
null,
dockerOkClientProvider)
}

Expand All @@ -281,6 +292,7 @@ class DockerRegistryNamedAccountCredentials extends AbstractAccountCredentials<D
String repositoriesRegex,
boolean insecureRegistry,
List<String> requiredGroupMembership,
Permissions permissions,
DockerOkClientProvider dockerOkClientProvider) {
if (!accountName) {
throw new IllegalArgumentException("Docker Registry account must be provided with a name.")
Expand Down Expand Up @@ -336,8 +348,8 @@ class DockerRegistryNamedAccountCredentials extends AbstractAccountCredentials<D
this.sortTagsByDate = sortTagsByDate
this.insecureRegistry = insecureRegistry;
this.skip = skip ?: []
this.requiredGroupMembership = requiredGroupMembership == null ? Collections.emptyList() : Collections.unmodifiableList(requiredGroupMembership)
this.credentials = buildCredentials(repositories, catalogFile)
this.permissions = permissions ?: buildPermissionsFromRequiredGroupMembership(requiredGroupMembership)
this.credentials = buildCredentials(repositories, catalogFile, dockerconfigFile)
}

@JsonIgnore
Expand Down Expand Up @@ -408,7 +420,7 @@ class DockerRegistryNamedAccountCredentials extends AbstractAccountCredentials<D
return CLOUD_PROVIDER
}

private DockerRegistryCredentials buildCredentials(List<String> repositories, String catalogFile) {
private DockerRegistryCredentials buildCredentials(List<String> repositories, String catalogFile, File dockerconfigFile) {
try {
DockerRegistryClient client = (new DockerRegistryClient.Builder())
.address(address)
Expand All @@ -420,6 +432,7 @@ class DockerRegistryNamedAccountCredentials extends AbstractAccountCredentials<D
.clientTimeoutMillis(clientTimeoutMillis)
.paginateSize(paginateSize)
.catalogFile(catalogFile)
.dockerconfigFile(dockerconfigFile)
.repositoriesRegex(repositoriesRegex)
.insecureRegistry(insecureRegistry)
.okClientProvider(dockerOkClientProvider)
Expand All @@ -435,6 +448,17 @@ class DockerRegistryNamedAccountCredentials extends AbstractAccountCredentials<D
}
}

private static Permissions buildPermissionsFromRequiredGroupMembership(List<String> requiredGroupMembership) {
if (requiredGroupMembership?.empty ?: true) {
return Permissions.EMPTY
}
def builder = new Permissions.Builder()
requiredGroupMembership.forEach {
builder.add(Authorization.READ, it).add(Authorization.WRITE, it)
}
builder.build()
}

private static final String CLOUD_PROVIDER = "dockerRegistry"
private final String accountName
final String environment
Expand All @@ -458,7 +482,7 @@ class DockerRegistryNamedAccountCredentials extends AbstractAccountCredentials<D
final boolean insecureRegistry
@JsonIgnore
final DockerRegistryCredentials credentials
final List<String> requiredGroupMembership
final Permissions permissions
final List<String> skip
final String catalogFile
final String repositoriesRegex
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ public DockerRegistryHealthIndicator dockerRegistryHealthIndicator(
.insecureRegistry(a.getInsecureRegistry())
.repositories(a.getRepositories())
.skip(a.getSkip())
.permissions(a.getPermissions().build())
.dockerOkClientProvider(dockerOkClientProvider)
.build(),
dockerRegistryCredentialsRepository);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
/*
* Copyright 2024 Apple, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.netflix.spinnaker.clouddriver.docker.registry.controllers;

import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

import com.netflix.spectator.api.NoopRegistry;
import com.netflix.spectator.api.Registry;
import com.netflix.spinnaker.cats.cache.CacheData;
import com.netflix.spinnaker.cats.cache.DefaultCacheData;
import com.netflix.spinnaker.cats.cache.WriteableCache;
import com.netflix.spinnaker.cats.mem.InMemoryCache;
import com.netflix.spinnaker.clouddriver.docker.registry.DockerRegistryCloudProvider;
import com.netflix.spinnaker.clouddriver.docker.registry.cache.Keys;
import com.netflix.spinnaker.clouddriver.docker.registry.controllers.DockerRegistryImageLookupControllerTest.TestConfig;
import com.netflix.spinnaker.clouddriver.docker.registry.security.DockerRegistryNamedAccountCredentials;
import com.netflix.spinnaker.clouddriver.security.AccountCredentialsProvider;
import com.netflix.spinnaker.clouddriver.security.AccountCredentialsRepository;
import com.netflix.spinnaker.clouddriver.security.DefaultAccountCredentialsProvider;
import com.netflix.spinnaker.clouddriver.security.MapBackedAccountCredentialsRepository;
import com.netflix.spinnaker.fiat.model.Authorization;
import com.netflix.spinnaker.fiat.model.UserPermission;
import com.netflix.spinnaker.fiat.model.resources.Account;
import com.netflix.spinnaker.fiat.model.resources.Permissions;
import com.netflix.spinnaker.fiat.model.resources.Role;
import com.netflix.spinnaker.fiat.shared.EnableFiatAutoConfig;
import com.netflix.spinnaker.fiat.shared.FiatService;
import com.netflix.spinnaker.fiat.shared.FiatStatus;
import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService;
import com.netflix.spinnaker.kork.dynamicconfig.SpringDynamicConfigService;
import java.util.List;
import java.util.Map;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.json.AutoConfigureJson;
import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureWebMvc;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.mock.mockito.MockBean;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Import;
import org.springframework.security.test.context.support.WithMockUser;
import org.springframework.test.web.servlet.MockMvc;

@SpringBootTest(classes = TestConfig.class, properties = "services.fiat.cache.max-entries=0")
@AutoConfigureMockMvc
@AutoConfigureWebMvc
@AutoConfigureJson
@EnableFiatAutoConfig
@WithMockUser
class DockerRegistryImageLookupControllerTest {
@Import(DockerRegistryImageLookupController.class)
static class TestConfig {
@Bean
WriteableCache cache() {
return new InMemoryCache();
}

@Bean
AccountCredentialsRepository accountCredentialsRepository() {
return new MapBackedAccountCredentialsRepository();
}

@Bean
AccountCredentialsProvider accountCredentialsProvider(
AccountCredentialsRepository accountCredentialsRepository) {
return new DefaultAccountCredentialsProvider(accountCredentialsRepository);
}

@Bean
Registry registry() {
return new NoopRegistry();
}

@Bean
DynamicConfigService dynamicConfigService() {
return new SpringDynamicConfigService();
}
}

@Autowired MockMvc mockMvc;
@Autowired WriteableCache cache;
@Autowired AccountCredentialsRepository accountCredentialsRepository;
@MockBean FiatStatus fiatStatus;
@MockBean FiatService fiatService;

@BeforeEach
void setUp() {
given(fiatStatus.isEnabled()).willReturn(true);
}

@Test
void authorizedToReadTags() throws Exception {
var permissions = createAuthorizedUserPermission();
given(fiatService.getUserPermission(eq("user"))).willReturn(permissions);

mockMvc
.perform(
get("/dockerRegistry/images/tags")
.queryParam("account", "test-account")
.queryParam("repository", "test-repository"))
.andExpect(status().isOk());
}

@Test
void notAuthorizedToReadTags() throws Exception {
var permissions = createUnauthorizedUserPermission();
given(fiatService.getUserPermission("user")).willReturn(permissions);

mockMvc
.perform(
get("/dockerRegistry/images/tags")
.queryParam("account", "test-account")
.queryParam("repository", "test-repository"))
.andExpect(status().isForbidden());
}

@Test
void canSearchForAuthorizedItems() throws Exception {
var permissions = createAuthorizedUserPermission();
given(fiatService.getUserPermission("user")).willReturn(permissions);
cache.merge(Keys.Namespace.TAGGED_IMAGE.getNs(), createTestAccountTaggedImageCacheData());
var credentials = createTestAccountCredentials();
accountCredentialsRepository.save(credentials.getName(), credentials);

mockMvc
.perform(get("/dockerRegistry/images/find"))
.andExpect(jsonPath("$[0].account").value("test-account"));
}

@Test
void filtersOutUnauthorizedItems() throws Exception {
var permissions = createUnauthorizedUserPermission();
given(fiatService.getUserPermission("user")).willReturn(permissions);
cache.merge(Keys.Namespace.TAGGED_IMAGE.getNs(), createTestAccountTaggedImageCacheData());
var credentials = createTestAccountCredentials();
accountCredentialsRepository.save(credentials.getName(), credentials);

mockMvc
.perform(get("/dockerRegistry/images/find"))
.andExpectAll(status().isOk(), jsonPath("$.length()").value(0));
}

private static UserPermission.View createAuthorizedUserPermission() {
return new UserPermission()
.setId("user")
.addResources(
List.of(
new Account()
.setName("test-account")
.setPermissions(
new Permissions.Builder().add(Authorization.READ, "user").build()),
new Role("user")))
.getView();
}

private static UserPermission.View createUnauthorizedUserPermission() {
return new UserPermission().setId("user").addResources(List.of(new Role("user"))).getView();
}

private static CacheData createTestAccountTaggedImageCacheData() {
String imageKey = Keys.getTaggedImageKey("test-account", "test-repository", "1.0");
return new DefaultCacheData(
imageKey,
Map.of(
"account",
"test-account",
"digest",
"test-digest",
"labels",
Map.of(
"commitId",
"test-commit",
"buildNumber",
"1",
"branch",
"test-branch",
"jobName",
"test-job")),
Map.of());
}

private static DockerRegistryNamedAccountCredentials createTestAccountCredentials() {
var credentials = mock(DockerRegistryNamedAccountCredentials.class);
given(credentials.getName()).willReturn("test-account");
given(credentials.getCloudProvider())
.willReturn(DockerRegistryCloudProvider.getDOCKER_REGISTRY());
given(credentials.getRegistry()).willReturn("test-registry");
return credentials;
}
}

0 comments on commit 4d786f7

Please sign in to comment.