spring-security icon indicating copy to clipboard operation
spring-security copied to clipboard

OAuth2AuthenticationExceptionMixin doesn't work in JDK 17

Open EvgeniGordeev opened this issue 2 years ago • 6 comments

Describe the bug With Redis session enabled, GenericJackson2JsonRedisSerializer based on ObjectMapper with OAuth2ClientJackson2Module an exception is thrown in JDK 17 while serializing:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Invalid type definition for type `org.springframework.security.oauth2.core.OAuth2AuthenticationException`: Failed to construct BeanSerializer for [simple type, class org.springframework.security.oauth2.core.OAuth2AuthenticationException]: (java.lang.IllegalArgumentException) Failed to call `setAccess()` on Field 'detailMessage' (of class `java.lang.Throwable`) due to `java.lang.reflect.InaccessibleObjectException`, problem: Unable to make field private java.lang.String java.lang.Throwable.detailMessage accessible: module java.base does not "opens java.lang" to unnamed module @5aebe890

To Reproduce

Spring boot 2.7.2:


@EnableRedisRepositories
@Configuration
public class RedisSessionConfig implements BeanClassLoaderAware {
    private ClassLoader loader;

    /**
     * Workaround for https://github.com/spring-projects/spring-session/issues/124.
     */
    @Bean
    public ConfigureRedisAction configureRedisAction() {
        return ConfigureRedisAction.NO_OP;
    }

    @Bean
    public RedisSerializer<Object> springSessionDefaultRedisSerializer() {
        return new GenericJackson2JsonRedisSerializer(objectMapper());
    }

    private ObjectMapper objectMapper() {
        ObjectMapper om = new ObjectMapper();
        om.activateDefaultTyping(om.getPolymorphicTypeValidator(), ObjectMapper.DefaultTyping.NON_FINAL, JsonTypeInfo.As.PROPERTY);
        om.registerModules(SecurityJackson2Modules.getModules(this.loader));
        om.registerModule(new OidcSecurityUserModule());
        return om;
    }

    @Override
    public void setBeanClassLoader(ClassLoader classLoader) {
        this.loader = classLoader;
    }

Expected behavior OAuth2AuthenticationException object is successfully serialized in JDK 17.

Workaround

VM option --add-opens java.base/java.lang=ALL-UNNAMED as usual.

EvgeniGordeev avatar Sep 22 '22 17:09 EvgeniGordeev

Hi, @EvgeniGordeev, I wonder if this is related to https://github.com/FasterXML/jackson-databind/issues/3275. Have you already tried using the latest Jackson dependencies to see if that resolves the issue? It appears that the above issue was fixed in 2.13.4, though Boot 2.7.2 uses 2.13.3.

jzheaux avatar Sep 23 '22 19:09 jzheaux

@jzheaux Checked it in SB 2.7.4 - the issue is still there.

EvgeniGordeev avatar Sep 26 '22 17:09 EvgeniGordeev

@jzheaux here's a rudimentary test that shows off the issue with Spring Boot 2.7.4 and Jackson 2.13.4: https://github.com/chrisrhut/spring-security-11893

As mentioned by @EvgeniGordeev it can be fixed by adding the --add-opens VM argument.

chrisrhut avatar Sep 27 '22 23:09 chrisrhut

Since the exception is complaining about code controlled by Jackson, it seems to me this would need to be addressed in Jackson.

Is there some way that Spring Security is using Jackson incorrectly? If not, please open a ticket with Jackson. Feel free to add here a link to that ticket.

jzheaux avatar Oct 06 '22 17:10 jzheaux

@jzheaux I do believe this is something Spring Security is doing incorrectly, but that the failure only manifests in newer JDK's (16 and higher), that have hardened security around reflection in the module system.

In the OAuth2AuthenticationExceptionMixin definition:

@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS)
@JsonAutoDetect(fieldVisibility = JsonAutoDetect.Visibility.ANY, getterVisibility = JsonAutoDetect.Visibility.NONE,
		isGetterVisibility = JsonAutoDetect.Visibility.NONE)
@JsonIgnoreProperties(ignoreUnknown = true, value = { "cause", "stackTrace", "suppressedExceptions" })
abstract class OAuth2AuthenticationExceptionMixin {
	@JsonCreator
	OAuth2AuthenticationExceptionMixin(@JsonProperty("error") OAuth2Error error,
			@JsonProperty("detailMessage") String message) {
	}
}

I think the @JsonProperty("detailMessage") - combined with the ANY field visibility - means that Spring is asking Jackson to set the private field in java.lang.Throwable directly, rather than via the constructor chain, this is not allowed as of JDK 16.

Using message - instead of detailMessage - fixes this; I have tested this change on my machine with the above test repo and it works; what I'm unsure of is downstream ramifications / compatibility issues around this.

The patch is simply this. I'm happy to make it into a PR:

diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/jackson2/OAuth2AuthenticationExceptionMixin.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/jackson2/OAuth2AuthenticationExceptionMixin.java
index 9fa810c1ac..b52557d5aa 100644
--- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/jackson2/OAuth2AuthenticationExceptionMixin.java
+++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/jackson2/OAuth2AuthenticationExceptionMixin.java
@@ -42,7 +42,7 @@ abstract class OAuth2AuthenticationExceptionMixin {

    @JsonCreator
    OAuth2AuthenticationExceptionMixin(@JsonProperty("error") OAuth2Error error,
-           @JsonProperty("detailMessage") String message) {
+           @JsonProperty("message") String message) {
    }

 }

Thanks!

chrisrhut avatar Oct 06 '22 23:10 chrisrhut

@chrisrhut I had to add a similar mixin to get around it.

EvgeniGordeev avatar Oct 07 '22 03:10 EvgeniGordeev

workaround:

Mixin


import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import org.springframework.security.oauth2.core.OAuth2Error;

/**
 * https://github.com/spring-projects/spring-security/issues/11893.
 * See {@link org.springframework.security.oauth2.client.jackson2.OAuth2AuthenticationExceptionMixin}.
 */
@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS)
@JsonIgnoreProperties(ignoreUnknown = true, value = {"cause", "stackTrace", "suppressedExceptions", "detailMessage", "suppressed", "localizedMessage"})
abstract class OAuth2AuthenticationExceptionMixinJdk17Fix {

    @JsonCreator
    OAuth2AuthenticationExceptionMixinJdk17Fix(@JsonProperty("error") OAuth2Error error,
                                               @JsonProperty("message") String message) {
    }

}

Custom Jackson Module:

public class CustomModule extends SimpleModule {
    public CustomModule() {
        super(CustomModule.class.getName(), new Version(1, 0, 0, null, null, null));
    }

    public void setupModule(Module.SetupContext context) {
        context.setMixInAnnotations(OAuth2AuthenticationException.class, OAuth2AuthenticationExceptionMixinJdk17Fix.class);
    }
}

Test:

import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.springframework.security.jackson2.SecurityJackson2Modules;
import org.springframework.security.oauth2.core.OAuth2AuthenticationException;
import org.springframework.security.oauth2.core.OAuth2Error;
import org.springframework.security.oauth2.core.OAuth2ErrorCodes;

public class OAuth2AuthenticationExceptionMixinJdk17FixTest {
    @Test
    public void writeAndRead() throws JsonProcessingException {
        ObjectMapper om = new ObjectMapper();
        om.activateDefaultTyping(om.getPolymorphicTypeValidator(), ObjectMapper.DefaultTyping.NON_FINAL, JsonTypeInfo.As.PROPERTY);
        om.registerModules(SecurityJackson2Modules.getModules(this.getClass().getClassLoader()));
        om.registerModule(new OidcSupportModule());

        OAuth2AuthenticationException exception = new OAuth2AuthenticationException(new OAuth2Error(OAuth2ErrorCodes.INVALID_CLIENT), "errorMessage");
        String deser = om.writeValueAsString(exception);
        OAuth2AuthenticationException ser = om.readValue(deser, OAuth2AuthenticationException.class);

        Assertions.assertEquals(exception.getError().getErrorCode(), ser.getError().getErrorCode());
        Assertions.assertEquals(exception.getMessage(), ser.getMessage());
    }
}

EvgeniGordeev avatar Dec 21 '22 16:12 EvgeniGordeev

Thank you, @chrisrhut for offering to make a PR out of https://github.com/spring-projects/spring-security/issues/11893#issuecomment-1270842722. That would be most welcome.

It would be nice to merge this to 6.x as well as 5.x in which case, the mixin should still recognize old serialized values that use the detailMessage key.

jzheaux avatar Dec 22 '22 20:12 jzheaux

(Edit see the update below.)

@jzheaux I started to take a look at this over the holiday. I'm a little stymied because I can't reproduce the issue in the spring-security project. I tried directly copying my test case into OAuth2AuthenticationExceptionMixinTests and cannot get the test to fail, even if I check out the 5.7.2 tag.

I thought it may be because the project declares the workaround JVM args in its JUnit test configuration (source):

tasks.named('test', Test).configure {
	onlyIf { !project.hasProperty("buildSrc.skipTests") }
	useJUnitPlatform()
	jvmArgs(
			'--add-opens', 'java.base/java.lang=ALL-UNNAMED',
			'--add-opens', 'java.base/java.util=ALL-UNNAMED'
	)
}

But even if I remove that, I cannot get the test to fail and thus I can't get into a valid starting position for the fix and associated tests. I'm not sure if you have any inkling what could be different, but I'd appreciate a second set of eyes.

Here are a couple of other things I tried:

  • Updated my test project to use Spring Security 6.0.1.RELEASE - the test still fails
  • Updated my test project to use Jackson 2.14.0 - the test still fails

Thanks for any new insight!

PS here's a patch to copy my test case into spring-security:

diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/jackson2/OAuth2AuthenticationExceptionMixinTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/jackson2/OAuth2AuthenticationExceptionMixinTests.java
index ab97544e2f..4df7f0a0e7 100644
--- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/jackson2/OAuth2AuthenticationExceptionMixinTests.java
+++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/jackson2/OAuth2AuthenticationExceptionMixinTests.java
@@ -65,6 +65,14 @@ public class OAuth2AuthenticationExceptionMixinTests {
        JSONAssert.assertEquals(expected, serializedJson, true);
    }

+   @Test
+   public void demonstratesIssue11893() throws Exception {
+       Exception e = new OAuth2AuthenticationException(new OAuth2Error("invalid_nonce"));
+       byte[] bytes = mapper.writeValueAsBytes(e);
+
+       assertThat(bytes).isNotNull();
+   }
+
    @Test

EDIT:

Update, the lava.lang package seems to be implicitly marked as open by the runtime because of Jacoco/instrumentation in the spring-security project. (Tried to attach a screen shot of an IDE debugging session to demonstrate why I think this is, but Github isn't allowing that at the moment.) Thus I'm not sure if/how we can get a viable test case for this issue.

chrisrhut avatar Dec 26 '22 19:12 chrisrhut

Hi, found out that the Saml2AuthenticationExceptionMixin as the same problem with jdk17:

Caused by: java.io.UncheckedIOException: com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Invalid type definition for type org.springframework.security.saml2.provider.service.authentication.Saml2AuthenticationException: Failed to construct BeanSerializer for [simple type, class org.springframework.security.saml2.provider.service.authentication.Saml2AuthenticationException]: (java.lang.IllegalArgumentException) Failed to call setAccess() on Field 'detailMessage' (of class java.lang.Throwable) due to java.lang.reflect.InaccessibleObjectException, problem: Unable to make field private java.lang.String java.lang.Throwable.detailMessage accessible: module java.base does not "opens java.lang" to unnamed module @536f2a7e

Should i create a seperate ticket for it?

ugrave avatar Jun 02 '23 07:06 ugrave

OAuth2AuthenticationExceptionMixinTests fail due to this issue ~on JDK 21~ when upgrading the jacoco tool version from 0.8.7 to 0.8.9.

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Invalid type definition for type `org.springframework.security.oauth2.core.OAuth2AuthenticationException`: Failed to construct BeanSerializer for [simple type, class org.springframework.security.oauth2.core.OAuth2AuthenticationException]: (java.lang.IllegalArgumentException) Failed to call `setAccess()` on Field 'detailMessage' (of class `java.lang.Throwable`) due to `java.lang.reflect.InaccessibleObjectException`, problem: Unable to make field private java.lang.String java.lang.Throwable.detailMessage accessible: module java.base does not "opens java.lang" to unnamed module @c88a337
	at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:72)
	at com.fasterxml.jackson.databind.SerializerProvider.reportBadTypeDefinition(SerializerProvider.java:1280)
	at com.fasterxml.jackson.databind.ser.BeanSerializerFactory.constructBeanOrAddOnSerializer(BeanSerializerFactory.java:475)
	at com.fasterxml.jackson.databind.ser.BeanSerializerFactory.findBeanOrAddOnSerializer(BeanSerializerFactory.java:295)
	at com.fasterxml.jackson.databind.ser.BeanSerializerFactory._createSerializer2(BeanSerializerFactory.java:240)
	at com.fasterxml.jackson.databind.ser.BeanSerializerFactory.createSerializer(BeanSerializerFactory.java:174)
	at com.fasterxml.jackson.databind.SerializerProvider._createUntypedSerializer(SerializerProvider.java:1503)
	at com.fasterxml.jackson.databind.SerializerProvider._createAndCacheUntypedSerializer(SerializerProvider.java:1451)
	at com.fasterxml.jackson.databind.SerializerProvider.findValueSerializer(SerializerProvider.java:556)
	at com.fasterxml.jackson.databind.SerializerProvider.findTypedValueSerializer(SerializerProvider.java:834)
	at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider.serializeValue(DefaultSerializerProvider.java:307)
	at com.fasterxml.jackson.databind.ObjectMapper._writeValueAndClose(ObjectMapper.java:4719)
	at com.fasterxml.jackson.databind.ObjectMapper.writeValueAsString(ObjectMapper.java:3964)
	at org.springframework.security.oauth2.client.jackson2.OAuth2AuthenticationExceptionMixinTests.serializeWhenRequiredAttributesOnlyThenSerializes(OAuth2AuthenticationExceptionMixinTests.java:63)
        ...

sjohnr avatar Sep 12 '23 20:09 sjohnr