Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions multiapps-controller-core/src/main/java/module-info.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
exports org.cloudfoundry.multiapps.controller.core.validators.parameters;
exports org.cloudfoundry.multiapps.controller.core.validators.parameters.v2;
exports org.cloudfoundry.multiapps.controller.core.validators.parameters.v3;
exports org.cloudfoundry.multiapps.controller.core.security.encryption;

requires transitive jakarta.persistence;
requires transitive org.cloudfoundry.multiapps.controller.client;
Expand Down Expand Up @@ -80,5 +81,8 @@
requires static java.compiler;
requires static org.immutables.value;
requires spring.security.oauth2.client;
requires java.desktop;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this used for?

requires io.netty.common;
requires org.bouncycastle.fips.core;
Comment on lines +85 to +86
Copy link
Contributor

Choose a reason for hiding this comment

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

order them ASC


}
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,14 @@ public class Constants {

public static final String B3_TRACE_ID_HEADER = "X-B3-TraceId";
public static final String B3_SPAN_ID_HEADER = "X-B3-SpanId";
public static final String CYPHER_TRANSFORMATION_NAME = "AES/GCM/NoPadding";
public static final String ENCRYPTION_DECRYPTION_ALGORITHM_NAME = "AES";

public static final int TOKEN_SERVICE_DELETION_CORE_POOL_SIZE = 1;
public static final int TOKEN_SERVICE_DELETION_MAXIMUM_POOL_SIZE = 3;
public static final int TOKEN_SERVICE_DELETION_KEEP_ALIVE_THREAD_IN_SECONDS = 30;
public static final int INITIALISATION_VECTOR_LENGTH = 12;
public static final int GCM_AUTHENTICATION_TAG_LENGTH = 128;
Comment on lines +38 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add some comments why exactly these values were chosen


public static final String APP_FEATURE_SSH = "ssh";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ public final class Messages {
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!";
Copy link
Contributor

Choose a reason for hiding this comment

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

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

public static final String DECRYPTION_BOUNCY_CASTLE_AES256_HAS_FAILED = "Decryption with AES256 by Bouncy Castle has failed!";

// Warning messages
public static final String ENVIRONMENT_VARIABLE_IS_NOT_SET_USING_DEFAULT = "Environment variable \"{0}\" is not set. Using default \"{1}\"...";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@

import org.cloudfoundry.multiapps.controller.core.Messages;
import org.cloudfoundry.multiapps.controller.core.cf.CloudHandlerFactory;
import org.cloudfoundry.multiapps.controller.core.security.serialization.SecureSerialization;
import org.cloudfoundry.multiapps.controller.core.security.serialization.DynamicSecureSerialization;
import org.cloudfoundry.multiapps.controller.core.security.serialization.SecureSerializationFactory;
import org.cloudfoundry.multiapps.controller.core.util.UserMessageLogger;
import org.cloudfoundry.multiapps.mta.handlers.v2.DescriptorMerger;
import org.cloudfoundry.multiapps.mta.handlers.v2.DescriptorValidator;
Expand All @@ -28,11 +29,13 @@ public MtaDescriptorMerger(CloudHandlerFactory handlerFactory, Platform platform
this.userMessageLogger = userMessageLogger;
}

public DeploymentDescriptor merge(DeploymentDescriptor deploymentDescriptor, List<ExtensionDescriptor> extensionDescriptors) {
public DeploymentDescriptor merge(DeploymentDescriptor deploymentDescriptor, List<ExtensionDescriptor> extensionDescriptors,
List<String> parameterNamesToBeCensored) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

DescriptorValidator validator = handlerFactory.getDescriptorValidator();
validator.validateDeploymentDescriptor(deploymentDescriptor, platform);
validator.validateExtensionDescriptors(extensionDescriptors, deploymentDescriptor);

DynamicSecureSerialization dynamicSecureSerialization = SecureSerializationFactory.ofAdditionalValues(parameterNamesToBeCensored);
DescriptorMerger merger = handlerFactory.getDescriptorMerger();

// Merge the passed set of descriptors into one deployment descriptor. The deployment descriptor at the root of
Expand All @@ -45,7 +48,7 @@ public DeploymentDescriptor merge(DeploymentDescriptor deploymentDescriptor, Lis

deploymentDescriptor = handlerFactory.getDescriptorParametersCompatibilityValidator(mergedDescriptor, userMessageLogger)
.validate();
logDebug(Messages.MERGED_DESCRIPTOR, SecureSerialization.toJson(deploymentDescriptor));
logDebug(Messages.MERGED_DESCRIPTOR, dynamicSecureSerialization.toJson(deploymentDescriptor));

return deploymentDescriptor;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package org.cloudfoundry.multiapps.controller.core.security.encryption;

public class AESDecryptionException extends RuntimeException {

public AESDecryptionException(String message) {
super(message);
}

public AESDecryptionException(String message, Throwable cause) {
super(message, cause);
}

public AESDecryptionException(Throwable cause) {
super(cause);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package org.cloudfoundry.multiapps.controller.core.security.encryption;

public class AESEncryptionException extends RuntimeException {

public AESEncryptionException(String message) {
super(message);
}

public AESEncryptionException(String message, Throwable cause) {
super(message, cause);
}

public AESEncryptionException(Throwable cause) {
super(cause);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package org.cloudfoundry.multiapps.controller.core.security.encryption;

import java.nio.charset.StandardCharsets;
import java.security.SecureRandom;
import javax.crypto.Cipher;
import javax.crypto.spec.GCMParameterSpec;
import javax.crypto.spec.SecretKeySpec;

import org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider;
import org.cloudfoundry.multiapps.controller.core.Constants;
import org.cloudfoundry.multiapps.controller.core.Messages;

public class AesEncryptionUtil {

public static byte[] encrypt(String plainText, byte[] encryptionKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor spelling issue: the variable name uses “Cypher” instead of “Cipher”. Consider updating the variable names to use “Cipher” for clarity and consistency.

try {
byte[] gcmInitialisationVector = new byte[Constants.INITIALISATION_VECTOR_LENGTH];
new SecureRandom().nextBytes(gcmInitialisationVector);

Cipher cipherObject = Cipher.getInstance(Constants.CYPHER_TRANSFORMATION_NAME, BouncyCastleFipsProvider.PROVIDER_NAME);
SecretKeySpec secretKeySpec = new SecretKeySpec(encryptionKey, Constants.ENCRYPTION_DECRYPTION_ALGORITHM_NAME);
GCMParameterSpec gcmParameterSpec = new GCMParameterSpec(Constants.GCM_AUTHENTICATION_TAG_LENGTH, gcmInitialisationVector);

cipherObject.init(Cipher.ENCRYPT_MODE, secretKeySpec, gcmParameterSpec);
Comment on lines +20 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

The cipher initialization logic in encrypt() and decrypt() is repeated: creating the cipher instance, SecretKeySpec, and GCMParameterSpec. Consider extracting a private helper method to reduce duplication.


byte[] cipherValue = cipherObject.doFinal(plainText.getBytes(StandardCharsets.UTF_8));

byte[] combinedCypherValueAndInitialisationVector = new byte[gcmInitialisationVector.length + cipherValue.length];

System.arraycopy(gcmInitialisationVector, 0, combinedCypherValueAndInitialisationVector, 0, gcmInitialisationVector.length);
System.arraycopy(cipherValue, 0, combinedCypherValueAndInitialisationVector, gcmInitialisationVector.length,
cipherValue.length);

return combinedCypherValueAndInitialisationVector;
} catch (Exception e) {
throw new AESEncryptionException(Messages.ENCRYPTION_BOUNCY_CASTLE_AES256_HAS_FAILED
+ e.getMessage(), e);
Comment on lines +36 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

Use MessageFormat for better formatting of the error message

}
}

public static String decrypt(byte[] encryptedValue, byte[] encryptionKey) {
try {
byte[] gcmInitialisationVector = new byte[Constants.INITIALISATION_VECTOR_LENGTH];
System.arraycopy(encryptedValue, 0, gcmInitialisationVector, 0,
gcmInitialisationVector.length);

byte[] cipherValue = new byte[encryptedValue.length - 12];
System.arraycopy(encryptedValue, 12, cipherValue, 0, cipherValue.length);
Comment on lines +47 to +48
Copy link
Contributor

Choose a reason for hiding this comment

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

Extract the hardcoded 12 in a constant.


Cipher cipherObject = Cipher.getInstance(Constants.CYPHER_TRANSFORMATION_NAME, BouncyCastleFipsProvider.PROVIDER_NAME);
SecretKeySpec secretKeySpec = new SecretKeySpec(encryptionKey, Constants.ENCRYPTION_DECRYPTION_ALGORITHM_NAME);
GCMParameterSpec gcmParameterSpec = new GCMParameterSpec(Constants.GCM_AUTHENTICATION_TAG_LENGTH, gcmInitialisationVector);
cipherObject.init(Cipher.DECRYPT_MODE, secretKeySpec, gcmParameterSpec);

byte[] resultInBytes = cipherObject.doFinal(cipherValue);
return new String(resultInBytes, StandardCharsets.UTF_8);
} catch (Exception e) {
throw new AESDecryptionException(Messages.DECRYPTION_BOUNCY_CASTLE_AES256_HAS_FAILED
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

+ e.getMessage(), e);
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package org.cloudfoundry.multiapps.controller.core.security.serialization;

import org.cloudfoundry.multiapps.controller.core.security.serialization.model.DeploymentDescriptorSerializer;
import org.cloudfoundry.multiapps.controller.core.security.serialization.model.ModuleSerializer;
import org.cloudfoundry.multiapps.controller.core.security.serialization.model.ProvidedDependencySerializer;
import org.cloudfoundry.multiapps.controller.core.security.serialization.model.RequiredDependencySerializer;
import org.cloudfoundry.multiapps.controller.core.security.serialization.model.ResourceSerializer;
import org.cloudfoundry.multiapps.mta.model.DeploymentDescriptor;
import org.cloudfoundry.multiapps.mta.model.Module;
import org.cloudfoundry.multiapps.mta.model.ProvidedDependency;
import org.cloudfoundry.multiapps.mta.model.RequiredDependency;
import org.cloudfoundry.multiapps.mta.model.Resource;
import org.cloudfoundry.multiapps.mta.model.VersionedEntity;

public final class DynamicSecureSerialization {
Copy link
Contributor

Choose a reason for hiding this comment

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

There still a few places where is SecureSerialization is used. For example in ProcessDescriptorStep
getStepLogger().debug(Messages.SUBSCRIPTIONS, SecureSerialization.toJson(subscriptions));
Can this print some variables that must be secret?


private final SecureSerializerConfiguration secureSerializerConfiguration;

DynamicSecureSerialization(SecureSerializerConfiguration secureSerializerConfiguration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why no modifier?

this.secureSerializerConfiguration = secureSerializerConfiguration;
}

public String toJson(Object object) {
SecureJsonSerializer secureJsonSerializer = createDynamicJsonSerializer(object, secureSerializerConfiguration);
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you pass this parameter everywhere when you have it as a field?

return secureJsonSerializer.serialize(object);
}

private static SecureJsonSerializer createDynamicJsonSerializer(Object object,
SecureSerializerConfiguration secureSerializerConfiguration) {
SecureJsonSerializer secureJsonSerializer = createDynamicJsonSerializerForVersionedEntity(object, secureSerializerConfiguration);
if (secureJsonSerializer == null) {
return new SecureJsonSerializer(secureSerializerConfiguration);
}

return secureJsonSerializer;
}

private static SecureJsonSerializer createDynamicJsonSerializerForVersionedEntity(Object object,
SecureSerializerConfiguration secureSerializerConfiguration) {
if (object instanceof VersionedEntity) {
return createDynamicJsonSerializerForVersionedEntity((VersionedEntity) object, secureSerializerConfiguration);
}

return null;
}

private static SecureJsonSerializer createDynamicJsonSerializerForVersionedEntity(VersionedEntity versionedEntity,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hooks with secret values were printed in Operation log. Maybe should be addressed here?

SecureSerializerConfiguration secureSerializerConfiguration) {
if (versionedEntity.getMajorSchemaVersion() < 3) {
return null;
}

if (versionedEntity instanceof DeploymentDescriptor) {
return new DeploymentDescriptorSerializer(secureSerializerConfiguration);
}

if (versionedEntity instanceof Module) {
return new ModuleSerializer(secureSerializerConfiguration);
}

if (versionedEntity instanceof ProvidedDependency) {
return new ProvidedDependencySerializer(secureSerializerConfiguration);
}

if (versionedEntity instanceof RequiredDependency) {
return new RequiredDependencySerializer(secureSerializerConfiguration);
}

if (versionedEntity instanceof Resource) {
return new ResourceSerializer(secureSerializerConfiguration);
}

return null;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package org.cloudfoundry.multiapps.controller.core.security.serialization;

import java.util.Collection;

public final class SecureSerializationFactory {

private SecureSerializationFactory() {

}

public static DynamicSecureSerialization ofAdditionalValues(Collection<String> additionalSensitiveElementNames) {
SecureSerializerConfiguration secureSerializerConfigurationWithAdditionalValues = new SecureSerializerConfiguration();

secureSerializerConfigurationWithAdditionalValues.setAdditionalSensitiveElementNames(additionalSensitiveElementNames);
return new DynamicSecureSerialization(secureSerializerConfigurationWithAdditionalValues);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.util.Collection;
import java.util.Collections;
import java.util.LinkedList;
import java.util.List;

import org.apache.commons.lang3.StringUtils;
Expand All @@ -17,8 +18,25 @@ public class SecureSerializerConfiguration {
private Collection<String> sensitiveElementNames = DEFAULT_SENSITIVE_NAMES;
private Collection<String> sensitiveElementPaths = Collections.emptyList();

private Collection<String> additionalSensitiveElementNames = Collections.emptyList();

public Collection<String> getSensitiveElementNames() {
return sensitiveElementNames;
if (additionalSensitiveElementNames == null || additionalSensitiveElementNames.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is null when the feature is not used?

return sensitiveElementNames;
}

List<String> mergedSensitiveElementNames = new LinkedList<>(sensitiveElementNames);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why linked list?


for (String currentAdditionalSensitiveElement : additionalSensitiveElementNames) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that renaming the variable additionalSensitiveElement can improve readability.

boolean isExistentAlready = mergedSensitiveElementNames.stream()
.anyMatch(sensitiveElement -> sensitiveElement.equalsIgnoreCase(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using anyMatch(...) and negating the result, consider using noneMatch(...) directly to avoid the extra logical inversion.

currentAdditionalSensitiveElement));
if (!isExistentAlready) {
mergedSensitiveElementNames.add(currentAdditionalSensitiveElement);
}
}
Comment on lines +34 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't using HashSet make the method easier? No duplicates will be added


return mergedSensitiveElementNames;
}

public Collection<String> getSensitiveElementPaths() {
Expand All @@ -33,6 +51,10 @@ public void setSensitiveElementNames(Collection<String> sensitiveElementNames) {
this.sensitiveElementNames = sensitiveElementNames;
}

public void setAdditionalSensitiveElementNames(Collection<String> additionalSensitiveElementNames) {
this.additionalSensitiveElementNames = additionalSensitiveElementNames;
}

public void setSensitiveElementPaths(Collection<String> sensitiveElementPaths) {
this.sensitiveElementPaths = sensitiveElementPaths;
}
Expand Down
Loading
Loading