-
Notifications
You must be signed in to change notification settings - Fork 42
Enabling using secret values during a deployment #1755
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -80,5 +81,8 @@ | |
| requires static java.compiler; | ||
| requires static org.immutables.value; | ||
| requires spring.security.oauth2.client; | ||
| requires java.desktop; | ||
| requires io.netty.common; | ||
| requires org.bouncycastle.fips.core; | ||
|
Comment on lines
+85
to
+86
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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!"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}\"..."; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that |
||
| 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 | ||
|
|
@@ -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; | ||
| } | ||
|
|
||
| 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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cipher initialization logic in |
||
|
|
||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| private final SecureSerializerConfiguration secureSerializerConfiguration; | ||
|
|
||
| DynamicSecureSerialization(SecureSerializerConfiguration secureSerializerConfiguration) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why linked list? |
||
|
|
||
| for (String currentAdditionalSensitiveElement : additionalSensitiveElementNames) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that renaming the variable |
||
| boolean isExistentAlready = mergedSensitiveElementNames.stream() | ||
| .anyMatch(sensitiveElement -> sensitiveElement.equalsIgnoreCase( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using |
||
| currentAdditionalSensitiveElement)); | ||
| if (!isExistentAlready) { | ||
| mergedSensitiveElementNames.add(currentAdditionalSensitiveElement); | ||
| } | ||
| } | ||
|
Comment on lines
+34
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() { | ||
|
|
@@ -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; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
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?