Introduce Support for Reading RSA Keys #6505

jzheaux · 2019-02-04T22:41:12Z

rwinch

I have provided feedback inline.

Also, we might consider how this is integrated with Spring's ApplicationContext. Should we provide support to ensure that it is automatically included? Should we also support conversion from a Resource to a key? For example:

@Value("classpath:key.private")
RSAPrivateKey key;

crypto/src/main/java/org/springframework/security/crypto/factory/RsaKeyConverters.java

jzheaux · 2019-02-07T03:21:21Z

@rwinch, I've added to more commits to this PR which address integrating with the application context.

I'm partial to actually having that in a separate PR, but if you feel like the changes are largely obvious/non-controversial, then I'm fine reviewing it all here.

rwinch · 2019-02-07T17:12:55Z

As discussed offline I will give you some time to polish this (i.e. move addConverters to be private within the post processor)

jzheaux · 2019-02-07T19:34:07Z

@rwinch, I've applied some polish. First, I moved the converter registration into the post-processor. Also, the post-processor has been simplified a bit, to implement BeanFactoryPostProcessor instead of BeanDefinitionRegistryPostProcessor.

Second, I consolidated and simplified the ApplicationContext-integration tests.

I think this looks much better now, and I'm okay with having everything reviewed together.

rwinch

Thanks for the updates! I have provided feedback/questions inline

rwinch · 2019-02-15T20:03:41Z

...ngframework/security/config/annotation/web/configuration/ConversionServicePostProcessor.java

+ * @author Josh Cummings
+ * @since 5.2
+ */
+public class ConversionServicePostProcessor implements BeanFactoryPostProcessor {


I wonder if there is a reason this is so generic given it adds RSA converts. I also wonder if it makes sense in the current package since RSA is applicable to non web apps and to reactive applications.

A quick search across the code base shows a handful of classes that extend BeanFactoryPostProcessor. For example, there is one for ldap, one for debug, and one for websocket, among others.

Each of these is in spring-security-config and follows the pattern org.springframework.security.config.{module}.

So, it seems natural to rename this as org.springframework.security.config.crypto.RsaKeyConversionServicePostProcessor since it is post processing the ConversionService to support RSA keys.

rwinch · 2019-02-15T20:05:33Z

...ngframework/security/config/annotation/web/configuration/ConversionServicePostProcessor.java

+
+	private Converter<String, InputStream> pemInputStreamConverter() {
+		ResourceLoader resourceLoader = new DefaultResourceLoader();
+		return source -> source.startsWith("-----") ?


Can you help me understand what the conditional logic is for? It looks like this might be to handle things like classpath:foo.pem. However, my concern is that it should not interfere with existing String to InputStream conversions.

Does this work if a variable (i.e. ${some.path} is used)? I cannot recall if anything special is necessary, but it has bitten me before.

Regarding your first question, the actual registered converter is String -> RSAPublicKey so it will only be invoked when the target type is RSAPublicKey. Yes, the conditional logic is to differentiate between a raw key and one referred to via classpath:foo.pem.

I didn't test your second case, but I will double-check this.

I changed the sample to use ${some.property.name} to confirm that works.

rwinch · 2019-02-15T20:08:55Z

...ngframework/security/config/annotation/web/configuration/ConversionServicePostProcessor.java

+	}
+
+	private Converter<String, InputStream> pemInputStreamConverter() {
+		ResourceLoader resourceLoader = new DefaultResourceLoader();


I think we should make the ResourceLoader a member variable and allow it to be injected.

rwinch · 2019-02-15T20:15:34Z

...ngframework/security/config/annotation/web/configuration/ConversionServicePostProcessor.java

+	 */
+	@Override
+	public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory) throws BeansException {
+		ConversionService service = beanFactory.getConversionService();


Does Spring always register a ConversionService (XML, Java Config, and Boot)?

No, XML and Java Config do not register a ConversionService. If the user (or Boot) does not supply one then Spring will fall back to the PropertyEditor conversion style.

I didn't look into property editors - would you recommend that in order to provide more in-depth support?

jzheaux · 2019-03-02T04:15:03Z

@rwinch, I've responded to your comments. Let me know what you think.

rwinch · 2019-03-13T19:26:18Z

crypto/src/main/java/org/springframework/security/crypto/factory/RsaKeyConverters.java

+	public static Converter<InputStream, RSAPrivateKey> pkcs8() {
+		KeyFactory keyFactory = rsaFactory();
+		return source -> {
+			BufferedReader reader = new BufferedReader(new InputStreamReader(source));


Perhaps I'm missing something, but shouldn't we close the InputStream after reading it? If not, we should make it clear we don't close the InputStream in the Javadoc and explain why.

rwinch · 2019-03-13T19:26:28Z

crypto/src/main/java/org/springframework/security/crypto/factory/RsaKeyConverters.java

+	public static Converter<InputStream, RSAPublicKey> x509() {
+		KeyFactory keyFactory = rsaFactory();
+		return source -> {
+			BufferedReader reader = new BufferedReader(new InputStreamReader(source));


Perhaps I'm missing something, but shouldn't we close the InputStream after reading it? If not, we should make it clear we don't close the InputStream in the Javadoc and explain why.

Good catch. My intent was to not make an assumption about the nature of the stream being passed in. So, I'll add a Javadoc to both methods.

That said, I think it makes sense to close it in the post processor since it's the post processor that opens it, so I'll change that, too.

rwinch · 2019-03-13T19:53:49Z

crypto/src/main/java/org/springframework/security/crypto/factory/RsaKeyConverters.java

+			BufferedReader reader = new BufferedReader(new InputStreamReader(source));
+			List<String> lines = reader.lines().collect(Collectors.toList());
+			Assert.isTrue(!lines.isEmpty() && lines.get(0).startsWith(X509_PEM_HEADER),
+					"Key is not in PEM-encoded X.509 format");


Perhaps add details about the X509_PEM_HEADER is missing to the error message?

rwinch · 2019-03-13T19:54:13Z

crypto/src/main/java/org/springframework/security/crypto/factory/RsaKeyConverters.java

+			BufferedReader reader = new BufferedReader(new InputStreamReader(source));
+			List<String> lines = reader.lines().collect(Collectors.toList());
+			Assert.isTrue(!lines.isEmpty() && lines.get(0).startsWith(PKCS8_PEM_HEADER),
+					"Key is not in PEM-encoded PKCS#8 format");


Perhaps add details about PKCS8_PEM_HEADER missing to the error message?

rwinch

Thanks. I provided feedback inline

rwinch · 2019-03-29T18:11:07Z

crypto/spring-security-crypto.gradle

@@ -1,6 +1,7 @@
 apply plugin: 'io.spring.convention.spring-module'

 dependencies {
+	optional 'org.springframework:spring-core'


Sorry for not realizing this till late in the game...I'm realizing we have a tangle here. The core includes crypto and now we have optional dependency on core. I think we need to move the crypo classes that use core into core.

Fixes: spring-projectsgh-6494

Issue: spring-projectsgh-6494

jzheaux · 2019-03-29T22:55:18Z

@rwinch I moved RsaKeyConverters and RsaKeyConvertersTest over to core. Otherwise, I simply cleaned up the commits a bit so now we just have two: One for the new feature and one for the change in one of the resource server samples.

Note that I placed these in org.springframework.security.converter, which is a new package. I chose it because converter is the tail of the package name for Converter itself, which aligns nicely, I think. Let me know, though, if there is a better spot for it.

jzheaux · 2019-04-14T01:39:24Z

This is now merged into master. (And I'm super excited about this feature!)

rwinch requested changes Feb 5, 2019

View reviewed changes

crypto/src/main/java/org/springframework/security/crypto/factory/RsaKeyConverters.java Outdated Show resolved Hide resolved

jzheaux force-pushed the gh-6494 branch from de39b3c to 30ef158 Compare February 7, 2019 01:16

jzheaux force-pushed the gh-6494 branch from 30ef158 to e9e3daf Compare February 7, 2019 19:19

jzheaux force-pushed the gh-6494 branch from e9e3daf to 751e76b Compare February 7, 2019 19:37

rwinch requested changes Feb 15, 2019

View reviewed changes

jzheaux force-pushed the gh-6494 branch from 751e76b to 5731afd Compare March 2, 2019 04:13

rwinch requested changes Mar 13, 2019

View reviewed changes

jzheaux force-pushed the gh-6494 branch 5 times, most recently from 722ad3f to e5ba1fd Compare March 19, 2019 19:37

rwinch requested changes Mar 29, 2019

View reviewed changes

jzheaux added 2 commits March 29, 2019 16:19

Introduce Support for Reading RSA Keys

a255b3e

Fixes: spring-projectsgh-6494

Resource Server Static Sample Uses @value For Key

6f99c34

Issue: spring-projectsgh-6494

jzheaux force-pushed the gh-6494 branch from e5ba1fd to 6f99c34 Compare March 29, 2019 22:20

rwinch approved these changes Apr 13, 2019

View reviewed changes

jzheaux merged commit b252371 into spring-projects:master Apr 14, 2019

jzheaux deleted the gh-6494 branch April 14, 2019 01:39

jzheaux self-assigned this Apr 14, 2019

jzheaux added this to the 5.2.0.M2 milestone Apr 14, 2019

jzheaux added in: core An issue in spring-security-core status: duplicate A duplicate of another issue New Feature labels Apr 14, 2019

rwinch added the type: enhancement A general enhancement label May 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Support for Reading RSA Keys #6505

Introduce Support for Reading RSA Keys #6505

jzheaux commented Feb 4, 2019

rwinch left a comment

jzheaux commented Feb 7, 2019

rwinch commented Feb 7, 2019

jzheaux commented Feb 7, 2019

rwinch left a comment

rwinch Feb 15, 2019

jzheaux Mar 2, 2019

rwinch Feb 15, 2019

rwinch Feb 15, 2019

jzheaux Feb 18, 2019

jzheaux Mar 2, 2019

rwinch Feb 15, 2019

rwinch Feb 15, 2019

jzheaux Feb 18, 2019

jzheaux commented Mar 2, 2019

rwinch Mar 13, 2019

rwinch Mar 13, 2019

jzheaux Mar 15, 2019

rwinch Mar 13, 2019

rwinch Mar 13, 2019

rwinch left a comment

rwinch Mar 29, 2019

jzheaux commented Mar 29, 2019 •

edited

Loading

jzheaux commented Apr 14, 2019 •

edited

Loading

Introduce Support for Reading RSA Keys #6505

Introduce Support for Reading RSA Keys #6505

Conversation

jzheaux commented Feb 4, 2019

rwinch left a comment

Choose a reason for hiding this comment

jzheaux commented Feb 7, 2019

rwinch commented Feb 7, 2019

jzheaux commented Feb 7, 2019

rwinch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzheaux commented Mar 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwinch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzheaux commented Mar 29, 2019 • edited Loading

jzheaux commented Apr 14, 2019 • edited Loading

jzheaux commented Mar 29, 2019 •

edited

Loading

jzheaux commented Apr 14, 2019 •

edited

Loading