You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@myfaces.apache.org by "volosied (via GitHub)" <gi...@apache.org> on 2025/06/24 19:13:03 UTC
[PR] Use SHA256DRBG as default psuedo-random generator [myfaces]
volosied opened a new pull request, #911:
URL: https://github.com/apache/myfaces/pull/911
This is considered a breaking change (as 4.1 is released now), so we don't have to merge it. I'll let the community decide.
There is value in updating to a stronger SHA algorithm.
https://issues.apache.org/jira/browse/MYFACES-4726
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG,DRBG,SHA1PRNG as default psuedo-random generator [myfaces]
Posted by "volosied (via GitHub)" <gi...@apache.org>.
volosied closed pull request #911: MYFACES-4726 [4.1.x] Use SHA256DRBG,DRBG,SHA1PRNG as default psuedo-random generator
URL: https://github.com/apache/myfaces/pull/911
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG,DRBG,SHA1PRNG as default psuedo-random generator [myfaces]
Posted by "volosied (via GitHub)" <gi...@apache.org>.
volosied commented on code in PR #911:
URL: https://github.com/apache/myfaces/pull/911#discussion_r2286059040
##########
impl/src/main/java/org/apache/myfaces/util/token/SessionIdGenerator.java:
##########
@@ -18,35 +18,19 @@
*/
package org.apache.myfaces.util.token;
-import java.security.NoSuchAlgorithmException;
-import java.security.NoSuchProviderException;
import java.security.SecureRandom;
-import java.util.Queue;
-import java.util.concurrent.ConcurrentLinkedQueue;
-import java.util.logging.Level;
-import java.util.logging.Logger;
-import org.apache.myfaces.util.lang.ClassUtils;
-import org.apache.myfaces.util.lang.StringUtils;
-
/**
* NOTE: Class taken from tomcat 7 org.apache.catalina.util.SessionIdGenerator
* and used here as an alternative for server side state token encryption.
*
*/
public class SessionIdGenerator
-{
-
- private static Logger log = Logger.getLogger(SessionIdGenerator.class.getName());
-
- /**
- * Queue of random number generator objects to be used when creating session
- * identifiers. If the queue is empty when a random number generator is
- * required, a new random number generator object is created. This is
- * designed this way since random number generators use a sync to make them
- * thread-safe and the sync makes using a a single object slow(er).
+{
+ /*
+ * For performance, isolating SecureRandom between threads.
*/
- private Queue<SecureRandom> randoms = new ConcurrentLinkedQueue<>();
-
+ private static ThreadLocal<SecureRandom> randomLocal = new ThreadLocal<>();
+
Review Comment:
I use a static instance, so it's still reused. ThreadLocal doesn't make any impact here..
Need to review this some more....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG,DRBG,SHA1PRNG as default psuedo-random generator [myfaces]
Posted by "tandraschko (via GitHub)" <gi...@apache.org>.
tandraschko commented on code in PR #911:
URL: https://github.com/apache/myfaces/pull/911#discussion_r2287264178
##########
impl/src/main/java/org/apache/myfaces/config/webparameters/MyfacesConfig.java:
##########
@@ -1606,9 +1607,16 @@ public String getRandomKeyInViewStateSessionTokenSecureRandomProvider()
return randomKeyInViewStateSessionTokenSecureRandomProvider;
}
- public String getRandomKeyInViewStateSessionTokenSecureRandomAlgorithm()
+ public String[] getRandomKeyInViewStateSessionTokenSecureRandomAlgorithmList()
{
- return randomKeyInViewStateSessionTokenSecureRandomAlgorithm;
+ String[] algorithms =
Review Comment:
thats something that should be cached
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG,DRBG,SHA1PRNG as default psuedo-random generator [myfaces]
Posted by "volosied (via GitHub)" <gi...@apache.org>.
volosied commented on code in PR #911:
URL: https://github.com/apache/myfaces/pull/911#discussion_r2285376390
##########
impl/src/main/java/org/apache/myfaces/util/token/TokenGenerator.java:
##########
@@ -31,23 +34,46 @@ public class TokenGenerator
{
private final AtomicLong seed;
+ private static Logger log = Logger.getLogger(TokenGenerator.class.getName());
+
+ // TODO -- make a web parameter or it would be nice
+ // to consolidate RANDOM_KEY_IN_CSRF_SESSION_TOKEN_SECURE_RANDOM_ALGORITM,
+ // and RANDOM_KEY_IN_VIEW_STATE_SESSION_TOKEN_SECURE_RANDOM_ALGORITHM
+ private String[] supportedAlgorithmsList = {"SHA256DRBG","DRBG","SHA1PRNG"};
+
public TokenGenerator()
{
seed = new AtomicLong(generateSeed());
}
private long generateSeed()
{
- SecureRandom rng;
- try
+ SecureRandom rng = null;
Review Comment:
It's only called within the constructor.
generateSeed is only called once per instance but I don't thin there should be harm in making it static and caching it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG,DRBG,SHA1PRNG as default psuedo-random generator [myfaces]
Posted by "volosied (via GitHub)" <gi...@apache.org>.
volosied commented on code in PR #911:
URL: https://github.com/apache/myfaces/pull/911#discussion_r2285626954
##########
impl/src/main/java/org/apache/myfaces/util/token/TokenGenerator.java:
##########
@@ -31,23 +34,46 @@ public class TokenGenerator
{
private final AtomicLong seed;
+ private static Logger log = Logger.getLogger(TokenGenerator.class.getName());
+
+ // TODO -- make a web parameter or it would be nice
+ // to consolidate RANDOM_KEY_IN_CSRF_SESSION_TOKEN_SECURE_RANDOM_ALGORITM,
+ // and RANDOM_KEY_IN_VIEW_STATE_SESSION_TOKEN_SECURE_RANDOM_ALGORITHM
+ private String[] supportedAlgorithmsList = {"SHA256DRBG","DRBG","SHA1PRNG"};
+
public TokenGenerator()
{
seed = new AtomicLong(generateSeed());
}
private long generateSeed()
{
- SecureRandom rng;
- try
+ SecureRandom rng = null;
Review Comment:
I agree, let me fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG as default psuedo-random generator [myfaces]
Posted by "volosied (via GitHub)" <gi...@apache.org>.
volosied commented on PR #911:
URL: https://github.com/apache/myfaces/pull/911#issuecomment-3005876501
@tandraschko Your thoughts? Should we fix it in 4.1 (since it's recently released) or just in 5.0 instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG as default psuedo-random generator [myfaces]
Posted by "volosied (via GitHub)" <gi...@apache.org>.
volosied commented on PR #911:
URL: https://github.com/apache/myfaces/pull/911#issuecomment-3197190788
@melloware @tandraschko
Please take a look at the new update. These changes the config slightly. Instead of specifying one algorithm, you can use specify multiple in order of priority.
This affects `RANDOM_KEY_IN_VIEW_STATE_SESSION_TOKEN_SECURE_RANDOM_ALGORITHM` and `RANDOM_KEY_IN_CSRF_SESSION_TOKEN_SECURE_RANDOM_ALGORITM`.
This allows use to set new defaults: {"SHA256DRBG","DRBG", "SHA1PRNG"};
We'll try each in order (order of security), and if no success, then we'll resort to the default platform.
There are some performance trade-offs, but, with security, I think it's better to err on the side of caution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG as default psuedo-random generator [myfaces]
Posted by "melloware (via GitHub)" <gi...@apache.org>.
melloware commented on PR #911:
URL: https://github.com/apache/myfaces/pull/911#issuecomment-3001766097
personally i am OK with it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG as default psuedo-random generator [myfaces]
Posted by "volosied (via GitHub)" <gi...@apache.org>.
volosied commented on PR #911:
URL: https://github.com/apache/myfaces/pull/911#issuecomment-3145333123
I think this stills some more work -- making it a draft for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG,DRBG,SHA1PRNG as default psuedo-random generator [myfaces]
Posted by "tandraschko (via GitHub)" <gi...@apache.org>.
tandraschko commented on code in PR #911:
URL: https://github.com/apache/myfaces/pull/911#discussion_r2285388821
##########
impl/src/main/java/org/apache/myfaces/util/token/TokenGenerator.java:
##########
@@ -31,23 +34,46 @@ public class TokenGenerator
{
private final AtomicLong seed;
+ private static Logger log = Logger.getLogger(TokenGenerator.class.getName());
+
+ // TODO -- make a web parameter or it would be nice
+ // to consolidate RANDOM_KEY_IN_CSRF_SESSION_TOKEN_SECURE_RANDOM_ALGORITM,
+ // and RANDOM_KEY_IN_VIEW_STATE_SESSION_TOKEN_SECURE_RANDOM_ALGORITHM
+ private String[] supportedAlgorithmsList = {"SHA256DRBG","DRBG","SHA1PRNG"};
+
public TokenGenerator()
{
seed = new AtomicLong(generateSeed());
}
private long generateSeed()
{
- SecureRandom rng;
- try
+ SecureRandom rng = null;
Review Comment:
then we should rework this completely
we should look all allgorithm on startup (MyFacesConfig?) and only remember the algorithm to use
TokenGenerator/SessionIdGenerator should not try and try over and over
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG,DRBG,SHA1PRNG as default psuedo-random generator [myfaces]
Posted by "volosied (via GitHub)" <gi...@apache.org>.
volosied commented on PR #911:
URL: https://github.com/apache/myfaces/pull/911#issuecomment-3229339442
SHA256DRBG is not available on all JDKs. I don' think it should be the default.
SHA1PRNG is still used by Tomcat, so I think there's no rush to move to something stronger just yet.
Closing this for now, but perhaps we can revisit it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG,DRBG,SHA1PRNG as default psuedo-random generator [myfaces]
Posted by "tandraschko (via GitHub)" <gi...@apache.org>.
tandraschko commented on code in PR #911:
URL: https://github.com/apache/myfaces/pull/911#discussion_r2285082849
##########
impl/src/main/java/org/apache/myfaces/config/webparameters/MyfacesConfig.java:
##########
@@ -1609,7 +1609,14 @@ public String getRandomKeyInViewStateSessionTokenSecureRandomProvider()
public String[] getRandomKeyInViewStateSessionTokenSecureRandomAlgorithmList()
{
- return randomKeyInViewStateSessionTokenSecureRandomAlgorithmList.split(",");
+ String[] algorthms =
+ randomKeyInViewStateSessionTokenSecureRandomAlgorithmList.split(",");
Review Comment:
algorthms -> "i" missing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG as default psuedo-random generator [myfaces]
Posted by "volosied (via GitHub)" <gi...@apache.org>.
volosied commented on code in PR #911:
URL: https://github.com/apache/myfaces/pull/911#discussion_r2283278247
##########
impl/src/main/java/org/apache/myfaces/config/webparameters/MyfacesConfig.java:
##########
@@ -1609,7 +1609,14 @@ public String getRandomKeyInViewStateSessionTokenSecureRandomProvider()
public String[] getRandomKeyInViewStateSessionTokenSecureRandomAlgorithmList()
{
- return randomKeyInViewStateSessionTokenSecureRandomAlgorithmList.split(",");
+ String[] algorthms =
+ randomKeyInViewStateSessionTokenSecureRandomAlgorithmList.split(",");
Review Comment:
Indented on purpose as it's one statement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG,DRBG,SHA1PRNG as default psuedo-random generator [myfaces]
Posted by "tandraschko (via GitHub)" <gi...@apache.org>.
tandraschko commented on code in PR #911:
URL: https://github.com/apache/myfaces/pull/911#discussion_r2287288280
##########
impl/src/main/java/org/apache/myfaces/util/token/TokenGenerator.java:
##########
@@ -31,23 +34,46 @@ public class TokenGenerator
{
private final AtomicLong seed;
+ private static Logger log = Logger.getLogger(TokenGenerator.class.getName());
+
+ // TODO -- make a web parameter or it would be nice
+ // to consolidate RANDOM_KEY_IN_CSRF_SESSION_TOKEN_SECURE_RANDOM_ALGORITM,
+ // and RANDOM_KEY_IN_VIEW_STATE_SESSION_TOKEN_SECURE_RANDOM_ALGORITHM
+ private String[] supportedAlgorithmsList = {"SHA256DRBG","DRBG","SHA1PRNG"};
+
public TokenGenerator()
{
seed = new AtomicLong(generateSeed());
}
private long generateSeed()
{
- SecureRandom rng;
- try
+ SecureRandom rng = null;
Review Comment:
ping me if you did it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG,DRBG,SHA1PRNG as default psuedo-random generator [myfaces]
Posted by "volosied (via GitHub)" <gi...@apache.org>.
volosied commented on code in PR #911:
URL: https://github.com/apache/myfaces/pull/911#discussion_r2286018867
##########
impl/src/main/java/org/apache/myfaces/util/token/SessionIdGenerator.java:
##########
@@ -18,35 +18,19 @@
*/
package org.apache.myfaces.util.token;
-import java.security.NoSuchAlgorithmException;
-import java.security.NoSuchProviderException;
import java.security.SecureRandom;
-import java.util.Queue;
-import java.util.concurrent.ConcurrentLinkedQueue;
-import java.util.logging.Level;
-import java.util.logging.Logger;
-import org.apache.myfaces.util.lang.ClassUtils;
-import org.apache.myfaces.util.lang.StringUtils;
-
/**
* NOTE: Class taken from tomcat 7 org.apache.catalina.util.SessionIdGenerator
* and used here as an alternative for server side state token encryption.
*
*/
public class SessionIdGenerator
-{
-
- private static Logger log = Logger.getLogger(SessionIdGenerator.class.getName());
-
- /**
- * Queue of random number generator objects to be used when creating session
- * identifiers. If the queue is empty when a random number generator is
- * required, a new random number generator object is created. This is
- * designed this way since random number generators use a sync to make them
- * thread-safe and the sync makes using a a single object slow(er).
+{
+ /*
+ * For performance, isolating SecureRandom between threads.
*/
- private Queue<SecureRandom> randoms = new ConcurrentLinkedQueue<>();
-
+ private static ThreadLocal<SecureRandom> randomLocal = new ThreadLocal<>();
+
Review Comment:
I removed the queue as I don't think it's necessary anymore?
SecureRandom has been thread safe for years now (since Java 7). However, it's probaly not best to reuse the secure random instance with multiple threads, but, if we make it thread local, then I think it should be safe?
I shouldn't expect any performance hits, at least.
Please review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MYFACES-4726 [4.1.x] Use SHA256DRBG,DRBG,SHA1PRNG as default psuedo-random generator [myfaces]
Posted by "tandraschko (via GitHub)" <gi...@apache.org>.
tandraschko commented on code in PR #911:
URL: https://github.com/apache/myfaces/pull/911#discussion_r2285086426
##########
impl/src/main/java/org/apache/myfaces/util/token/TokenGenerator.java:
##########
@@ -31,23 +34,46 @@ public class TokenGenerator
{
private final AtomicLong seed;
+ private static Logger log = Logger.getLogger(TokenGenerator.class.getName());
+
+ // TODO -- make a web parameter or it would be nice
+ // to consolidate RANDOM_KEY_IN_CSRF_SESSION_TOKEN_SECURE_RANDOM_ALGORITM,
+ // and RANDOM_KEY_IN_VIEW_STATE_SESSION_TOKEN_SECURE_RANDOM_ALGORITHM
+ private String[] supportedAlgorithmsList = {"SHA256DRBG","DRBG","SHA1PRNG"};
+
public TokenGenerator()
{
seed = new AtomicLong(generateSeed());
}
private long generateSeed()
{
- SecureRandom rng;
- try
+ SecureRandom rng = null;
Review Comment:
i assume generateSeed will be called again and again
so please cache rng - always re-lookup it causes exceptions and exceptions each time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org