Enabling using secret values during a deployment #1755
Conversation
LMCROSSITXSADEPLOY-2301
karrgov
force-pushed
the
passing_secrets_during_deployment_new
branch
from
f0f4384 to
7208ebc
Compare
|
|
||
| public DeploymentDescriptor merge(DeploymentDescriptor deploymentDescriptor, List<ExtensionDescriptor> extensionDescriptors) { | ||
| public DeploymentDescriptor merge(DeploymentDescriptor deploymentDescriptor, List<ExtensionDescriptor> extensionDescriptors, | ||
| List<String> parameterNamesToBeCensored) { |
There was a problem hiding this comment.
I think that parameterNamesToBeMasked, Hidden, Redacted, flow better here for the name.
| 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!"; |
There was a problem hiding this comment.
This message is too specific, we should avoid leaking implementation details (algorithm and the provider). Consider using a more generic message.
| <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); |
There was a problem hiding this comment.
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.
| @@ -0,0 +1,17 @@ | |||
| package org.cloudfoundry.multiapps.controller.process.security.resolver; | |||
|
|
|||
| public class MissingCredentialsFromUserProvidedServiceEncryptionRelated extends RuntimeException { | |||
There was a problem hiding this comment.
Name feels too long and missing a exception suffix. Consider shortening it.
| if (resultSet.next()) { | ||
| return resultSet.getLong(1); | ||
| } | ||
| throw new SQLException("INSERT secret_token did not return an id"); |
There was a problem hiding this comment.
Could we include the process instance and variable name in the exception message?
|
|
||
| private MultiValuedMap<String, String> getParametersNameValueMapFromExtensionDescriptors( | ||
| List<ExtensionDescriptor> extensionDescriptors) { | ||
| MultiValuedMap<String, String> parametersNameValueMapFromExtensionDescriptors = new ArrayListValuedHashMap<>(); |
There was a problem hiding this comment.
parametersNameValueMapFromExtensionDescriptors and parametersNameValueMapFromDeploymentDescriptor seems to be very long. Consider a shorter, more descriptive name to improve clarity.
| 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()); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The nested loops make the logic a bit hard to follow. Could we simplify by using streams or helper methods?
| 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(), ""))); | ||
| } |
There was a problem hiding this comment.
getParametersStringCastedValue and getPropertiesStringCastedValue share almost identical logic. Consider a generic helper method to reduce duplication.
| private void getParametersAndPropertiesPerModule(MultiValuedMap<String, String> multiValuedMap, Module module, boolean isRequired, | ||
| boolean isWhole, boolean isProvided) { |
There was a problem hiding this comment.
Long boolean flag lists reduce readability. Consider replacing them with an enum or separate helper methods for each case.
2 participants
LMCROSSITXSADEPLOY-2301