Enabling using secret values during a deployment #1755

karrgov · 2025-12-19T11:57:33Z

LMCROSSITXSADEPLOY-2301

vkalapov · 2025-12-30T09:32:27Z

...re/src/main/java/org/cloudfoundry/multiapps/controller/core/helpers/MtaDescriptorMerger.java


-    public DeploymentDescriptor merge(DeploymentDescriptor deploymentDescriptor, List<ExtensionDescriptor> extensionDescriptors) {
+    public DeploymentDescriptor merge(DeploymentDescriptor deploymentDescriptor, List<ExtensionDescriptor> extensionDescriptors,
+                                      List<String> parameterNamesToBeCensored) {


I think that parameterNamesToBeMasked, Hidden, Redacted, flow better here for the name.

vkalapov · 2025-12-30T09:50:14Z

...iapps-controller-core/src/main/java/org/cloudfoundry/multiapps/controller/core/Messages.java

    public static final String BUILDPACKS_NOT_ALLOWED_WITH_DOCKER = "Buildpacks must not be provided when lifecycle is set to 'docker'.";
    public static final String EXTENSION_DESCRIPTORS_COULD_NOT_BE_PARSED_TO_VALID_YAML = "Extension descriptor(s) could not be parsed as a valid YAML file. These descriptors may fail future deployments once stricter validation is enforced. Please review and correct them now to avoid future issues. Use at your own risk";
    public static final String UNSUPPORTED_FILE_FORMAT = "Unsupported file format! \"{0}\" detected";
+    public static final String ENCRYPTION_BOUNCY_CASTLE_AES256_HAS_FAILED = "Encryption with AES256 by Bouncy Castle has failed!";


This message is too specific, we should avoid leaking implementation details (algorithm and the provider). Consider using a more generic message.

vkalapov · 2025-12-30T09:57:00Z

...in/resources/org/cloudfoundry/multiapps/controller/persistence/db/changelog/db-changelog.xml

+    <include
+            file="/org/cloudfoundry/multiapps/controller/persistence/db/changelog/db-changelog-TEST-SECRET-TOKEN-TABLE-ADDITION-persistence.xml"/>


Why was this added?

It is the file, responsible for introducing the new table in our database - secret_token. That is where we hold the encrypted secret values while a deployment is ongoing

vkalapov · 2025-12-30T10:14:01Z

...rocess/src/main/java/org/cloudfoundry/multiapps/controller/process/steps/ProcessContext.java

+        Set<String> secretParameters = VariableHandling.get(execution, Variables.SECURE_EXTENSION_DESCRIPTOR_PARAMETER_NAMES);
+        DynamicSecureSerialization dynamicSecureSerialization = SecureSerializationFactory.ofAdditionalValues(secretParameters);


These two lines follow the same pattern and appear in several files (with minor variations). Might be worth extracting into a small utility or helper.

Fixed on all places, thanks :)

vkalapov · 2025-12-30T12:17:09Z

...er/process/security/resolver/MissingCredentialsFromUserProvidedServiceEncryptionRelated.java

@@ -0,0 +1,17 @@
+package org.cloudfoundry.multiapps.controller.process.security.resolver;
+
+public class MissingCredentialsFromUserProvidedServiceEncryptionRelated extends RuntimeException {


Name feels too long and missing a exception suffix. Consider shortening it.

vkalapov · 2026-01-05T11:18:44Z

...oudfoundry/multiapps/controller/persistence/query/providers/SqlSecretTokenQueryProvider.java

+                    if (resultSet.next()) {
+                        return resultSet.getLong(1);
+                    }
+                    throw new SQLException("INSERT secret_token did not return an id");


Could we include the process instance and variable name in the exception message?

vkalapov · 2026-01-05T11:26:56Z

.../src/main/java/org/cloudfoundry/multiapps/controller/process/steps/MergeDescriptorsStep.java

+
+    private MultiValuedMap<String, String> getParametersNameValueMapFromExtensionDescriptors(
+        List<ExtensionDescriptor> extensionDescriptors) {
+        MultiValuedMap<String, String> parametersNameValueMapFromExtensionDescriptors = new ArrayListValuedHashMap<>();


parametersNameValueMapFromExtensionDescriptors and parametersNameValueMapFromDeploymentDescriptor seems to be very long. Consider a shorter, more descriptive name to improve clarity.

Fixed, since I have changed the logic to follow the Visitor pattern :)

vkalapov · 2026-01-05T11:31:47Z

.../src/main/java/org/cloudfoundry/multiapps/controller/process/steps/MergeDescriptorsStep.java

+    private Set<String> getNestedParameterNamesToBeCensored(MultiValuedMap<String, String> parameterNameValueMap,
+                                                            Set<String> parameterNamesToBeCensored) {
+        Set<String> nestedParameterNamesToBeCensored = new HashSet<>();
+        for (Map.Entry<String, Collection<String>> parameterEntryInStringType : parameterNameValueMap.asMap()
+                                                                                                     .entrySet()) {
+            List<String> entryValuesToString = parameterEntryInStringType.getValue()
+                                                                         .stream()
+                                                                         .map(String::toString)
+                                                                         .toList();
+            for (String complexValue : entryValuesToString) {
+                for (String nameToBeCensored : parameterNamesToBeCensored) {
+                    if (complexValue.contains(nameToBeCensored)) {
+                        nestedParameterNamesToBeCensored.add(parameterEntryInStringType.getKey());
+                    }
+                }
+            }
+        }


The nested loops make the logic a bit hard to follow. Could we simplify by using streams or helper methods?

Fixed, since I have changed the logic to follow the Visitor pattern :)

vkalapov · 2026-01-05T11:38:54Z

.../src/main/java/org/cloudfoundry/multiapps/controller/process/steps/MergeDescriptorsStep.java

+    private Map<String, String> getParametersStringCastedValue(ParametersContainer parametersContainer) {
+        return parametersContainer.getParameters()
+                                  .entrySet()
+                                  .stream()
+                                  .collect(Collectors.toMap(Map.Entry::getKey,
+                                                            currentParameter -> Objects.toString(currentParameter.getValue(), "")));
+    }
+
+    private Map<String, String> getPropertiesStringCastedValue(PropertiesContainer propertiesContainer) {
+        return propertiesContainer.getProperties()
+                                  .entrySet()
+                                  .stream()
+                                  .collect(Collectors.toMap(Map.Entry::getKey,
+                                                            currentProperty -> Objects.toString(currentProperty.getValue(), "")));
+    }


getParametersStringCastedValue and getPropertiesStringCastedValue share almost identical logic. Consider a generic helper method to reduce duplication.

Fixed, since I have changed the logic to follow the Visitor pattern :)

vkalapov · 2026-01-05T11:40:58Z

.../src/main/java/org/cloudfoundry/multiapps/controller/process/steps/MergeDescriptorsStep.java

+    private void getParametersAndPropertiesPerModule(MultiValuedMap<String, String> multiValuedMap, Module module, boolean isRequired,
+                                                     boolean isWhole, boolean isProvided) {


Long boolean flag lists reduce readability. Consider replacing them with an enum or separate helper methods for each case.

Fixed, since I have changed the logic to follow the Visitor pattern :)

multiapps-controller-core/src/main/java/module-info.java

...n/java/org/cloudfoundry/multiapps/controller/core/security/encryption/AesEncryptionUtil.java

.../cloudfoundry/multiapps/controller/process/security/resolver/SecretTokenKeyResolverImpl.java

...n/java/org/cloudfoundry/multiapps/controller/core/security/encryption/AesEncryptionUtil.java

...ess/src/main/java/org/cloudfoundry/multiapps/controller/process/jobs/SecretTokenCleaner.java

...udfoundry/multiapps/controller/process/security/SecretTransformationStrategyContextImpl.java

...ntroller/process/security/resolver/MissingUserProvidedServiceEncryptionRelatedException.java

.../cloudfoundry/multiapps/controller/process/security/resolver/SecretTokenKeyResolverImpl.java

.../org/cloudfoundry/multiapps/controller/process/steps/ProcessMtaExtensionDescriptorsStep.java

LMCROSSITXSADEPLOY-2301

IvanBorislavovDimitrov · 2026-01-23T09:16:08Z

...dfoundry/multiapps/controller/core/security/serialization/SecureSerializerConfiguration.java

+        for (String additionalSensitiveElement : additionalSensitiveElementNames) {
+            boolean isNotExistent = mergedSensitiveElementNames.stream()
+                                                               .noneMatch(sensitiveElement -> sensitiveElement.equalsIgnoreCase(
+                                                                   additionalSensitiveElement));


Why is it needed to have this check? Why not adding them as they are unique in the set?

IvanBorislavovDimitrov · 2026-01-23T09:20:18Z

...er-persistence/src/main/java/org/cloudfoundry/multiapps/controller/persistence/Messages.java

    public static final String BACKUP_DESCRIPTOR_WITH_ID_NOT_EXIST = "Backup descriptor with ID \"{0}\" does not exist";
+    public static final String SECRET_TOKEN_WITH_ID_NOT_EXIST = "Secret token with ID \"{0}\" does not exist";
    public static final String DATABASE_HEALTH_CHECK_FAILED = "Database health check failed";
+    public static final String INSERT_QUERY_NOT_RETURN_ID_FOR_VARIABLE_NAME_0_AND_PROCESS_WITH_ID_1 = "INSERT into secret_token table did not return an id for a variable name \"{0}\" and process with id \"{1}\"";


not used, delete it

IvanBorislavovDimitrov · 2026-01-23T09:22:36Z

...in/resources/org/cloudfoundry/multiapps/controller/persistence/db/changelog/db-changelog.xml

    <include file="/org/cloudfoundry/multiapps/controller/persistence/db/changelog/db-changelog-2.35.0-persistence.xml"/>

+    <include
+            file="/org/cloudfoundry/multiapps/controller/persistence/db/changelog/db-changelog-TEST-SECRET-TOKEN-TABLE-ADDITION-persistence.xml"/>


version reminder

IvanBorislavovDimitrov · 2026-01-23T09:24:54Z

.../cloudfoundry/multiapps/controller/process/security/resolver/SecretTokenKeyResolverImpl.java

+@Named
+public class SecretTokenKeyResolverImpl implements SecretTokenKeyResolver {
+
+    CloudControllerClientProvider cloudControllerClientProvider;


why there is no modifier?

IvanBorislavovDimitrov · 2026-01-23T09:25:10Z

.../cloudfoundry/multiapps/controller/process/security/resolver/SecretTokenKeyResolverImpl.java

+    public SecretTokenKeyResolverImpl() {
+    }


IvanBorislavovDimitrov · 2026-01-23T14:58:31Z

.../src/main/java/org/cloudfoundry/multiapps/controller/process/steps/SecureProcessContext.java

+        //        SecretTransformationStrategy secretTransformationStrategyContext = new SecretTransformationStrategyContextImpl(
+        //            secureParameterNames);
+        //        SecureSerializerConfiguration secureSerializerConfiguration = new SecureSerializerConfiguration();
+        //        secureSerializerConfiguration.setAdditionalSensitiveElementNames(secureParameterNames);


why commented?

IvanBorislavovDimitrov · 2026-01-23T15:00:03Z

...in/java/org/cloudfoundry/multiapps/controller/process/util/OperationInFinalStateHandler.java

        safeExecutor.execute(() -> deleteCloudControllerClientForProcess(execution));
        safeExecutor.execute(() -> setOperationState(correlationId, state));
        safeExecutor.execute(() -> deletePreviousBackupDescriptors(execution, processType, state));
+        safeExecutor.execute(() -> deleteSecretTokensForProcess(state, correlationId, execution));


Is the service instance deleted after the process finishes?

IvanBorislavovDimitrov · 2026-01-23T15:01:40Z

...in/java/org/cloudfoundry/multiapps/controller/process/util/OperationInFinalStateHandler.java

+        if (state != State.FINISHED) {
+            return;
+        }


Not needed. We want to remove the secret tokens when user aborts his operations.

IvanBorislavovDimitrov · 2026-01-23T15:02:13Z

...ontroller-process/src/main/java/org/cloudfoundry/multiapps/controller/process/Constants.java

+    public static final long TTL_CACHE_ENTRY = 60_000L;
+    public static final int MAX_CACHE_ENTRIES = 256;


IvanBorislavovDimitrov · 2026-01-23T15:03:03Z

multiapps-controller-process/src/main/java/module-info.java

+    requires org.mybatis;
+    requires java.annotation;
+    requires tools.jackson.databind;


vkalapov · 2026-01-23T13:21:27Z

.../src/main/java/org/cloudfoundry/multiapps/controller/process/steps/MergeDescriptorsStep.java

    @Inject
    private UnsupportedParameterFinder unsupportedParameterFinder;

+    private SecretParametersCollectingVisitor secretParametersCollectingVisitor = new SecretParametersCollectingVisitor();


Can this be done through a field injection like the other fields?

vkalapov · 2026-01-23T13:24:38Z

.../src/main/java/org/cloudfoundry/multiapps/controller/process/steps/SecureProcessContext.java

+        //        SecretTransformationStrategy secretTransformationStrategyContext = new SecretTransformationStrategyContextImpl(
+        //            secureParameterNames);
+        //        SecureSerializerConfiguration secureSerializerConfiguration = new SecureSerializerConfiguration();
+        //        secureSerializerConfiguration.setAdditionalSensitiveElementNames(secureParameterNames);


Remove these comments.

vkalapov · 2026-01-23T13:24:49Z

.../src/main/java/org/cloudfoundry/multiapps/controller/process/steps/SecureProcessContext.java

+    //    private SecretTransformationStrategy secretTransformationStrategy;
+
+    public SecureProcessContext(DelegateExecution execution, StepLogger stepLogger, CloudControllerClientProvider clientProvider,
+                                SecretTokenStore secretTokenStore) {
+        super(execution, stepLogger, clientProvider);
+        this.secretTokenStore = secretTokenStore;
+        //        this.secretTransformationStrategy = secretTransformationStrategy;


Remove these comments.

vkalapov · 2026-01-23T13:40:45Z

...va/org/cloudfoundry/multiapps/controller/core/security/encryption/AesEncryptionUtilTest.java

+        byte[] tampered = Arrays.copyOf(encrypted, encrypted.length);
+        tampered[tampered.length - 1] = (byte) (tampered[tampered.length - 1] ^ 0x01);
+
+        assertThrows(Exception.class, () -> AesEncryptionUtil.decrypt(tampered, KEY_FOR_256_32_BYTES));


Its better to assert the specific exception type (in this case SLException), than a generic Exception.

vkalapov · 2026-01-23T14:02:08Z

...in/java/org/cloudfoundry/multiapps/controller/process/steps/SecureProcessContextFactory.java

+    public SecureProcessContextFactory() {
+
+    }


We probably don’t need this explicit constructor, the generated builder is used for instantiation.

karrgov force-pushed the passing_secrets_during_deployment_new branch from f0f4384 to 7208ebc Compare December 19, 2025 18:10

vkalapov requested changes Jan 5, 2026

View reviewed changes

IvanBorislavovDimitrov reviewed Jan 7, 2026

View reviewed changes

.../org/cloudfoundry/multiapps/controller/process/steps/ProcessMtaExtensionDescriptorsStep.java Outdated Show resolved Hide resolved

karrgov added 2 commits January 15, 2026 11:18

Enabling using secret values during a deployment

0df70a7

LMCROSSITXSADEPLOY-2301

Fixings of comments

148d46c

LMCROSSITXSADEPLOY-2301

karrgov force-pushed the passing_secrets_during_deployment_new branch from 7208ebc to 148d46c Compare January 22, 2026 14:11

Adding the Ali cloud exclusion of bouncy castle

cb60e46

LMCROSSITXSADEPLOY-2301

IvanBorislavovDimitrov reviewed Jan 23, 2026

View reviewed changes

vkalapov requested changes Jan 23, 2026

View reviewed changes

		<include
		file="/org/cloudfoundry/multiapps/controller/persistence/db/changelog/db-changelog-TEST-SECRET-TOKEN-TABLE-ADDITION-persistence.xml"/>

		Set<String> secretParameters = VariableHandling.get(execution, Variables.SECURE_EXTENSION_DESCRIPTOR_PARAMETER_NAMES);
		DynamicSecureSerialization dynamicSecureSerialization = SecureSerializationFactory.ofAdditionalValues(secretParameters);

		@@ -0,0 +1,17 @@
		package org.cloudfoundry.multiapps.controller.process.security.resolver;

		public class MissingCredentialsFromUserProvidedServiceEncryptionRelated extends RuntimeException {

		private void getParametersAndPropertiesPerModule(MultiValuedMap<String, String> multiValuedMap, Module module, boolean isRequired,
		boolean isWhole, boolean isProvided) {

		public static final long TTL_CACHE_ENTRY = 60_000L;
		public static final int MAX_CACHE_ENTRIES = 256;

Enabling using secret values during a deployment #1755

Are you sure you want to change the base?

Enabling using secret values during a deployment #1755

Uh oh!

Conversation

karrgov commented Dec 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karrgov Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karrgov Jan 8, 2026 •

edited

Loading