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

[WIP] authenticated microservices #1238

Draft
wants to merge 90 commits into
base: master
Choose a base branch
from

Conversation

petertrr
Copy link
Member

@petertrr petertrr commented Sep 19, 2022

  • Make default HttpSecurityChain have lowest precedence, because it captures all endpoints (/**) and more specific matchers should be able to override it by using higher precedence. Make ConvertingAuthenticationManager primary bean to avoid conflict when autowiring by type
  • App-to-app authentication using ServiceAccount tokens (similarly to this authentication mode of Vault)
    • Add a ServiceAccount microservice-sa for all microservices; mount its token as a projected volume
    • Add a WebClientCustomizer to add this token as a custom header; token is updated every 5 minutes
    • Add spring-security components to authorize request based on the service account token
    • Add SA-based authentication for /internal endpoints of backend and for orchestartor and sandbox
  • Switch spring-cloud-kubernetes implementation from kubernetes-client to fabric8-client in save-backend
  • Enable loading ConfigMaps using spring-cloud-kubernetes integration (for some reason resolving ${spring.datasource.backend-url} won't work with existing loading of optional properties file)

Part of #1049

* Precompiled script plugin for kotlin-jvm plugin application and configuration
* Move some dependencies to libs.versions.toml
@nulls nulls mentioned this pull request Sep 28, 2022
…ed-microservices

# Conflicts:
#	buildSrc/src/main/kotlin/com/saveourtool/save/buildutils/kotlin-jvm-configuration.gradle.kts
#	gradle/libs.versions.toml
#	save-backend/build.gradle.kts
#	save-cloud-charts/save-cloud/templates/backend-deployment.yaml
#	save-orchestrator/build.gradle.kts
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 15 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

ktlint found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

…ed-microservices

# Conflicts:
#	save-backend/src/main/kotlin/com/saveourtool/save/backend/utils/ConvertingAuthenticationManager.kt
#	save-backend/src/main/kotlin/com/saveourtool/save/backend/utils/CustomAuthenticationBasicConverter.kt
…ed-microservices

# Conflicts:
#	authentication-service/build.gradle.kts
#	save-cloud-charts/save-cloud/templates/backend-configmap.yaml
#	save-orchestrator/src/main/kotlin/com/saveourtool/save/orchestrator/service/BackendOrchestratorAgentService.kt
#	save-preprocessor/src/main/kotlin/com/saveourtool/save/preprocessor/SavePreprocessor.kt
#	save-sandbox/src/main/kotlin/com/saveourtool/save/sandbox/SaveSandbox.kt
…ces' into feature/authenticated-microservices
*/
@Configuration
class SecurityWebClientCustomizers {
@Bean

Check failure

Code scanning / ktlint

[KDOC_WITHOUT_RETURN_TAG] all methods which return values should have @return tag in KDoc: serviceAccountTokenHeaderWebClientCustomizer

[KDOC_WITHOUT_RETURN_TAG] all methods which return values should have @return tag in KDoc: serviceAccountTokenHeaderWebClientCustomizer
# Conflicts:
#	.github/workflows/helm_push.yml
#	gradle/libs.versions.toml
#	save-backend/build.gradle.kts
#	save-backend/src/main/kotlin/com/saveourtool/save/backend/SaveApplication.kt
#	save-cloud-charts/save-cloud/templates/backend-deployment.yaml
#	save-sandbox/src/main/kotlin/com/saveourtool/save/sandbox/SaveSandbox.kt
@sanyavertolet sanyavertolet self-assigned this Apr 5, 2023
@sanyavertolet sanyavertolet added enhancement New feature or request infra Issues related to build or deploy infrastructure labels Apr 5, 2023
@@ -18,6 +18,7 @@ spec:
annotations:
{{- include "pod.common.annotations" (dict "service" .Values.backend ) | nindent 8 }}
spec:
serviceAccountName: microservice-sa
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can see, all the microservices that are able to send requests should have microservice-sa (e.g. demo-cpg does not send anything so service account for it is not required(?))

Moreover, it seems that orchestrator and demo role bindings should also reference microservice cluster role

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea was, that communication between some microservices can still be secure without security, because we were planning to account only for not trusted executed tools and their network activity could be restricted with a NetworkPolicy on agent pods. This way using SA token for authentication is only required for microservices which receive requests from agents. But then, microservices that send requests to these services also need to authenticate, so it may be easier to unify everything.

Copy link
Member

Choose a reason for hiding this comment

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

As far as I can see, many things have been done: KubernetesAuthenticaitonUtils provide almost everything for X-Service-Account-Token check. So the question is: what hasn't been done yet? (regarding this PR) Seems that ktor needs to be able to add custom headers. And it seems that some Beans are required (e.g. KubernetesClient bean in KubernetesAuthenticationUtils). Am I right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose it was already more or less working. This PR affects only services, not agents, and I'm not sure if we have ktor on server-side. But yes, the part that validates token against k8s api server requires KubernetesClient. The part that adds it as a custom header requires the token to be mounted (added to YAML spec as projected volume)

Comment on lines +18 to +25
@SpringBootApplication(exclude = [
ReactiveSecurityAutoConfiguration::class,
ReactiveUserDetailsServiceAutoConfiguration::class,
ReactiveManagementWebSecurityAutoConfiguration::class,
])
@EnableWebFlux
@EnableConfigurationProperties(ConfigProperties::class)
@Import(SecurityWebClientCustomizers::class)
Copy link
Member

Choose a reason for hiding this comment

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

It seems that all the microservices should also be configured with SecurityWebClientCustomizers. What if ktor client is used? Should we implement some kind of plugin that adds required header?

@@ -18,6 +18,7 @@ spec:
annotations:
{{- include "pod.common.annotations" (dict "service" .Values.backend ) | nindent 8 }}
spec:
serviceAccountName: microservice-sa
Copy link
Member Author

Choose a reason for hiding this comment

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

The idea was, that communication between some microservices can still be secure without security, because we were planning to account only for not trusted executed tools and their network activity could be restricted with a NetworkPolicy on agent pods. This way using SA token for authentication is only required for microservices which receive requests from agents. But then, microservices that send requests to these services also need to authenticate, so it may be easier to unify everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request infra Issues related to build or deploy infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants