spring-attic / spring-security-oauth

Support for adding OAuth1(a) and OAuth2 features (consumer and provider) for Spring web applications.
http://github.com/spring-projects/spring-security-oauth
Apache License 2.0
4.69k stars 4.05k forks source link

RedisTokenStore will break when upgrading spring-security-core #662

Open efenderbosch opened 8 years ago

efenderbosch commented 8 years ago

We just encountered this when upgrading to Spring Boot 1.3.0. Spring Boot 1.2.4 uses security core 3.2.7 but Spring Boot 1.3.0 has upgraded to 4.0.3.

Caused by: java.io.InvalidClassException: org.springframework.security.core.authority.SimpleGrantedAuthority; local class incompatible: stream classdesc serialVersionUID = 320, local class serialVersionUID = 400

This happens because the default serialization strategy is JdkSerializationStrategy and SimpleGrantedAuthority gets its serialVersionUID from SpringSecurityCoreVersion.SERIAL_VERSION_UID. I'm sure other objects in the tree have also had their serialVersionUID changed from 320 to 400 as well.

I'm not sure what to do about this. I guess just a warning to users when they upgrade? We are likely going to have to flush all our token related Redis keys when we deploy our services based on Spring Boot 1.3.0.

We are also considering implementing a Kyro based serialization strategy to avoid this problem in the future.

ricardoMogg commented 8 years ago

did you find any solution for this?

efenderbosch commented 8 years ago

I think a key prefix was added recently. The prefix could be used to keep v3 serialized objects and v4 serialized objects separate. I would effectively expire all tokens, though.

ricardoMogg commented 8 years ago

Cool! You really saved me some hours with this. Thank you!

sustainablepace commented 8 years ago

We have run into the same problem updating from Spring Boot 1.2.8 to 1.3.3.

Our "solution" is to implement a custom JSON serializer that is independent of changes in the SERIAL_VERSION_UID.

However, is anything done to prevent this problem with JdkSerialization in future updates of Spring Security? We can't afford to invalidate thousands of access tokens, but would love to continue working with an uncustomized Spring Boot stack.

Any feedback is welcome, thanks.

jgrandja commented 8 years ago

@dsyer What are your thoughts on this issue? Should the default RedisTokenStoreSerializationStrategy implementation be a JSON serializer instead of a JDK serializer?

efenderbosch commented 8 years ago

I tried writing a JSON serializer, but polymorphism was a problem. I got pretty far in to a Jackson Module to handle it, but it just wasn't quite right.

Something like Kryo would probably work pretty easily, however.

narup commented 7 years ago

I got this exception when I upgraded my Spring boot 1.3.1 to 1.4 Is this related? I haven't been able to upgrade to 1.4 at the moment since I am not sure how to resolve it.

dsyer commented 7 years ago

Probably. You don't really have a choice but to deal with it. Either delete all the tokens and let users generate new ones. Or converts them from the old to the new format in batch.

narup commented 7 years ago

Interesting. Thanks @dsyer

maksimu commented 7 years ago

Probably. You don't really have a choice but to deal with it. Either delete all the tokens and let users generate new ones. Or converts them from the old to the new format in batch.

What would be the best way to convert them in a batch? Do I have to have two version of the same lib. to be installed in order to convert from one to another?

jp-silva commented 5 years ago
aschatten commented 5 years ago

@dsyer The only difference inSimpleGrantedAuthority between 4.2.10.RELEASE and 5.0.9.RELEASE is just this:

@@ -39,10 +39,12 @@
        this.role = role;
    }

+   @Override
    public String getAuthority() {
        return role;
    }

+   @Override
    public boolean equals(Object obj) {
        if (this == obj) {
            return true;
@@ -55,10 +57,12 @@
        return false;
    }

+   @Override
    public int hashCode() {
        return this.role.hashCode();
    }

+   @Override
    public String toString() {
        return this.role;
    }

According to Java Object Serialization Specification this change is compatible and the version should remain the same. Otherwise, in order to upgrade all the tokens must be flushed or perform a complicated migration between 2 versions, which is completely unnecessary in this case.

aschatten commented 5 years ago

Following up on the discussion from https://gitter.im/spring-projects/spring-security?at=5c37972683c7e377654b3771:

There are multiple github tickets that discuss this issue. I am not sure which one I should move the discussion to. In OAuth project besides this one, there are couple more: spring-projects/spring-security-oauth#662, spring-projects/spring-security-oauth#665, spring-projects/spring-security-oauth#1547 Original change that caused the issue is this one: https://jira.spring.io/browse/SEC-1709 (spring-projects/spring-security#1945) – the root cause was that the same version of the class had different serialVersionUid in different JVMs, but after the fix set the version to the same constant across the entire library, it has been changing with every minor version. Here @rwinch stated that these objects were not supposed to be serialized between multiple versions. Then it means that even minor version upgrade is backwards incompatible, if this is the intention, then these objects should not be serialized and stored at all. More issues in Spring Security project here: spring-projects/spring-security#3918, spring-projects/spring-security#3737.

It looks like going forward the best course of action is to serialize using other formats. @fhanik mentioned JSON. I wonder if JSON or Kryo is considered to be a default format in the future?

ufosaga commented 5 years ago

As I mentioned in #1547, save serialized java class object is a horrible way, it makes very hard to migrate. I have to deploy a migrating service using the old version. Hope we can switch to JSON or other proper format in future version.

neale-bpm commented 4 years ago

@jgrandja @dsyer What do you propose for zero downtime deployments on this?

mkopylec commented 4 years ago

IMO the source cause of all the issues with serialization in spring-security is that spring-security forces developers to deal with object serialization. This is because spring-security sets those objects in the HTTP session as attributes. As objects I mean: OAuth2ClientContext, AuthenticationException, CsrfToken and so on. Those objects have no common structure that allows serialization tools to properly serialize and deserialize them, f.e. no-arg constructor. Another problem with those objects is that they are not backword compatible. Additionally, even if those objects don't change between versions, they are still not backword compatible because the SpringSecurityCoreVersion.SERIAL_VERSION_UID changes. This successfully prevents developers from upgrading spring-security's version, because it will likely break the already deployed application.

Instead of relaying on object session attributes, spring-security should rely on primitive/wrapper/string session attributes.

OrangeDog commented 4 years ago

The upgrade from 2.3.6 -> 2.3.7 also has this new serialization problem: #1803

juliusspencer commented 4 years ago

Hi, I have the same issue trying to migrate from 2.0.9 to 2.1.6 of spring-boot:

java.lang.IllegalArgumentException: java.io.InvalidClassException: org.springframework.security.core.authority.SimpleGrantedAuthority; local class incompatible: stream classdesc serialVersionUID = 500, local class serialVersionUID = 510

Is there any article that details how to get handle this with a number of users who have Oauth access and refresh tokens?

OrangeDog commented 4 years ago

@juliusspencer in theory, since the classes are actually identical, if you can modify the serialVersionUID in the binary data then it should work.

neale-bpm commented 4 years ago

You can provide your own deserialisation by overriding that method on the Spring Sec bean and fiddle the object stream to override the serialVersionUID. We've got a working version internally.

On Mon, 6 Jan 2020 at 15:14, James Howe notifications@github.com wrote:

@juliusspencer https://github.com/juliusspencer in theory, since the classes are actually identical, if you can modify the serialVersionUID in the binary data then it should work.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/spring-projects/spring-security-oauth/issues/662?email_source=notifications&email_token=AM4TI5Y6FI74UNR5EOMRJALQ4NDHHA5CNFSM4BW6LANKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIFXKTA#issuecomment-571176268, or unsubscribe https://github.com/notifications/unsubscribe-auth/AM4TI5577WTHFSNAQVG7O7LQ4NDHHANCNFSM4BW6LANA .

--

Disclaimer:

This email and any files transmitted with it are confidential and may be privileged. It is intended solely for the above addressee. If you have received it in error please delete it and notify the sender. The unauthorised use, disclosure, copying, alteration or  dissemination of any information in this e-mail is strictly forbidden. BigPay and its affiliates accepts no liability whatsoever that may arise from or in connection with this e-mail and/or its contents.

OrangeDog commented 4 years ago

@neale-bpm which exactly is "that method"?

neale-bpm commented 4 years ago

For us, override JdbcTokenStore's deserialisation:

    @Override
    protected OAuth2Authentication deserializeAuthentication(byte[] authentication) {
            return deserializeIgnoringVersionUid(authentication);
    }

where deserializeIgnoringVersionUid uses an ObjectInputStream subclass that overrides the readClassDescriptor() method with an copy that ignores the UID.

juliusspencer commented 4 years ago

Thanks for the help @neale-bpm. Do I need to create my own subclass of JdbcTokenStore, set the serialVersionUID to the original value and supply that in the tokenStore Bean?

juliusspencer commented 4 years ago

Just to follow up currently I'm just using the following:

        @Bean
    public TokenStore tokenStore() {
        return new JdbcTokenStore(dataSource);
    }
azdragon2 commented 4 years ago

@juliusspencer Yeah, I followed his recommendation and it worked for me on Spring Boot 2.2.4. Create your own subclass of JdbcTokenStore, override the deserializeAuthentication method. And use this:

public static <T> T deserializeIgnoringVersionUid(byte[] byteArray)
    {
        ObjectInputStream oip = null;
        try
        {
            oip = new YourOwnConfigurableObjectInputStream(new ByteArrayInputStream(byteArray), Thread.currentThread().getContextClassLoader());
            @SuppressWarnings("unchecked")
            T result = (T) oip.readObject();
            return result;
        }
        catch (IOException e)
        {
            throw new IllegalArgumentException(e);
        }
        catch (ClassNotFoundException e)
        {
            throw new IllegalArgumentException(e);
        }
        finally
        {
            if (oip != null)
            {
                try
                {
                    oip.close();
                }
                catch (IOException e)
                {
                    // eat it
                }
            }
        }
    }

And then for YourOwnConfigurableObjectInputStream, this guy's answer works perfectly: https://stackoverflow.com/questions/1816559/make-java-runtime-ignore-serialversionuids

DevDengChao commented 4 years ago

I think there should be an OAuth2AuthenticationJackson2Deserializer.

OrangeDog commented 4 years ago

@XieEDeHeiShou none of the classes involved are beans, and cannot be deserialized with Jackson.

DevDengChao commented 4 years ago

Can these code meet (skip de-serialization gap && readability of access/refresh token)'s requirement ?

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import lombok.extern.slf4j.Slf4j;
import org.springframework.data.redis.serializer.SerializationException;
import org.springframework.lang.NonNull;
import org.springframework.security.oauth2.provider.OAuth2Authentication;
import org.springframework.security.oauth2.provider.token.store.redis.JdkSerializationStrategy;
import org.springframework.security.oauth2.provider.token.store.redis.StandardStringSerializationStrategy;

import java.io.*;
import java.nio.charset.StandardCharsets;

@Slf4j
public class JacksonRedisTokenStoreSerializationStrategy extends StandardStringSerializationStrategy {

    @NonNull
    private final ObjectMapper mapper;
    @NonNull
    private final JdkSerializationStrategy jdkSerializationStrategy;

    public JacksonRedisTokenStoreSerializationStrategy() {
        this(new ObjectMapper());
    }

    public JacksonRedisTokenStoreSerializationStrategy(@NonNull ObjectMapper mapper) {
        this.mapper = mapper;
        jdkSerializationStrategy = new JdkSerializationStrategy();
    }

    @Override
    protected <T> T deserializeInternal(byte[] bytes, Class<T> clazz) {
        if (OAuth2Authentication.class.equals(clazz)) {
            try {
                log.debug("Deserializing input into OAuth2Authentication");
                //noinspection unchecked
                return (T) new DecompressibleInputStream(new ByteArrayInputStream(bytes)).readObject();
            } catch (Exception e) {
                log.warn("Unable to deserialize input into OAuth2Authentication object with DecompressibleInputStream");
                return jdkSerializationStrategy.deserialize(bytes, clazz);
            }
        }

        try {
            return mapper.readValue(bytes, clazz);
        } catch (IOException e) {
            String msg = "Unable to deserialize " + new String(bytes, StandardCharsets.UTF_8) + " into " + clazz.getName();
            throw new SerializationException(msg, e);
        }
    }

    @Override
    protected byte[] serializeInternal(Object object) {
        if (object instanceof OAuth2Authentication) {
            log.debug("Serializing OAuth2Authentication");
            return jdkSerializationStrategy.serialize(object);
        }
        try {
            return mapper.writeValueAsBytes(object);
        } catch (JsonProcessingException e) {
            throw new SerializationException("Unable to serialize " + object.getClass().getName(), e);
        }
    }

    @Slf4j
    private static class DecompressibleInputStream extends ObjectInputStream {

        public DecompressibleInputStream(InputStream in) throws IOException {
            super(in);
        }

        @Override
        protected ObjectStreamClass readClassDescriptor() throws IOException, ClassNotFoundException {
            ObjectStreamClass resultClassDescriptor = super.readClassDescriptor(); // initially streams descriptor
            Class<?> localClass; // the class in the local JVM that this descriptor represents.
            try {
                localClass = Class.forName(resultClassDescriptor.getName());
            } catch (ClassNotFoundException e) {
                log.error("No local class for " + resultClassDescriptor.getName(), e);
                return resultClassDescriptor;
            }
            ObjectStreamClass localClassDescriptor = ObjectStreamClass.lookup(localClass);
            if (localClassDescriptor != null) { // only if class implements serializable
                final long localSUID = localClassDescriptor.getSerialVersionUID();
                final long streamSUID = resultClassDescriptor.getSerialVersionUID();
                if (streamSUID != localSUID) { // check for serialVersionUID mismatch.
                    String s = "Overriding serialized class version mismatch: local serialVersionUID = " + localSUID +
                            " stream serialVersionUID = " + streamSUID;
                    Exception e = new InvalidClassException(s);
                    log.error("Potentially Fatal Deserialization Operation.", e);
                    resultClassDescriptor = localClassDescriptor; // Use local class descriptor for deserialization
                }
            }
            return resultClassDescriptor;
        }
    }
}