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