Skip to content
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

OAuth2AuthenticationExceptionMixin doesn't work in JDK 17 #11893

Closed
EvgeniGordeev opened this issue Sep 22, 2022 · 11 comments
Closed

OAuth2AuthenticationExceptionMixin doesn't work in JDK 17 #11893

EvgeniGordeev opened this issue Sep 22, 2022 · 11 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: bug A general bug
Milestone

Comments

@EvgeniGordeev
Copy link

EvgeniGordeev commented Sep 22, 2022

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 EvgeniGordeev added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Sep 22, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Sep 23, 2022

Hi, @EvgeniGordeev, I wonder if this is related to FasterXML/jackson-databind#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 jzheaux added status: waiting-for-feedback We need additional information before we can continue in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Sep 23, 2022
@jzheaux jzheaux self-assigned this Sep 23, 2022
@EvgeniGordeev
Copy link
Author

EvgeniGordeev commented Sep 26, 2022

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

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 26, 2022
@chrisrhut
Copy link

@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.

@jzheaux
Copy link
Contributor

jzheaux commented Oct 6, 2022

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 jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Oct 6, 2022
@chrisrhut
Copy link

@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!

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 6, 2022
@EvgeniGordeev
Copy link
Author

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

@EvgeniGordeev
Copy link
Author

EvgeniGordeev commented Dec 21, 2022

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());
    }
}

@jzheaux
Copy link
Contributor

jzheaux commented Dec 22, 2022

Thank you, @chrisrhut for offering to make a PR out of #11893 (comment). 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.

@chrisrhut
Copy link

chrisrhut commented Dec 26, 2022

(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.

@ugrave
Copy link
Contributor

ugrave commented Jun 2, 2023

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?

@sjohnr
Copy link
Member

sjohnr commented Sep 12, 2023

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 sjohnr assigned sjohnr and unassigned jzheaux Sep 12, 2023
@sjohnr sjohnr removed the status: feedback-provided Feedback has been provided label Sep 12, 2023
@sjohnr sjohnr moved this to In Progress in Spring Security Team Sep 12, 2023
@sjohnr sjohnr added the type: bug A general bug label Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: bug A general bug
Projects
Archived in project
Development

No branches or pull requests

6 participants