-
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?
Enabling using secret values during a deployment #1755
Conversation
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.
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.
| 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.
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.
...in/resources/org/cloudfoundry/multiapps/controller/persistence/db/changelog/db-changelog.xml
Show resolved
Hide resolved
| Set<String> secretParameters = VariableHandling.get(execution, Variables.SECURE_EXTENSION_DESCRIPTOR_PARAMETER_NAMES); | ||
| DynamicSecureSerialization dynamicSecureSerialization = SecureSerializationFactory.ofAdditionalValues(secretParameters); |
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.
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.
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.
Fixed on all places, thanks :)
| @@ -0,0 +1,17 @@ | |||
| package org.cloudfoundry.multiapps.controller.process.security.resolver; | |||
|
|
|||
| public class MissingCredentialsFromUserProvidedServiceEncryptionRelated extends RuntimeException { | |||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parametersNameValueMapFromExtensionDescriptors and parametersNameValueMapFromDeploymentDescriptor seems to be very long. Consider a shorter, more descriptive name to improve clarity.
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.
Fixed, since I have changed the logic to follow the Visitor pattern :)
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nested loops make the logic a bit hard to follow. Could we simplify by using streams or helper methods?
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.
Fixed, since I have changed the logic to follow the Visitor pattern :)
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getParametersStringCastedValue and getPropertiesStringCastedValue share almost identical logic. Consider a generic helper method to reduce duplication.
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.
Fixed, since I have changed the logic to follow the Visitor pattern :)
| private void getParametersAndPropertiesPerModule(MultiValuedMap<String, String> multiValuedMap, Module module, boolean isRequired, | ||
| boolean isWhole, boolean isProvided) { |
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.
Long boolean flag lists reduce readability. Consider replacing them with an enum or separate helper methods for each case.
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.
Fixed, since I have changed the logic to follow the Visitor pattern :)
...n/java/org/cloudfoundry/multiapps/controller/core/security/encryption/AesEncryptionUtil.java
Outdated
Show resolved
Hide resolved
.../cloudfoundry/multiapps/controller/process/security/resolver/SecretTokenKeyResolverImpl.java
Outdated
Show resolved
Hide resolved
...n/java/org/cloudfoundry/multiapps/controller/core/security/encryption/AesEncryptionUtil.java
Outdated
Show resolved
Hide resolved
...ess/src/main/java/org/cloudfoundry/multiapps/controller/process/jobs/SecretTokenCleaner.java
Outdated
Show resolved
Hide resolved
...ess/src/main/java/org/cloudfoundry/multiapps/controller/process/jobs/SecretTokenCleaner.java
Outdated
Show resolved
Hide resolved
...udfoundry/multiapps/controller/process/security/SecretTransformationStrategyContextImpl.java
Outdated
Show resolved
Hide resolved
...ntroller/process/security/resolver/MissingUserProvidedServiceEncryptionRelatedException.java
Outdated
Show resolved
Hide resolved
.../cloudfoundry/multiapps/controller/process/security/resolver/SecretTokenKeyResolverImpl.java
Outdated
Show resolved
Hide resolved
.../org/cloudfoundry/multiapps/controller/process/steps/ProcessMtaExtensionDescriptorsStep.java
Outdated
Show resolved
Hide resolved
7208ebc to
148d46c
Compare
...dfoundry/multiapps/controller/core/security/serialization/SecureSerializerConfiguration.java
Show resolved
Hide resolved
| 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}\""; |
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.
not used, delete it
...in/resources/org/cloudfoundry/multiapps/controller/persistence/db/changelog/db-changelog.xml
Show resolved
Hide resolved
.../cloudfoundry/multiapps/controller/process/security/resolver/SecretTokenKeyResolverImpl.java
Outdated
Show resolved
Hide resolved
.../cloudfoundry/multiapps/controller/process/security/resolver/SecretTokenKeyResolverImpl.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/cloudfoundry/multiapps/controller/process/steps/SecureProcessContext.java
Outdated
Show resolved
Hide resolved
...in/java/org/cloudfoundry/multiapps/controller/process/util/OperationInFinalStateHandler.java
Show resolved
Hide resolved
...in/java/org/cloudfoundry/multiapps/controller/process/util/OperationInFinalStateHandler.java
Outdated
Show resolved
Hide resolved
...ontroller-process/src/main/java/org/cloudfoundry/multiapps/controller/process/Constants.java
Show resolved
Hide resolved
| @Inject | ||
| private UnsupportedParameterFinder unsupportedParameterFinder; | ||
|
|
||
| private SecretParametersCollectingVisitor secretParametersCollectingVisitor = new SecretParametersCollectingVisitor(); |
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.
Can this be done through a field injection like the other fields?
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.
fixed, thanks :)
| // SecretTransformationStrategy secretTransformationStrategyContext = new SecretTransformationStrategyContextImpl( | ||
| // secureParameterNames); | ||
| // SecureSerializerConfiguration secureSerializerConfiguration = new SecureSerializerConfiguration(); | ||
| // secureSerializerConfiguration.setAdditionalSensitiveElementNames(secureParameterNames); |
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.
Remove these comments.
| // private SecretTransformationStrategy secretTransformationStrategy; | ||
|
|
||
| public SecureProcessContext(DelegateExecution execution, StepLogger stepLogger, CloudControllerClientProvider clientProvider, | ||
| SecretTokenStore secretTokenStore) { | ||
| super(execution, stepLogger, clientProvider); | ||
| this.secretTokenStore = secretTokenStore; | ||
| // this.secretTransformationStrategy = secretTransformationStrategy; |
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.
Remove these comments.
| 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)); |
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.
Its better to assert the specific exception type (in this case SLException), than a generic Exception.
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.
fixed, thanks :)
| public SecureProcessContextFactory() { | ||
|
|
||
| } |
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.
We probably don’t need this explicit constructor, the generated builder is used for instantiation.
| boolean isSecureExtensionDescriptorPresent = extensionDescriptors.stream() | ||
| .anyMatch(extensionDescriptor -> extensionDescriptor.getId() | ||
| .equals( | ||
| Constants.SECURE_EXTENSION_DESCRIPTOR_ID)); | ||
| if (isSecureExtensionDescriptorPresent) { | ||
| List<ExtensionDescriptor> extensionDescriptorsWithoutSecure = extensionDescriptors.stream() | ||
| .filter( | ||
| extensionDescriptor -> !extensionDescriptor.getId() | ||
| .equals( | ||
| Constants.SECURE_EXTENSION_DESCRIPTOR_ID)) | ||
| .toList(); | ||
| getStepLogger().debug(Messages.PROVIDED_EXTENSION_DESCRIPTORS, SecureSerialization.toJson(extensionDescriptorsWithoutSecure) | ||
| + Messages.SECURE_EXTENSION_DESCRIPTOR_CONSTRUCTED_AND_APPLIED_FROM_ENVIRONMENT_VARIABLES); | ||
| } else { | ||
| getStepLogger().debug(Messages.PROVIDED_EXTENSION_DESCRIPTORS, SecureSerialization.toJson(extensionDescriptors)); | ||
| } |
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.
The check against Constants.SECURE_EXTENSION_DESCRIPTOR_ID is duplicated in anyMatch and filter. We could compute the filtered list once and infer presence from the size difference:
boolean securePresent = extensionDescriptorsWithoutSecure.size() != extensionDescriptors.size();
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.
thanks for the suggestion, fixed :)
| private static Cipher setUpCipherObject(byte[] encryptionKey, byte[] gcmInitialisationVector, boolean shouldEncrypt) | ||
| throws InvalidAlgorithmParameterException, InvalidKeyException, NoSuchPaddingException, NoSuchAlgorithmException, | ||
| NoSuchProviderException { | ||
| Cipher cipherObject = Cipher.getInstance(Constants.CIPHER_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); | ||
|
|
||
| if (shouldEncrypt) { | ||
| cipherObject.init(Cipher.ENCRYPT_MODE, secretKeySpec, gcmParameterSpec); | ||
| } else { | ||
| cipherObject.init(Cipher.DECRYPT_MODE, secretKeySpec, gcmParameterSpec); | ||
| } | ||
|
|
||
| return cipherObject; | ||
| } |
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.
Why not pass Cipher.ENCRYPT_MODE / Cipher.DECRYPT_MODE instead of a boolean shouldEncrypt in the method signature? It would make the call sites more explicit.
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.
fixed, thanks for the suggestion
| String namespace = VariableHandling.get(execution, Variables.MTA_NAMESPACE); | ||
|
|
||
| if (mtaId == null) { | ||
| throw new SLException("Missing mtaId in encryption key resolver! Cannot continue from here!"); |
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.
Extract this message in a constant.
| @Inject | ||
| public SecretTokenKeyResolverImpl(CloudControllerClientProvider cloudControllerClientProvider) { | ||
| this.cloudControllerClientProvider = cloudControllerClientProvider; | ||
| } | ||
|
|
||
| public SecretTokenKeyResolverImpl() { | ||
| } |
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.
Is there a reason this class has both an @Inject constructor and a no-args constructor? With the default constructor present, the dependency could be left uninitialized.
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.
Accidentally must have left the no-args one, fixed, thanks :)
| SecretTokenKeyContainer secretTokenKeyContainer = null; | ||
| if (serviceInstanceID != null) { | ||
| secretTokenKeyContainer = cachedMap.get(serviceInstanceID.toString()); | ||
| } | ||
| if (secretTokenKeyContainer != null) { | ||
| return secretTokenKeyContainer; | ||
| } |
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.
The cache lookup could be simplified using an early return:
if (serviceInstanceID != null) {
SecretTokenKeyContainer secretTokenKeyContainer = cachedMap.get(serviceInstanceID.toString());
if (secretTokenKeyContainer != null) return secretTokenKeyContainer;
}
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.
Fixed :)
LMCROSSITXSADEPLOY-2301
LMCROSSITXSADEPLOY-2301
LMCROSSITXSADEPLOY-2301
LMCROSSITXSADEPLOY-2301
cb60e46 to
5fa31a4
Compare
LMCROSSITXSADEPLOY-2301
| @@ -22,14 +26,17 @@ | |||
| requires org.cloudfoundry.client; | |||
| requires com.fasterxml.jackson.core; | |||
| requires com.fasterxml.jackson.databind; | |||
| requires com.google.common; | |||
| requires flowable.bpmn.model; | |||
| requires flowable.engine.common; | |||
| requires flowable.engine.common.api; | |||
| requires flowable.job.service; | |||
| requires flowable.job.service.api; | |||
| requires flowable.variable.service; | |||
| requires flowable.variable.service.api; | |||
| requires jakarta.annotation; | |||
| requires jakarta.persistence; | |||
| requires java.annotation; | |||
| requires java.sql; | |||
| requires jakarta.xml.bind; | |||
| requires jakarta.inject; | |||
| @@ -39,23 +46,29 @@ | |||
| requires org.apache.commons.collections4; | |||
| requires org.apache.commons.io; | |||
| requires org.apache.commons.lang3; | |||
| requires org.bouncycastle.fips.core; | |||
| requires org.cloudfoundry.multiapps.common; | |||
| requires org.cloudfoundry.multiapps.controller.client; | |||
| requires org.cloudfoundry.multiapps.controller.persistence; | |||
| requires org.cloudfoundry.multiapps.mta; | |||
| requires org.joda.time; | |||
| requires org.mybatis; | |||
| requires org.slf4j; | |||
| requires spring.beans; | |||
| requires spring.context; | |||
| requires spring.core; | |||
| requires spring.security.oauth2.core; | |||
| requires spring.web; | |||
| requires tools.jackson.databind; | |||
| requires reactor.netty; | |||
| requires io.netty.handler; | |||
| requires io.netty.transport; | |||
|
|
|||
| requires static java.compiler; | |||
| requires static org.immutables.value; | |||
| requires org.cloudfoundry.multiapps.controller.shutdown.client; | |||
| requires org.checkerframework.checker.qual; | |||
| requires s3; | |||
| requires org.eclipse.persistence.jpa; | |||
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.
Do we need all of the newly added module dependencies, or can any of them be removed?
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.
Tested without them, everything is fine, thanks :)
| String plainText = "top secret"; | ||
| byte[] encrypted = AesEncryptionUtil.encrypt(plainText, KEY_FOR_256_32_BYTES); | ||
|
|
||
| byte[] differentValidKey = "0123456789abcdef0123456789ABCDEF".getBytes(StandardCharsets.UTF_8); |
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.
Consider extracting this into a constant.
| .map(element -> { | ||
| if (element instanceof String) { | ||
| String transformedJson = transformJson((String) element, censor); | ||
| return getResultFromTransformedJson(transformedJson, element, false); | ||
| } else if (element instanceof byte[]) { | ||
| String transformedJson = transformJson(new String((byte[]) element), censor); | ||
| return getResultFromTransformedJson(transformedJson, element, true); | ||
| } | ||
| return element; | ||
| }) |
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.
I think extracting the lambda logic would improve readability.
| return OBJECT_MAPPER.writeValueAsString(output); | ||
| } | ||
| return null; | ||
| } catch (Exception e) { |
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.
Do we need to catch generic exceptions or is it possible that we can narrow down into more JSON specific ones?
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.
Fixed, thanks :) Added only JsonProcessingException since JsonMappingException is a child exception of JsonProcessingException
| return objectNode; | ||
| } | ||
|
|
||
| private void determineWhetherToCensorOrUncensor(JsonNode childNode, JsonNode processedNode, ObjectNode objectNode, String currentField, |
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.
Is censor/uncensor the most appropriate terminology for this case? Could something like encode/decode better reflect what’s happening?
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.
Fixed, thanks for the suggestion!
| for (String secretParameter : secretParameters) { | ||
| if (!secretParameter.isEmpty() && value.contains(secretParameter)) { | ||
| nestedParameters.add(currentParameterName); | ||
| } | ||
| } |
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.
Once we add currentParameterName, would it make sense to return early?
| } | ||
| } | ||
|
|
||
| private void shouldBeAddedInNestedParameters(Set<String> secretParameters, Set<String> nestedParameters, String value, |
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.
secretParameters is already a class field, can the method just use the field directly?
| @Component | ||
| public class SecretParametersCollector extends Visitor { | ||
|
|
||
| private final Set<String> secretParameters = new HashSet<>(); | ||
|
|
||
| private final MultiValuedMap<String, String> parametersNameValueMap = new ArrayListValuedHashMap<>(); |
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.
Since this class uses @Component , should these fields be made method local or thread safe?
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.
Fixed it by making it not a bean and adding it as a local object inside the method it is used instead of a field for the MergeDescriptorsStep
| } | ||
|
|
||
| @Override | ||
| public String resolve(DelegateExecution execution) { |
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.
This method can be broken down into smaller chunks.
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.
Fixed :)
| LOGGER.error("SecretTokenKeyResolver CACHE_MISSED instance={} correlationId={}", instance(), correlationId); | ||
|
|
||
| CloudControllerClient cloudControllerClient = createCloudControllerClient(execution, correlationId); | ||
| LOGGER.error("SecretTokenKeyResolver CLIENT_CREATED instance={} correlationId={} userGuid={} spaceGuid={}", | ||
| instance(), correlationId, StepsUtil.determineCurrentUserGuid(execution), | ||
| VariableHandling.get(execution, Variables.SPACE_GUID)); | ||
|
|
||
| String userProvidedServiceName = getUserProvidedServiceName(execution); | ||
| LOGGER.error("SecretTokenKeyResolver UPS_NAME_CHOSEN instance={} correlationId={} upsName='{}'", | ||
| instance(), correlationId, userProvidedServiceName); | ||
|
|
||
| CloudServiceInstance cloudServiceInstance = cloudControllerClient.getServiceInstance(userProvidedServiceName); | ||
| if (cloudServiceInstance == null) { | ||
| LOGGER.error("SecretTokenKeyResolver UPS_NOT_FOUND instance={} correlationId={} upsName='{}'", | ||
| instance(), correlationId, userProvidedServiceName); | ||
| throw new SLException(Messages.COULD_NOT_RETRIEVE_USER_PROVIDED_SERVICE_INSTANCE_ENCRYPTION_RELATED); | ||
| } | ||
|
|
||
| UUID upsGuid = cloudServiceInstance.getGuid(); | ||
| LOGGER.error("SecretTokenKeyResolver UPS_RESOLVED instance={} correlationId={} upsName='{}' guid={}", | ||
| instance(), correlationId, userProvidedServiceName, upsGuid); | ||
|
|
||
| Map<String, Object> serviceInstanceCredentials = getUserProvidedServiceInstanceParameters(cloudServiceInstance, | ||
| cloudControllerClient); | ||
| if (serviceInstanceCredentials == null || serviceInstanceCredentials.isEmpty()) { | ||
| LOGGER.error("SecretTokenKeyResolver CREDENTIALS_EMPTY instance={} correlationId={} guid={}", instance(), correlationId, | ||
| upsGuid); |
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.
Don't forget to remove these debugging logs.
LMCROSSITXSADEPLOY-2301
LMCROSSITXSADEPLOY-2301
LMCROSSITXSADEPLOY-2301
|
|
||
| private final CloudControllerClientProvider cloudControllerClientProvider; | ||
|
|
||
| private final Duration containerExpirationTime = Duration.ofMinutes(Constants.TTL_CACHE_ENTRY); |
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.
Fix expiration time or the duration here.
| secretTokenService.createQuery() | ||
| .processInstanceId(processInstanceId) | ||
| .delete(); | ||
| LOGGER.debug(MessageFormat.format(Messages.DELETED_SECRET_TOKENS_FOR_PROCESS_WITH_ID_0, processInstanceId)); |
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.
track how many rows were affected
| } | ||
|
|
||
| @Override | ||
| public void deleteByProcessInstanceId(String processInstanceId) { |
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.
return int
| @Override | ||
| public void deleteByProcessInstanceId(String processInstanceId) { | ||
| secretTokenService.createQuery() | ||
| .processInstanceId(processInstanceId) | ||
| .delete(); | ||
| LOGGER.debug(MessageFormat.format(Messages.DELETED_SECRET_TOKENS_FOR_PROCESS_WITH_ID_0, processInstanceId)); | ||
| } | ||
|
|
||
| @Override | ||
| public int deleteOlderThan(LocalDateTime expirationTime) { | ||
| int result = secretTokenService.createQuery() | ||
| .olderThan(expirationTime) | ||
| .delete(); | ||
| LOGGER.debug(MessageFormat.format(Messages.DELETED_SECRET_TOKENS_WITH_EXPIRATION_DATE_0, expirationTime)); | ||
| return result; | ||
| } |
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.
can you reuse the methods?
Rename SecretTokenStoreImplWithoutKey -> SecretTokenStoreForDeletion
SecretTokenStoreImpl extends SecretTokenStoreForDeletion
|
|
||
| @Override | ||
| public Object serialize(T value) { | ||
| Object encodedObject = serializer.serialize(value); |
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.
rename to serializedObject because it is still not encoded
| import org.immutables.value.Value; | ||
|
|
||
| @Value.Immutable | ||
| public abstract class SecureProcessContextFactory { |
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.
use interface with a default method for ofSecureProcessContext
| Variables.DISPOSABLE_USER_PROVIDED_SERVICE_NAME); | ||
| clientProvider.getControllerClient(userGuid, spaceGuid, correlationId) | ||
| .deleteServiceInstance(disposableUserProvidedServiceInstanceName); | ||
| LOGGER.debug( |
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.
use info
|
|
||
| } | ||
|
|
||
| private void deleteSecretTokensForProcess(Operation.State state, String correlationId, DelegateExecution execution) { |
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.
execution not used
| public static final Long UNSET_LAST_LOG_TIMESTAMP_MS = 0L; | ||
| public static final int LOG_STALLED_TASK_MINUTE_INTERVAL = 5; | ||
| public static final long TTL_CACHE_ENTRY = 60_000L; | ||
| public static final int MAX_CACHE_ENTRIES = 256; |
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.
not used?
| private MtaDescriptorPropertiesResolver resolver; | ||
| @Mock | ||
| private ModuleToDeployHelper moduleToDeployHelper; | ||
| // @Mock |
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.
Remove?
LMCROSSITXSADEPLOY-2301