From c19575b978010c8e8cf8dcf67c75559b04e036ad Mon Sep 17 00:00:00 2001 From: Tim Ysewyn Date: Fri, 26 Jul 2019 12:13:51 +0200 Subject: [PATCH 1/2] Added optimizations to EnvironmentDecryptApplicationInitializer --- ...ironmentDecryptApplicationInitializer.java | 140 ++++++++++++------ ...entDecryptApplicationInitializerTests.java | 96 ++++++++++-- 2 files changed, 179 insertions(+), 57 deletions(-) diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java b/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java index 430bedab..100b7c44 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java @@ -63,6 +63,11 @@ public class EnvironmentDecryptApplicationInitializer implements */ public static final String DECRYPTED_BOOTSTRAP_PROPERTY_SOURCE_NAME = "decryptedBootstrap"; + /** + * Prefix indicating an encrypted value. + */ + public static final String ENCRYPTED_PROPERTY_PREFIX = "{cipher}"; + private static final Pattern COLLECTION_PROPERTY = Pattern .compile("(\\S+)?\\[(\\d+)\\](\\.\\S+)?"); @@ -98,11 +103,24 @@ public class EnvironmentDecryptApplicationInitializer implements @Override public void initialize(ConfigurableApplicationContext applicationContext) { - ConfigurableEnvironment environment = applicationContext.getEnvironment(); MutablePropertySources propertySources = environment.getPropertySources(); Set found = new LinkedHashSet<>(); + if (!propertySources.contains(DECRYPTED_BOOTSTRAP_PROPERTY_SOURCE_NAME)) { + // No reason to decrypt bootstrap twice + PropertySource bootstrap = propertySources + .get(BootstrapApplicationListener.BOOTSTRAP_PROPERTY_SOURCE_NAME); + if (bootstrap != null) { + Map map = decrypt(bootstrap); + if (!map.isEmpty()) { + found.addAll(map.keySet()); + insert(applicationContext, new SystemEnvironmentPropertySource( + DECRYPTED_BOOTSTRAP_PROPERTY_SOURCE_NAME, map)); + } + } + } + removeDecryptedProperties(applicationContext); Map map = decrypt(propertySources); if (!map.isEmpty()) { // We have some decrypted properties @@ -110,16 +128,6 @@ public class EnvironmentDecryptApplicationInitializer implements insert(applicationContext, new SystemEnvironmentPropertySource( DECRYPTED_PROPERTY_SOURCE_NAME, map)); } - PropertySource bootstrap = propertySources - .get(BootstrapApplicationListener.BOOTSTRAP_PROPERTY_SOURCE_NAME); - if (bootstrap != null) { - map = decrypt(bootstrap); - if (!map.isEmpty()) { - found.addAll(map.keySet()); - insert(applicationContext, new SystemEnvironmentPropertySource( - DECRYPTED_BOOTSTRAP_PROPERTY_SOURCE_NAME, map)); - } - } if (!found.isEmpty()) { ApplicationContext parent = applicationContext.getParent(); if (parent != null) { @@ -166,26 +174,48 @@ public class EnvironmentDecryptApplicationInitializer implements } } + private void removeDecryptedProperties(ApplicationContext applicationContext) { + ApplicationContext parent = applicationContext; + while (parent != null) { + if (parent.getEnvironment() instanceof ConfigurableEnvironment) { + ((ConfigurableEnvironment) parent.getEnvironment()).getPropertySources() + .remove(DECRYPTED_PROPERTY_SOURCE_NAME); + } + parent = parent.getParent(); + } + } + public Map decrypt(PropertySources propertySources) { - Map overrides = new LinkedHashMap<>(); + Map properties = merge(propertySources); + decrypt(properties); + return properties; + } + + private Map decrypt(PropertySource source) { + Map properties = merge(source); + decrypt(properties); + return properties; + } + + private Map merge(PropertySources propertySources) { + Map properties = new LinkedHashMap<>(); List> sources = new ArrayList<>(); for (PropertySource source : propertySources) { sources.add(0, source); } for (PropertySource source : sources) { - decrypt(source, overrides); + merge(source, properties); } - return overrides; + return properties; } - private Map decrypt(PropertySource source) { - Map overrides = new LinkedHashMap<>(); - decrypt(source, overrides); - return overrides; + private Map merge(PropertySource source) { + Map properties = new LinkedHashMap<>(); + merge(source, properties); + return properties; } - private void decrypt(PropertySource source, Map overrides) { - + private void merge(PropertySource source, Map properties) { if (source instanceof CompositePropertySource) { List> sources = new ArrayList<>( @@ -193,7 +223,7 @@ public class EnvironmentDecryptApplicationInitializer implements Collections.reverse(sources); for (PropertySource nested : sources) { - decrypt(nested, overrides); + merge(nested, properties); } } @@ -206,47 +236,63 @@ public class EnvironmentDecryptApplicationInitializer implements Object property = source.getProperty(key); if (property != null) { String value = property.toString(); - if (value.startsWith("{cipher}")) { - value = value.substring("{cipher}".length()); - try { - value = this.encryptor.decrypt(value); - if (logger.isDebugEnabled()) { - logger.debug("Decrypted: key=" + key); - } - } - catch (Exception e) { - String message = "Cannot decrypt: key=" + key; - if (this.failOnError) { - throw new IllegalStateException(message, e); - } - if (logger.isDebugEnabled()) { - logger.warn(message, e); - } - else { - logger.warn(message); - } - // Set value to empty to avoid making a password out of the - // cipher text - value = ""; - } - overrides.put(key, value); + if (value.startsWith(ENCRYPTED_PROPERTY_PREFIX)) { + properties.put(key, value); if (COLLECTION_PROPERTY.matcher(key).matches()) { sourceHasDecryptedCollection = true; } } else if (COLLECTION_PROPERTY.matcher(key).matches()) { - // put non-ecrypted properties so merging of index properties + // put non-encrypted properties so merging of index properties // happens correctly otherCollectionProperties.put(key, value); } + else { + // override previously encrypted with non-encrypted property + properties.remove(key); + } } } // copy all indexed properties even if not encrypted if (sourceHasDecryptedCollection && !otherCollectionProperties.isEmpty()) { - overrides.putAll(otherCollectionProperties); + properties.putAll(otherCollectionProperties); } } } + private void decrypt(Map properties) { + properties.replaceAll((key, value) -> { + String valueString = value.toString(); + if (!valueString.startsWith(ENCRYPTED_PROPERTY_PREFIX)) { + return value; + } + return decrypt(key, valueString); + }); + } + + private String decrypt(String key, String original) { + String value = original.substring(ENCRYPTED_PROPERTY_PREFIX.length()); + try { + value = this.encryptor.decrypt(value); + if (logger.isDebugEnabled()) { + logger.debug("Decrypted: key=" + key); + } + return value; + } + catch (Exception e) { + String message = "Cannot decrypt: key=" + key; + if (this.failOnError) { + throw new IllegalStateException(message, e); + } + if (logger.isDebugEnabled()) { + logger.warn(message, e); + } + else { + logger.warn(message); + } + return ""; + } + } + } diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializerTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializerTests.java index 07137f9c..268c38d6 100644 --- a/spring-cloud-context/src/test/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializerTests.java +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializerTests.java @@ -18,6 +18,7 @@ package org.springframework.cloud.bootstrap.encrypt; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.Map; import org.junit.Test; @@ -36,9 +37,13 @@ import org.springframework.security.crypto.encrypt.Encryptors; import org.springframework.security.crypto.encrypt.TextEncryptor; import static org.assertj.core.api.BDDAssertions.then; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; +import static org.springframework.cloud.bootstrap.BootstrapApplicationListener.BOOTSTRAP_PROPERTY_SOURCE_NAME; +import static org.springframework.cloud.bootstrap.encrypt.EnvironmentDecryptApplicationInitializer.DECRYPTED_BOOTSTRAP_PROPERTY_SOURCE_NAME; import static org.springframework.cloud.bootstrap.encrypt.EnvironmentDecryptApplicationInitializer.DECRYPTED_PROPERTY_SOURCE_NAME; /** @@ -71,9 +76,8 @@ public class EnvironmentDecryptApplicationInitializerTests { public void propertySourcesOrderedCorrectly() { ConfigurableApplicationContext context = new AnnotationConfigApplicationContext(); TestPropertyValues.of("foo: {cipher}bar").applyTo(context); - context.getEnvironment().getPropertySources() - .addFirst(new MapPropertySource("test_override", - Collections.singletonMap("foo", "{cipher}spam"))); + context.getEnvironment().getPropertySources().addFirst(new MapPropertySource( + "test_override", Collections.singletonMap("foo", "{cipher}spam"))); this.listener.initialize(context); then(context.getEnvironment().getProperty("foo")).isEqualTo("spam"); } @@ -155,13 +159,9 @@ public class EnvironmentDecryptApplicationInitializerTests { @Test public void testDecryptCompositePropertySource() { - TextEncryptor textEncryptor = mock(TextEncryptor.class); - when(textEncryptor.decrypt(eq("value1"))).thenReturn("always1"); - when(textEncryptor.decrypt(eq("value2"))).thenReturn("always2"); - ConfigurableApplicationContext ctx = new AnnotationConfigApplicationContext(); EnvironmentDecryptApplicationInitializer initializer = new EnvironmentDecryptApplicationInitializer( - textEncryptor); + Encryptors.noOpText()); MapPropertySource devProfile = new MapPropertySource("dev-profile", Collections.singletonMap("key", "{cipher}value1")); @@ -176,7 +176,83 @@ public class EnvironmentDecryptApplicationInitializerTests { ctx.getEnvironment().getPropertySources().addLast(cps); initializer.initialize(ctx); - then(ctx.getEnvironment().getProperty("key")).isEqualTo("always1"); + then(ctx.getEnvironment().getProperty("key")).isEqualTo("value1"); + } + + @Test + public void propertySourcesOrderedCorrectlyWithUnencryptedOverrides() { + ConfigurableApplicationContext context = new AnnotationConfigApplicationContext(); + TestPropertyValues.of("foo: {cipher}bar").applyTo(context); + context.getEnvironment().getPropertySources().addFirst(new MapPropertySource( + "test_override", Collections.singletonMap("foo", "spam"))); + this.listener.initialize(context); + then(context.getEnvironment().getProperty("foo")).isEqualTo("spam"); + } + + @Test + public void doNotDecryptBootstrapTwice() { + TextEncryptor encryptor = mock(TextEncryptor.class); + when(encryptor.decrypt("bar")).thenReturn("bar"); + when(encryptor.decrypt("bar2")).thenReturn("bar2"); + when(encryptor.decrypt("bar3")).thenReturn("bar3"); + when(encryptor.decrypt("baz")).thenReturn("baz"); + + EnvironmentDecryptApplicationInitializer initializer = new EnvironmentDecryptApplicationInitializer( + encryptor); + + ConfigurableApplicationContext context = new AnnotationConfigApplicationContext(); + CompositePropertySource bootstrap = new CompositePropertySource( + BOOTSTRAP_PROPERTY_SOURCE_NAME); + bootstrap.addPropertySource(new MapPropertySource("configService", + Collections.singletonMap("foo", "{cipher}bar"))); + context.getEnvironment().getPropertySources().addFirst(bootstrap); + + Map props = new HashMap<>(); + props.put("foo2", "{cipher}bar2"); + props.put("bar", "{cipher}baz"); + context.getEnvironment().getPropertySources().addAfter( + BOOTSTRAP_PROPERTY_SOURCE_NAME, new MapPropertySource("remote", props)); + + initializer.initialize(context); + + // Simulate retrieval of new properties via Spring Cloud Config + props.put("foo2", "{cipher}bar3"); + context.getEnvironment().getPropertySources().replace("remote", + new MapPropertySource("remote", props)); + + initializer.initialize(context); + + verify(encryptor).decrypt("bar"); + verify(encryptor).decrypt("bar2"); + verify(encryptor).decrypt("bar3"); + verify(encryptor, times(2)).decrypt("baz"); + + // Check if all encrypted properties are still decrypted + PropertySource decryptedBootstrap = context.getEnvironment() + .getPropertySources().get(DECRYPTED_BOOTSTRAP_PROPERTY_SOURCE_NAME); + then(decryptedBootstrap.getProperty("foo")).isEqualTo("bar"); + + PropertySource decrypted = context.getEnvironment().getPropertySources() + .get(DECRYPTED_PROPERTY_SOURCE_NAME); + then(decrypted.getProperty("foo2")).isEqualTo("bar3"); + then(decrypted.getProperty("bar")).isEqualTo("baz"); + } + + @Test + public void testOnlyDecryptIfNotOverridden() { + ConfigurableApplicationContext context = new AnnotationConfigApplicationContext(); + TextEncryptor encryptor = mock(TextEncryptor.class); + when(encryptor.decrypt("bar2")).thenReturn("bar2"); + EnvironmentDecryptApplicationInitializer initializer = new EnvironmentDecryptApplicationInitializer( + encryptor); + TestPropertyValues.of("foo: {cipher}bar", "foo2: {cipher}bar2").applyTo(context); + context.getEnvironment().getPropertySources().addFirst(new MapPropertySource( + "test_override", Collections.singletonMap("foo", "spam"))); + initializer.initialize(context); + then(context.getEnvironment().getProperty("foo")).isEqualTo("spam"); + then(context.getEnvironment().getProperty("foo2")).isEqualTo("bar2"); + verify(encryptor).decrypt("bar2"); + verifyNoMoreInteractions(encryptor); } } From 86650dc215018ed087f4ac20536a142915f79a43 Mon Sep 17 00:00:00 2001 From: Tim Ysewyn Date: Fri, 26 Jul 2019 15:58:13 +0200 Subject: [PATCH 2/2] Updated license header --- .../encrypt/EnvironmentDecryptApplicationInitializer.java | 4 ++-- .../EnvironmentDecryptApplicationInitializerTests.java | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java b/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java index 100b7c44..327fd86c 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializer.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2017 the original author or authors. + * Copyright 2013-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -48,7 +48,7 @@ import org.springframework.security.crypto.encrypt.TextEncryptor; * override the encrypted values. * * @author Dave Syer - * + * @author Tim Ysewyn */ public class EnvironmentDecryptApplicationInitializer implements ApplicationContextInitializer, Ordered { diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializerTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializerTests.java index 268c38d6..a6ab2e6f 100644 --- a/spring-cloud-context/src/test/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializerTests.java +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/bootstrap/encrypt/EnvironmentDecryptApplicationInitializerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2013-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -49,6 +49,7 @@ import static org.springframework.cloud.bootstrap.encrypt.EnvironmentDecryptAppl /** * @author Dave Syer * @author Biju Kunjummen + * @author Tim Ysewyn */ public class EnvironmentDecryptApplicationInitializerTests {