Hi My mistake to suggest secureRandom as default. After doing the whole review it becomes clear.
In this moment with the methods moved to StateCache there is a conflict, because through the SPI interface (StateCacheFactory) in MYFACES-4078 you could override the token processing/generation, and that's not wanted by security reasons. I need to change my mind about instantiate StateTokenProcessor by StateCache, it doesn't work by that reason. I'll refactor the code to see what we can do. regards, Leonardo Uribe 2017-12-20 2:54 GMT-05:00 Thomas Andraschko <[email protected]>: > Hi, > > you suggested to make secureRandom as default, independent if we do this > change or not. > IMO "random" is enough - just sequence isn't safe enough anymore and must > not be used. > > +0 for reintroduce StateTokenProcessor > I currently don't see any benefit for another layer and i'm unsure if this > abstraction is ever needed. > But if you would like to refactor it, please do. > > > Regards, > Thomas > > 2017-12-20 2:18 GMT+01:00 Leonardo Uribe <[email protected]>: > >> Hi >> >> secureRandom is a performance bottleneck. That's a fact. It can't be set >> as default. >> >> See for example this blog: >> >> http://www.mattnworb.com/post/the-dangers-of-java-security-securerandom/ >> >> "... If you don’t read the descriptions carefully enough, you might miss >> the fact >> that /dev/random will block when there is not enough entropy data >> available. >> This means that in practice, it’s possible for your calls to >> new SecureRandom() to block for an unknown amount of time...." >> >> About MYFACES-3779: well, we are not solving that one here, but the >> current >> abstraction is strong enough to support it. >> >> About delete StateTokenProcessor: The token can include a mac, is >> encrypted, >> compressed, serialized. I think each one of these steps is complex enough >> to >> be encapsulated in one isolated class. Maybe we need to consider that >> StateTokenProcessor must be instantiated by StateCache, not by >> HtmlResponseStateManager. I don't think we are over engineering here, >> because >> the big abstraction are the three methods that does not really change, >> but if >> I want to create a new StateCache, maybe I don't want to write those three >> methods over again, right? If I'm writing a new StateCache instance, I >> want >> to focus on how to store the state. Please note MYFACES-4078 expose >> StateCacheFactory/StateCache to the public since 2.3.0, so these 3 methods >> in StateCache creates a conflict with this issue I have been working for a >> long time. >> >> This issue makes me feel I'm walking in circles. >> >> Keep it simple, follow the facts, the old known algorithm that uses mac >> +encryption >> works just fine. The algorithm has been designed to keep the token very >> small >> (IntIntKey...), so simplify here undo a previous work in that area. This >> is >> a problem where we reach a dead end. Just turn back. >> >> But move StateTokenProcessor inside StateCache is a good idea, but a >> default implementation that behaves like we already know is important. >> >> regards, >> >> Leonardo Uribe >> >> >> >> 2017-12-19 16:23 GMT-05:00 Thomas Andraschko <[email protected] >> >: >> >>> Yep, it's a viewstate-id or token, not view-id of course! >>> If the current secureRandom is not perfect, we can still improve it. >>> Also without this change discussed here, we would have set secureRandom >>> as our default (we discussed this in the other topic), so i don't think see >>> a problem here. >>> >>> >>> I understand your point about https://issues.apache.org/jira >>> /browse/MYFACES-3779 but quite a lot work must be done to get this >>> working and is unsure if we will ever work on something. >>> Also if we would work on it, i'm sure we would have to refactor much >>> more things. >>> >>> Also your statement >>> "The key point is maybe the state saving mode has something to say about >>> how the token is processed." >>> says that its actually one unit. >>> >>> We should not over-engineer something which is maybe never required. >>> maintainability > abstraction - as it's already quite hard the follow >>> our code in some places. >>> >>> >>> >>> >>> 2017-12-19 21:57 GMT+01:00 Leonardo Uribe <[email protected]>: >>> >>>> Hi >>>> >>>> TA>> Yep, it's not a bug but a good improvement. Personally i would >>>> only do >>>> TA>> it in 2.3. >>>> >>>> I agree, only 2.3. >>>> >>>> TA>> As i already explained, i removed those classes as "sequence" >>>> view-id >>>> TA>> generator must not be used anymore. >>>> >>>> I'm confused about this term "view-id". It is similar to JSF viewId, >>>> which >>>> is the path of the view. Let's do some recap. >>>> >>>> In server side state saving we use a token. This token is composed of: >>>> >>>> - A session counter. >>>> - viewId or related hashCode. >>>> - (optional) a random number. >>>> >>>> In JSF 1.1 the full viewId path was saved with a sequence to be able to >>>> identify the right view state in the collection. Later the viewId was >>>> changed with a hash code, because the viewId was too large, and we >>>> needed >>>> a smaller token. >>>> >>>> The idea behind a random number was the token should not be easily >>>> guessed. >>>> Well, it is not a requirement, since the token is mac-encoded and >>>> encrypted. >>>> But generate a random number is slow in some systems and JDK versions >>>> (linux, >>>> Sun-Oracle JDKs). That's the reason why the random number is not by >>>> default, >>>> because it is not possible. >>>> >>>> There are fast random number generators but remember, to make it safe >>>> it >>>> should not be possible to guess it, well if you have plans to disable >>>> encryption. So, there is an option to use a "secureRandom" number >>>> generator. >>>> >>>> TA>> My reasons to merge both classes are that we already have a >>>> artifact >>>> TA>> which handles client side state, and we already have a artifact >>>> which >>>> TA>> handles server side state. >>>> >>>> TA>> Maybe "Cache" is not a 100% perfect name but it's ok IMO. We could >>>> TA>> still search for another name, which would be a very very small >>>> TA>> improvement. There is no reason to e.g. use a >>>> TA>> ClientSide-StateTokenProcessor with a ServerSide-StateCache. >>>> TA>> Thereis one big mechanism for server-side state and one for >>>> TA>> client-side state. How to render it and how to create/restore it, >>>> TA>> are both sides of the medal. >>>> >>>> I can remember this issue: >>>> >>>> https://issues.apache.org/jira/browse/MYFACES-3779 >>>> Mixed mode(Server+client) for state saving >>>> >>>> It is a long discussion, but the point is that a mixed mode is possible. >>>> The work done in the API was meant to go towards that goal. The state >>>> saving algorithm is something really hard to customize. I strongly feel >>>> StateTokenProcessor has a lot of sense. The key point is maybe the >>>> state saving mode has something to say about how the token is processed. >>>> In this case, the key info is tell the processor that the token must >>>> not be serialized. >>>> >>>> regards, >>>> >>>> Leonardo Uribe >>>> >>>> 2017-12-19 15:21 GMT-05:00 Thomas Andraschko < >>>> [email protected]>: >>>> >>>>> Yep, it's not a bug but a good improvement. Personally i would only do >>>>> it in 2.3. >>>>> >>>>> As i already explained, i removed those classes as "sequence" view-id >>>>> generator must not be used anymore. >>>>> >>>>> My reasons to merge both classes are that we already have a artifact >>>>> which handles client side state, and we already have a artifact which >>>>> handles server side state. >>>>> Maybe "Cache" is not a 100% perfect name but it's ok IMO. We could >>>>> still search for another name, which would be a very very small >>>>> improvement. >>>>> There is no reason to e.g. use a ClientSide-StateTokenProcessor with a >>>>> ServerSide-StateCache. >>>>> Thereis one big mechanism for server-side state and one for >>>>> client-side state. How to render it and how to create/restore it, are both >>>>> sides of the medal. >>>>> >>>>> >>>>> >>>>> 2017-12-19 21:09 GMT+01:00 Leonardo Uribe <[email protected]>: >>>>> >>>>>> Hi >>>>>> >>>>>> There are some changes in MYFACES-4133 (2.3.x branch) that is >>>>>> required to be >>>>>> discussed on dev list and not on jira. >>>>>> >>>>>> MYFACES-4133 is all about refactor the existing state API to avoid >>>>>> the java >>>>>> token serialization. I have already stated this is an improvement, >>>>>> not a bug, >>>>>> but since there is a lot of interest in this point I would like to >>>>>> expose the >>>>>> ideas behind the existing API. >>>>>> >>>>>> The change proposed removes these classes: >>>>>> >>>>>> StateTokenProcessor >>>>>> IntIntSerializedViewKey >>>>>> CounterSessionViewStorageFactory >>>>>> IntByteArraySerializedViewKey >>>>>> CounterKeyFactory >>>>>> >>>>>> And the addition of some methods in StateCache: >>>>>> >>>>>> public abstract Object decodeStateToken(FacesContext facesContext, >>>>>> String token); >>>>>> public abstract String encodeStateToken(FacesContext facesContext, >>>>>> Object savedStateObject); >>>>>> public boolean isStatelessToken(FacesContext facesContext, String >>>>>> token) >>>>>> >>>>>> The first problem here is StateCache abstraction. >>>>>> >>>>>> "... provides an interface to separate the state caching operations >>>>>> (saving/restoring) from the renderkit specific stuff that >>>>>> HtmlResponseStateManager should do. ..." >>>>>> >>>>>> Token serialization is HtmlResponseStateManager stuff, specifically >>>>>> related to >>>>>> the render step. >>>>>> >>>>>> StateTokenProcessor was deleted but this interface had the following >>>>>> methods: >>>>>> >>>>>> public abstract Object decode(FacesContext facesContext, String >>>>>> token); >>>>>> public abstract String encode(FacesContext facesContext, Object >>>>>> savedStateObject); >>>>>> public abstract boolean isStateless(FacesContext facesContext, String >>>>>> token); >>>>>> >>>>>> My first question is what's the reason of merge StateTokenProcessor >>>>>> and >>>>>> StateCache? What's the advantage? >>>>>> >>>>>> Well, I can see one, StateCache has a logic related to client-server >>>>>> state saving. >>>>>> Server side state saving uses a counter as token, client side state >>>>>> saving >>>>>> encodes the view state inside the token. >>>>>> >>>>>> But still I think StateTokenProcessor has life on its own. Am I >>>>>> missing something? >>>>>> >>>>>> regards, >>>>>> >>>>>> Leonardo Uribe >>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> >
