refactor(auth): refactor auth propagation for pipelines (spinnaker#4081)

Cleans up a bunch of deprecations to enable some further refactors in kork around auth: * explicitly use runAs (rather than deprecated propagate with a principal object) when invoking as a specific user * refactors away from deprecated kork-security object in favor of Pipeline.AuthenticationDetails for passing auth * adds a #currentUser() SpEL expression that will retrieve the user in the current execution context (for example downstream of a manual judgment with auth propagation it will return the new user)
amanya · Mar 15, 2021 · c4f2ae7 · c4f2ae7
1 parent e52dc9c
commit c4f2ae7
Show file tree

Hide file tree

Showing 17 changed files with 84 additions and 90 deletions.
diff --git a/orca-api/src/main/java/com/netflix/spinnaker/orca/api/pipeline/models/PipelineExecution.java b/orca-api/src/main/java/com/netflix/spinnaker/orca/api/pipeline/models/PipelineExecution.java
@@ -15,14 +15,14 @@
  */
 package com.netflix.spinnaker.orca.api.pipeline.models;
 
-import static java.util.Arrays.asList;
 import static java.util.Collections.emptySet;
 
-import com.google.common.collect.ImmutableSet;
 import com.netflix.spinnaker.kork.annotations.Beta;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
@@ -161,18 +161,25 @@ public void setUser(@Nullable String user) {
     private Collection<String> allowedAccounts = emptySet();
 
     public Collection<String> getAllowedAccounts() {
-      return ImmutableSet.copyOf(allowedAccounts);
+      return allowedAccounts;
     }
 
     public void setAllowedAccounts(Collection<String> allowedAccounts) {
-      this.allowedAccounts = ImmutableSet.copyOf(allowedAccounts);
+      this.allowedAccounts = Set.copyOf(allowedAccounts);
     }
 
-    public AuthenticationDetails() {}
+    public AuthenticationDetails() {
+      this(null, Collections.emptySet());
+    }
 
     public AuthenticationDetails(String user, String... allowedAccounts) {
+      this(user, Set.of(allowedAccounts));
+    }
+
+    public AuthenticationDetails(String user, Collection<String> allowedAccounts) {
       this.user = user;
-      this.allowedAccounts = asList(allowedAccounts);
+      this.allowedAccounts =
+          allowedAccounts == null ? Collections.emptySet() : Set.copyOf(allowedAccounts);
     }
   }
 

diff --git a/orca-api/src/main/java/com/netflix/spinnaker/orca/api/pipeline/models/StageExecution.java b/orca-api/src/main/java/com/netflix/spinnaker/orca/api/pipeline/models/StageExecution.java
@@ -17,7 +17,6 @@
 
 import static java.util.Collections.emptySet;
 
-import com.google.common.collect.ImmutableSet;
 import com.netflix.spinnaker.kork.annotations.Beta;
 import com.netflix.spinnaker.orca.api.pipeline.SyntheticStageOwner;
 import java.util.*;
@@ -195,11 +194,11 @@ class LastModifiedDetails {
     @NonNull private Long lastModifiedTime;
 
     public @Nonnull Collection<String> getAllowedAccounts() {
-      return ImmutableSet.copyOf(allowedAccounts);
+      return Set.copyOf(allowedAccounts);
     }
 
     public void setAllowedAccounts(@Nonnull Collection<String> allowedAccounts) {
-      this.allowedAccounts = ImmutableSet.copyOf(allowedAccounts);
+      this.allowedAccounts = Set.copyOf(allowedAccounts);
     }
   }
 }
diff --git a/.../java/com/netflix/spinnaker/orca/clouddriver/pollers/RestorePinnedServerGroupsPoller.java b/.../java/com/netflix/spinnaker/orca/clouddriver/pollers/RestorePinnedServerGroupsPoller.java
@@ -35,8 +35,6 @@
 import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionNotFoundException;
 import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository;
 import com.netflix.spinnaker.security.AuthenticatedRequest;
-import com.netflix.spinnaker.security.User;
-import groovy.util.logging.Slf4j;
 import java.util.*;
 import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
@@ -47,7 +45,6 @@
 import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression;
 import org.springframework.stereotype.Component;
 
-@Slf4j
 @Component
 @ConditionalOnExpression(value = "${pollers.restore-pinned-server-groups.enabled:false}")
 public class RestorePinnedServerGroupsPoller extends AbstractPollingNotificationAgent {
@@ -151,19 +148,17 @@ protected void tick() {
       try {
         List<Map<String, Object>> jobs = new ArrayList<>();
         jobs.add(buildDeleteEntityTagsOperation(pinnedServerGroupTag));
-
-        User systemUser = new User();
-        systemUser.setUsername(username);
-        systemUser.setAllowedAccounts(Collections.singletonList(pinnedServerGroupTag.account));
+        var allowedAccount = Collections.singletonList(pinnedServerGroupTag.account);
 
         Optional<ServerGroup> serverGroup =
-            AuthenticatedRequest.propagate(
+            AuthenticatedRequest.runAs(
+                    username,
+                    allowedAccount,
                     () ->
                         pollerSupport.fetchServerGroup(
                             pinnedServerGroupTag.account,
                             pinnedServerGroupTag.location,
-                            pinnedServerGroupTag.serverGroup),
-                    systemUser)
+                            pinnedServerGroupTag.serverGroup))
                 .call();
 
         serverGroup.ifPresent(
@@ -183,12 +178,13 @@ protected void tick() {
             buildCleanupOperation(pinnedServerGroupTag, serverGroup, jobs);
         log.info((String) cleanupOperation.get("name"));
 
-        AuthenticatedRequest.propagate(
+        AuthenticatedRequest.runAs(
+                username,
+                allowedAccount,
                 () ->
                     executionLauncher.start(
                         ExecutionType.ORCHESTRATION,
-                        objectMapper.writeValueAsString(cleanupOperation)),
-                systemUser)
+                        objectMapper.writeValueAsString(cleanupOperation)))
             .call();
 
         triggeredCounter.increment();

diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/AuthenticatedStage.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/AuthenticatedStage.java
@@ -15,8 +15,8 @@
  */
 package com.netflix.spinnaker.orca;
 
+import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution;
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution;
-import com.netflix.spinnaker.security.User;
 import java.util.Optional;
 
 /**
@@ -28,5 +28,5 @@
  * module for just the User class?
  */
 public interface AuthenticatedStage {
-  Optional<User> authenticatedUser(StageExecution stage);
+  Optional<PipelineExecution.AuthenticationDetails> authenticatedUser(StageExecution stage);
 }
diff --git a/...etflix/spinnaker/orca/pipeline/expressions/functions/StageExpressionFunctionProvider.java b/...etflix/spinnaker/orca/pipeline/expressions/functions/StageExpressionFunctionProvider.java
@@ -24,6 +24,7 @@
 import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution;
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution;
 import java.util.Arrays;
+import java.util.Optional;
 import java.util.function.Predicate;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -53,6 +54,8 @@ public Functions getFunctions() {
                 PipelineExecution.class,
                 "execution",
                 "The execution containing the currently executing stage")),
+        new FunctionDefinition(
+            "currentUser", "Looks up the current authenticated user within the execution context."),
         new FunctionDefinition(
             "stageByRefId",
             "Locates and returns a stage with the given refId",
@@ -106,6 +109,14 @@ public static StageExecution currentStage(PipelineExecution execution) {
                 new SpelHelperFunctionException("No stage found with id '" + currentStageId + "'"));
   }
 
+  /** @return the current authenticated user in the Execution or anonymous. */
+  @SuppressWarnings("unused")
+  public static String currentUser() {
+    return Optional.ofNullable(ExecutionContext.get())
+        .map(ExecutionContext::getAuthenticatedUser)
+        .orElse("anonymous");
+  }
+
   /**
    * Finds a Stage by refId. This function should only be used by programmatic pipeline generators,
    * as refIds are fragile and may change from execution-to-execution.
@@ -186,6 +197,7 @@ public static String judgment(PipelineExecution execution, String id) {
   }
 
   /** Alias to judgment */
+  @SuppressWarnings("unused")
   public static String judgement(PipelineExecution execution, String id) {
     return judgment(execution, id);
   }

diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/PipelineExecutionImpl.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/PipelineExecutionImpl.java
@@ -33,7 +33,6 @@
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution;
 import com.netflix.spinnaker.orca.api.pipeline.models.Trigger;
 import com.netflix.spinnaker.security.AuthenticatedRequest;
-import com.netflix.spinnaker.security.User;
 import de.huxhorn.sulky.ulid.ULID;
 import java.io.Serializable;
 import java.util.ArrayList;
@@ -435,17 +434,5 @@ public static Optional<AuthenticationDetails> build() {
 
       return Optional.empty();
     }
-
-    public static Optional<User> toKorkUser(AuthenticationDetails authentication) {
-      return Optional.ofNullable(authentication)
-          .map(AuthenticationDetails::getUser)
-          .map(
-              it -> {
-                User user = new User();
-                user.setEmail(it);
-                user.setAllowedAccounts(authentication.getAllowedAccounts());
-                return user;
-              });
-    }
   }
 }
diff --git a/...-echo/src/main/groovy/com/netflix/spinnaker/orca/echo/pipeline/ManualJudgmentStage.groovy b/...-echo/src/main/groovy/com/netflix/spinnaker/orca/echo/pipeline/ManualJudgmentStage.groovy
@@ -21,9 +21,11 @@ import com.fasterxml.jackson.annotation.JsonAnySetter
 import com.fasterxml.jackson.annotation.JsonIgnore
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.OverridableTimeoutRetryableTask
+import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.echo.util.ManualJudgmentAuthorization
+import com.netflix.spinnaker.orca.pipeline.model.PipelineExecutionImpl
 
 import javax.annotation.Nonnull
 import java.util.concurrent.TimeUnit
@@ -32,7 +34,6 @@ import com.netflix.spinnaker.orca.*
 import com.netflix.spinnaker.orca.echo.EchoService
 import com.netflix.spinnaker.orca.api.pipeline.graph.StageDefinitionBuilder
 import com.netflix.spinnaker.orca.api.pipeline.graph.TaskNode
-import com.netflix.spinnaker.security.User
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
@@ -53,17 +54,16 @@ class ManualJudgmentStage implements StageDefinitionBuilder, AuthenticatedStage
   }
 
   @Override
-  Optional<User> authenticatedUser(StageExecution stage) {
+  Optional<PipelineExecution.AuthenticationDetails> authenticatedUser(StageExecution stage) {
     def stageData = stage.mapTo(StageData)
     if (stageData.state != StageData.State.CONTINUE || !stage.lastModified?.user || !stageData.propagateAuthenticationContext) {
       return Optional.empty()
     }
 
-    def user = new User()
-    user.setAllowedAccounts(stage.lastModified.allowedAccounts)
-    user.setUsername(stage.lastModified.user)
-    user.setEmail(stage.lastModified.user)
-    return Optional.of(user.asImmutable())
+    return Optional.of(
+        new PipelineExecution.AuthenticationDetails(
+            stage.lastModified.user,
+            stage.lastModified.allowedAccounts));
   }
 
   @Slf4j

diff --git a/.../main/groovy/com/netflix/spinnaker/orca/echo/spring/EchoNotifyingExecutionListener.groovy b/.../main/groovy/com/netflix/spinnaker/orca/echo/spring/EchoNotifyingExecutionListener.groovy
@@ -23,7 +23,6 @@ import com.netflix.spinnaker.orca.front50.Front50Service
 import com.netflix.spinnaker.orca.front50.model.ApplicationNotifications
 import com.netflix.spinnaker.orca.listeners.ExecutionListener
 import com.netflix.spinnaker.orca.listeners.Persister
-import com.netflix.spinnaker.orca.pipeline.model.PipelineExecutionImpl
 import com.netflix.spinnaker.orca.pipeline.util.ContextParameterProcessor
 import com.netflix.spinnaker.security.AuthenticatedRequest
 import groovy.transform.CompileStatic
@@ -121,12 +120,12 @@ class EchoNotifyingExecutionListener implements ExecutionListener {
    * @param pipeline
    */
   private void addApplicationNotifications(PipelineExecution pipeline) {
-    def user = PipelineExecutionImpl.AuthenticationHelper.toKorkUser(pipeline.getAuthentication())
+    def user = pipeline.authentication?.user
     ApplicationNotifications notifications
-    if (user?.isPresent()) {
-      notifications = AuthenticatedRequest.propagate({
+    if (user) {
+      notifications = AuthenticatedRequest.runAs(user, pipeline.authentication.allowedAccounts ?: [] as Collection<String>, {
         front50Service.getApplicationNotifications(pipeline.application)
-      }, user.get()).call()
+      }).call()
     } else {
       notifications = AuthenticatedRequest.allowAnonymous({
         front50Service.getApplicationNotifications(pipeline.application)

diff --git a/...o/src/test/groovy/com/netflix/spinnaker/orca/echo/pipeline/ManualJudgmentStageSpec.groovy b/...o/src/test/groovy/com/netflix/spinnaker/orca/echo/pipeline/ManualJudgmentStageSpec.groovy
@@ -236,7 +236,7 @@ class ManualJudgmentStageSpec extends Specification {
 
     then:
     authenticatedUser.isPresent() == isPresent
-    !isPresent || (authenticatedUser.get().username == "modifiedUser" && authenticatedUser.get().allowedAccounts == ["group1"])
+    !isPresent || (authenticatedUser.get().user == "modifiedUser" && authenticatedUser.get().allowedAccounts.toList() == ["group1"])
 
     where:
     judgmentStatus | propagateAuthenticationContext || isPresent

diff --git a/.../com/netflix/spinnaker/orca/echo/spring/EchoNotifyingPipelineExecutionListenerSpec.groovy b/.../com/netflix/spinnaker/orca/echo/spring/EchoNotifyingPipelineExecutionListenerSpec.groovy
@@ -225,7 +225,7 @@ class EchoNotifyingPipelineExecutionListenerSpec extends Specification {
 
     1 * front50Service.getApplicationNotifications("myapp") >> {
       assert MDC.get(Header.USER.header) == "[email protected]"
-      assert MDC.get(Header.ACCOUNTS.header) == "someAccount,anotherAccount"
+      assert MDC.get(Header.ACCOUNTS.header).split(",").toList().toSorted() == ["anotheraccount", "someaccount"]
       return notifications
     }
     1 * echoService.recordEvent(_)

diff --git a/...ront50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy b/...ront50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy
@@ -28,7 +28,6 @@ import com.netflix.spinnaker.orca.pipeline.ExecutionLauncher
 import com.netflix.spinnaker.orca.pipeline.util.ArtifactUtils
 import com.netflix.spinnaker.orca.pipeline.util.ContextParameterProcessor
 import com.netflix.spinnaker.security.AuthenticatedRequest
-import com.netflix.spinnaker.security.User
 import groovy.util.logging.Slf4j
 import org.springframework.beans.BeansException
 import org.springframework.beans.factory.annotation.Autowired
@@ -70,7 +69,7 @@ class DependentPipelineStarter implements ApplicationContextAware {
                             PipelineExecution parentPipeline,
                             Map suppliedParameters,
                             String parentPipelineStageId,
-                            User principal) {
+                            PipelineExecution.AuthenticationDetails authenticationDetails) {
     def json = objectMapper.writeValueAsString(pipelineConfig)
 
     if (pipelineConfig.disabled) {
@@ -81,7 +80,7 @@ class DependentPipelineStarter implements ApplicationContextAware {
 
     pipelineConfig.trigger = [
       type                 : "pipeline",
-      user                 : principal?.username ?: user ?: "[anonymous]",
+      user                 : authenticationDetails?.user ?: user ?: "[anonymous]",
       parentExecution      : parentPipeline,
       parentPipelineStageId: parentPipelineStageId,
       parameters           : [:],
@@ -153,11 +152,11 @@ class DependentPipelineStarter implements ApplicationContextAware {
     log.info('running pipeline {}:{}', pipelineConfig.id, json)
 
     log.debug("Source thread: MDC user: " + AuthenticatedRequest.getAuthenticationHeaders() +
-      ", principal: " + principal?.toString())
+      ", principal: " + authenticationDetails?.toString())
 
     Callable<PipelineExecution> callable
     if (artifactError == null) {
-      callable = AuthenticatedRequest.propagate({
+      callable = {
         log.debug("Destination thread user: " + AuthenticatedRequest.getAuthenticationHeaders())
         return executionLauncher().start(PIPELINE, json).with {
           Id id = registry.createId("pipelines.triggered")
@@ -166,15 +165,17 @@ class DependentPipelineStarter implements ApplicationContextAware {
           registry.counter(id).increment()
           return it
         }
-      } as Callable<PipelineExecution>, true, principal)
+      } as Callable<PipelineExecution>
     } else {
-      callable = AuthenticatedRequest.propagate({
+      callable = {
         log.debug("Destination thread user: " + AuthenticatedRequest.getAuthenticationHeaders())
         return executionLauncher().fail(PIPELINE, json, artifactError)
-      } as Callable<PipelineExecution>, true, principal)
+      } as Callable<PipelineExecution>
     }
 
-    def pipeline = callable.call()
+    def pipeline = authenticationDetails?.user ?
+        AuthenticatedRequest.runAs(authenticationDetails.user, authenticationDetails.allowedAccounts, callable).call() :
+        AuthenticatedRequest.propagate(callable).call()
 
     log.info('executing dependent pipeline {}', pipeline.id)
     return pipeline

diff --git a/...roovy/com/netflix/spinnaker/orca/front50/spring/DependentPipelineExecutionListener.groovy b/...roovy/com/netflix/spinnaker/orca/front50/spring/DependentPipelineExecutionListener.groovy
@@ -28,7 +28,6 @@ import com.netflix.spinnaker.orca.listeners.Persister
 import com.netflix.spinnaker.orca.pipeline.util.ContextParameterProcessor
 import com.netflix.spinnaker.orca.pipelinetemplate.V2Util
 import com.netflix.spinnaker.security.AuthenticatedRequest
-import com.netflix.spinnaker.security.User
 import groovy.transform.CompileDynamic
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
@@ -101,11 +100,10 @@ class DependentPipelineExecutionListener implements ExecutionListener {
             trigger.pipeline == execution.pipelineConfigId &&
             trigger.status.contains(status)
           ) {
-            User authenticatedUser = null
+            PipelineExecution.AuthenticationDetails authenticatedUser = null
 
             if (fiatStatus.enabled && trigger.runAsUser) {
-              authenticatedUser = new User()
-              authenticatedUser.setEmail(trigger.runAsUser)
+              authenticatedUser = new PipelineExecution.AuthenticationDetails(trigger.runAsUser)
             }
 
             dependentPipelineStarter.trigger(