Re: digest.bat (RealmBase.main()) is broken in current Tomcat 8 trunk. Tomcat 8.0.14 is OK.

2014-10-22 Thread Mark Thomas
On 17/10/2014 14:13, Konstantin Kolinko wrote:
 2014-09-30 19:22 GMT+04:00 Konstantin Kolinko knst.koli...@gmail.com:
 2014-09-29 14:43 GMT+04:00 Mark Thomas ma...@apache.org:
 On 27/09/2014 15:52, Konstantin Kolinko wrote:
 ()

 4) The current javadoc for RealmBase.main() says that algorithm (-a)
 is not required and If not specified a default of SHA-512 will be
 used.

 I wonder whether that is justified.

 That is what is currently implemented. Happy to discuss changes but
 SHA-512 doesn't seem unreasonable to me.


 I think there is a contradiction between -a algorithm and -h
 credential handler implementation class keys:
 1)  If -h is used I think it shall default to whatever default
 algorithm the credential handler implements.
 2) Custom credential handler implementations may lack setAlgorithm() method.

 I think that one of (-a, -h) is required, with no default for either.
 The old code had no default for algorithm.

I agree with the two issues above but I have a different solution.

If neither -a or -h is specified, SHA-512 and
MessageDigestCredentialHandler will be used.

If only -a is specified, the built-in handlers will be searched in order
(MessageDigestCredentialHandler, SecretKeyCredentialHandler) and the
first handler that supports the algorithm will be used.

If only -h is specified, no default will be used for -a. The handler may
or may nor support -a and may or may not supply a sensible default.


 String encoding = UTF-8;

 I think it shall use system encoding, because the value is passed on
 the command line and is not read from file etc.

Fixed.

 BTW,  That chapter in realm-howto in Tomcat 8 needs an update for the
 new features of digest.sh / RealmBase.main().

Fixed.

 I think that this have to be fixed before tagging next Tomcat 8 release.

I believe I have address all the outstanding concerns with these
changes. Let me know if I have missed something.

Mark


-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



Re: digest.bat (RealmBase.main()) is broken in current Tomcat 8 trunk. Tomcat 8.0.14 is OK.

2014-10-22 Thread Konstantin Kolinko
2014-10-22 14:22 GMT+04:00 Mark Thomas ma...@apache.org:
 On 17/10/2014 14:13, Konstantin Kolinko wrote:
 2014-09-30 19:22 GMT+04:00 Konstantin Kolinko knst.koli...@gmail.com:
 2014-09-29 14:43 GMT+04:00 Mark Thomas ma...@apache.org:
 On 27/09/2014 15:52, Konstantin Kolinko wrote:
 ()

 4) The current javadoc for RealmBase.main() says that algorithm (-a)
 is not required and If not specified a default of SHA-512 will be
 used.

 I wonder whether that is justified.

 That is what is currently implemented. Happy to discuss changes but
 SHA-512 doesn't seem unreasonable to me.


 I think there is a contradiction between -a algorithm and -h
 credential handler implementation class keys:
 1)  If -h is used I think it shall default to whatever default
 algorithm the credential handler implements.
 2) Custom credential handler implementations may lack setAlgorithm() method.

 I think that one of (-a, -h) is required, with no default for either.
 The old code had no default for algorithm.

 I agree with the two issues above but I have a different solution.

 If neither -a or -h is specified, SHA-512 and
 MessageDigestCredentialHandler will be used.

 If only -a is specified, the built-in handlers will be searched in order
 (MessageDigestCredentialHandler, SecretKeyCredentialHandler) and the
 first handler that supports the algorithm will be used.

 If only -h is specified, no default will be used for -a. The handler may
 or may nor support -a and may or may not supply a sensible default.

OK for me, if you find SHA-512 default useful.  It is just my personal
preference to ask the caller to specify algorithm name explicitly.

(The actual algorithm that a user needs depends upon how the realm is
configured in server.xml/context.xml. I think that not much typing is
saved by having a default here.

I think that many tools such as openssl do not have default algorithm
names in their command line. E.g. I do not see any default for
genpkey command
https://www.openssl.org/docs/apps/genpkey.html
)

I filed an issue for further improvements,
https://issues.apache.org/bugzilla/show_bug.cgi?id=57130

Best regards,
Konstantin Kolinko

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



Re: digest.bat (RealmBase.main()) is broken in current Tomcat 8 trunk. Tomcat 8.0.14 is OK.

2014-10-17 Thread Konstantin Kolinko
2014-09-30 19:22 GMT+04:00 Konstantin Kolinko knst.koli...@gmail.com:
 2014-09-29 14:43 GMT+04:00 Mark Thomas ma...@apache.org:
 On 27/09/2014 15:52, Konstantin Kolinko wrote:
 ()

 4) The current javadoc for RealmBase.main() says that algorithm (-a)
 is not required and If not specified a default of SHA-512 will be
 used.

 I wonder whether that is justified.

 That is what is currently implemented. Happy to discuss changes but
 SHA-512 doesn't seem unreasonable to me.


 I think there is a contradiction between -a algorithm and -h
 credential handler implementation class keys:
 1)  If -h is used I think it shall default to whatever default
 algorithm the credential handler implements.
 2) Custom credential handler implementations may lack setAlgorithm() method.

 I think that one of (-a, -h) is required, with no default for either.
 The old code had no default for algorithm.

 String encoding = UTF-8;

 I think it shall use system encoding, because the value is passed on
 the command line and is not read from file etc.

 The old code used system encoding by default. The system encoding is
 what the system uses, so it is reasonable.

 Note the following text (I am linking to Tomcat 7 docs),
 - Realms and AAA - Common Features - Digested passwords

 http://tomcat.apache.org/tomcat-7.0-doc/realm-howto.html#Digested_Passwords

 [quote]
 Non-ASCII usernames and/or passwords are supported using

 CATALINA_HOME/bin/digest.[bat|sh] -a {algorithm} -e {encoding} {input}

 but care is required to ensure that the non-ASCII input is correctly
 passed to the digester. The digester returns {input}:{digest}. If the
 input appears corrupted in the return, the digest will be invalid.
 [/quote]

 BTW,  That chapter in realm-howto in Tomcat 8 needs an update for the
 new features of digest.sh / RealmBase.main().


I think that this have to be fixed before tagging next Tomcat 8 release.

1. Remove default value for algorithm. Ask the caller to provide
either -a or -h option explicitly.

Motivation:
- Revert to previous behaviour.
- I see contradiction between -a and -h, as I wrote above.

2. Use system default encoding instead of UTF-8 by default.

Motivation:
- Revert to previous behaviour. It makes sense to expect system
encoding when you are calling something from the command line, as that
is the encoding that command line uses.

3. Update realm-howto.html#Digested_Passwords

It does not document the new -h option.

Best regards,
Konstantin Kolinko

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



Re: digest.bat (RealmBase.main()) is broken in current Tomcat 8 trunk. Tomcat 8.0.14 is OK.

2014-10-17 Thread Christopher Schultz
Konstantin,

On 10/17/14 9:13 AM, Konstantin Kolinko wrote:
 2014-09-30 19:22 GMT+04:00 Konstantin Kolinko knst.koli...@gmail.com:
 2014-09-29 14:43 GMT+04:00 Mark Thomas ma...@apache.org:
 On 27/09/2014 15:52, Konstantin Kolinko wrote:
 ()

 4) The current javadoc for RealmBase.main() says that algorithm (-a)
 is not required and If not specified a default of SHA-512 will be
 used.

 I wonder whether that is justified.

 That is what is currently implemented. Happy to discuss changes but
 SHA-512 doesn't seem unreasonable to me.


 I think there is a contradiction between -a algorithm and -h
 credential handler implementation class keys:
 1)  If -h is used I think it shall default to whatever default
 algorithm the credential handler implements.
 2) Custom credential handler implementations may lack setAlgorithm() method.

 I think that one of (-a, -h) is required, with no default for either.
 The old code had no default for algorithm.

 String encoding = UTF-8;

 I think it shall use system encoding, because the value is passed on
 the command line and is not read from file etc.

 The old code used system encoding by default. The system encoding is
 what the system uses, so it is reasonable.

 Note the following text (I am linking to Tomcat 7 docs),
 - Realms and AAA - Common Features - Digested passwords

 http://tomcat.apache.org/tomcat-7.0-doc/realm-howto.html#Digested_Passwords

 [quote]
 Non-ASCII usernames and/or passwords are supported using

 CATALINA_HOME/bin/digest.[bat|sh] -a {algorithm} -e {encoding} {input}

 but care is required to ensure that the non-ASCII input is correctly
 passed to the digester. The digester returns {input}:{digest}. If the
 input appears corrupted in the return, the digest will be invalid.
 [/quote]

 BTW,  That chapter in realm-howto in Tomcat 8 needs an update for the
 new features of digest.sh / RealmBase.main().

 
 I think that this have to be fixed before tagging next Tomcat 8 release.
 
 1. Remove default value for algorithm. Ask the caller to provide
 either -a or -h option explicitly.

 Motivation:
 - Revert to previous behaviour.
 - I see contradiction between -a and -h, as I wrote above.

+1

 2. Use system default encoding instead of UTF-8 by default.
 
 Motivation:
 - Revert to previous behaviour. It makes sense to expect system
 encoding when you are calling something from the command line, as that
 is the encoding that command line uses.

+1

We might want to

a) Allow credentials on stdin to avoid system encoding
b) Instruct the user to use -Dfile.encoding to alter the encoding
instead of using -e

 3. Update realm-howto.html#Digested_Passwords
 
 It does not document the new -h option.

I can fix that, but only after we agree on these changes.

-chris



signature.asc
Description: OpenPGP digital signature


Re: digest.bat (RealmBase.main()) is broken in current Tomcat 8 trunk. Tomcat 8.0.14 is OK.

2014-09-30 Thread Konstantin Kolinko
2014-09-29 14:43 GMT+04:00 Mark Thomas ma...@apache.org:
 On 27/09/2014 15:52, Konstantin Kolinko wrote:
 Hi!

 1). If I run any of the following:
 digest.bat -a foo
 digest.bat -a md5 foo

()

 4) The current javadoc for RealmBase.main() says that algorithm (-a)
 is not required and If not specified a default of SHA-512 will be
 used.

 I wonder whether that is justified.

 That is what is currently implemented. Happy to discuss changes but
 SHA-512 doesn't seem unreasonable to me.


I think there is a contradiction between -a algorithm and -h
credential handler implementation class keys:
1)  If -h is used I think it shall default to whatever default
algorithm the credential handler implements.
2) Custom credential handler implementations may lack setAlgorithm() method.

I think that one of (-a, -h) is required, with no default for either.
The old code had no default for algorithm.

 String encoding = UTF-8;

I think it shall use system encoding, because the value is passed on
the command line and is not read from file etc.

The old code used system encoding by default. The system encoding is
what the system uses, so it is reasonable.

Note the following text (I am linking to Tomcat 7 docs),
- Realms and AAA - Common Features - Digested passwords

http://tomcat.apache.org/tomcat-7.0-doc/realm-howto.html#Digested_Passwords

[quote]
Non-ASCII usernames and/or passwords are supported using

CATALINA_HOME/bin/digest.[bat|sh] -a {algorithm} -e {encoding} {input}

but care is required to ensure that the non-ASCII input is correctly
passed to the digester. The digester returns {input}:{digest}. If the
input appears corrupted in the return, the digest will be invalid.
[/quote]

BTW,  That chapter in realm-howto in Tomcat 8 needs an update for the
new features of digest.sh / RealmBase.main().

Best regards,
Konstantin Kolinko

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



Re: digest.bat (RealmBase.main()) is broken in current Tomcat 8 trunk. Tomcat 8.0.14 is OK.

2014-09-29 Thread Mark Thomas
On 27/09/2014 15:52, Konstantin Kolinko wrote:
 Hi!
 
 1). If I run any of the following:
 digest.bat -a foo
 digest.bat -a md5 foo
 
 I get
 foo:Sep 27, 2014 6:38:43 PM org.apache.catalina.startup.Tool main
 SEVERE: Exception calling main() method
 java.lang.IllegalStateException: Must call init() first

Fixed.

 2) If I run
 digest.bat
 without arguments, I get no output in return.

Fixed.

 4) The current javadoc for RealmBase.main() says that algorithm (-a)
 is not required and If not specified a default of SHA-512 will be
 used.
 
 I wonder whether that is justified.

That is what is currently implemented. Happy to discuss changes but
SHA-512 doesn't seem unreasonable to me.

Mark


-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org