Tim, On 4/14/14, 10:45 PM, Tim Whittington wrote: > > On 15/04/2014, at 1:14 pm, schu...@apache.org wrote: > >> Author: schultz >> Date: Tue Apr 15 01:14:40 2014 >> New Revision: 1587379 >> >> URL: http://svn.apache.org/r1587379 >> Log: >> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=56027 >> Add more nuanced support for entering/requiring FIPS mode when using APR >> connector. >> >> Modified: >> tomcat/trunk/java/org/apache/catalina/core/AprLifecycleListener.java >> tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties >> tomcat/trunk/java/org/apache/tomcat/jni/SSL.java >> tomcat/trunk/webapps/docs/config/listeners.xml >> >> Modified: >> tomcat/trunk/java/org/apache/catalina/core/AprLifecycleListener.java >> URL: >> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/AprLifecycleListener.java?rev=1587379&r1=1587378&r2=1587379&view=diff >> ============================================================================== >> --- tomcat/trunk/java/org/apache/catalina/core/AprLifecycleListener.java >> (original) >> +++ tomcat/trunk/java/org/apache/catalina/core/AprLifecycleListener.java Tue >> Apr 15 01:14:40 2014 >> @@ -70,6 +70,18 @@ public class AprLifecycleListener >> protected static boolean aprInitialized = false; >> protected static boolean aprAvailable = false; >> protected static boolean fipsModeActive = false; >> + /** >> + * FIPS_mode documentation states that the return value will be >> + * whatever value was originally passed-in to FIPS_mode_set(). >> + * FIPS_mode_set docs say the argument should be non-zero to enter >> + * FIPS mode, and that upon success, the return value will be the >> + * same as the argument passed-in. Docs also highly recommend >> + * that the value "1" be used "to avoid compatibility issues". >> + * In order to avoid the argument and check-value from getting out >> + * of sync for some reason, we are using the class constant >> + * FIPS_ON here. >> + */ >> + private static final int FIPS_ON = 1; > > > This looks like it belongs in SSL (as one of the valid return values of > SSL.fipsModeGet).
Fair enough.
> Also, does FIPS_mode() always return 1 if FIPS mode is enabled at boot (I see
> there’s no adaptation of the return code in the TCN imll).
FIPS_mode() is documented to return whatever was passed to it before.
There is no other information that I could find available. I'm willing
to bet that everyone always uses "1" since the API basically says "use 1".
>> protected static final Object lock = new Object();
>>
>> @@ -110,7 +122,7 @@ public class AprLifecycleListener
>> }
>> }
>> // Failure to initialize FIPS mode is fatal
>> - if ("on".equalsIgnoreCase(FIPSMode) && !isFIPSModeActive())
>> {
>> + if (!(null == FIPSMode || "off".equalsIgnoreCase(FIPSMode))
>> && !isFIPSModeActive()) {
>> Error e = new Error(
>> sm.getString("aprListener.initializeFIPSFailed"));
>> // Log here, because thrown error might be not logged
>> @@ -252,13 +264,59 @@ public class AprLifecycleListener
>> method = clazz.getMethod(methodName, paramTypes);
>> method.invoke(null, paramValues);
>>
>> - if("on".equalsIgnoreCase(FIPSMode)) {
>> + final boolean enterFipsMode;
>> +
>> + if("on".equalsIgnoreCase(FIPSMode)
>> + || "require".equalsIgnoreCase(FIPSMode)) {
>> + // FIPS_mode documentation states that the return value will be
>> + // whatever value was originally passed-in to FIPS_mode_set().
>> + // FIPS_mode_set docs say the argument should be non-zero to
>> enter
>> + // FIPS mode, and that upon success, the return value will be
>> the
>> + // same as the argument passed-in. Docs also highly recommend
>> + // that the value "1" be used "to avoid compatibility issues".
>> + // In order to avoid the argument and check-value from getting
>> out
>> + // of sync for some reason, we are using the class constant
>> + // FIPS_ON here.
>> + final int fipsModeState = SSL.fipsModeGet();
>
>
> I don’t think this needs to be documented twice, and maybe not at all?
> Aren’t we already handling the FIPS_* ‘peculiarities' in TCN so that
> SSL.fipsModeGet() has a sane 0/1 return code?
No. Other code could call FIPS_mode_set() and thus the return value
might be different. Yes, this is documented identically in multiple
places. I didn't want anyone to say "oh, having a constant for (int)1 is
stupid: let's just use 1 and be done with it" without actually reading
why there is a constant being used there.
>> +
>> + if(log.isDebugEnabled())
>> + log.debug(sm.getString("aprListener.currentFIPSMode",
>> + Integer.valueOf(fipsModeState)));
>> +
>> + // Return values: 0=Not in FIPS mode, 1=In FIPS mode,
>> + // exception if FIPS totally unavailable
>> + enterFipsMode = 1 != fipsModeState;
>> +
>> + if("on".equalsIgnoreCase(FIPSMode)) {
>> + if(!enterFipsMode)
>> +
>> log.info(sm.getString("aprListener.skipFIPSInitialization"));
>> + } else if("require".equalsIgnoreCase(FIPSMode)) {
>> + if(enterFipsMode) {
>> + String message =
>> sm.getString("aprListener.alreadyInFIPSMode");
>> + log.error(message);
>> + throw new IllegalStateException(message);
>> + }
>> + }
>> + }
>> + else if("enter".equalsIgnoreCase(FIPSMode)) {
>> + enterFipsMode = true;
>> + } else
>> + enterFipsMode = false;
>> +
>> + if(enterFipsMode) {
>> log.info(sm.getString("aprListener.initializingFIPS"));
>>
>> - int result = SSL.fipsModeSet(1);
>> + // FIPS_mode_set docs say the argument should be non-zero to
>> enter
>> + // FIPS mode, and that upon success, the return value will be
>> the
>> + // same as the argument passed-in. Docs also highly recommend
>> + // that the value "1" be used "to avoid compatibility issues".
>> + // In order to avoid the argument and check-value from getting
>> out
>> + // of sync for some reason, we are using the class constant
>> + // FIPS_ON here.
>
>
> Again, these docs look like they belong on SSL.fipsModeSet or in the TCN
> implementation of that.
This can't be in TCN because the Java code needs the constant value. I
suppose I could call a function in tcnative to get the value of "1" but
that seems a bit of overkill and I'd have to wait for yet another
tcnative release to get this out there.
>> + final int result = SSL.fipsModeSet(FIPS_ON);
>>
>> - // success is defined as return value = 1
>> - if(1 == result) {
>> + // success is defined as return value = last argument to
>> FIPS_mode_set()
>> + if(FIPS_ON == result) {
>> fipsModeActive = true;
>>
>> log.info(sm.getString("aprListener.initializeFIPSSuccess"));
>>
>> Modified: tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties
>> URL:
>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties?rev=1587379&r1=1587378&r2=1587379&view=diff
>> ==============================================================================
>> --- tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties
>> (original)
>> +++ tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties Tue
>> Apr 15 01:14:40 2014
>> @@ -57,6 +57,9 @@ aprListener.aprDestroy=Failed shutdown o
>> aprListener.sslInit=Failed to initialize the SSLEngine.
>> aprListener.tcnValid=Loaded APR based Apache Tomcat Native library {0} using
>> APR version {1}.
>> aprListener.flags=APR capabilities: IPv6 [{0}], sendfile [{1}], accept
>> filters [{2}], random [{3}].
>> +aprListener.currentFIPSMode=Current FIPS mode: {0}
>> +aprListener.skipFIPSInitialization=Already in FIPS mode; skipping FIPS
>> initialization.
>> +aprListener.alreadyInFIPSMode=AprLifecycleListener requested to force
>> entering FIPS mode, but FIPS mode was already enabled.
>> aprListener.initializingFIPS=Initializing FIPS mode...
>> aprListener.initializeFIPSSuccess=Successfully entered FIPS mode
>> aprListener.initializeFIPSFailed=Failed to enter FIPS mode
>>
>> Modified: tomcat/trunk/java/org/apache/tomcat/jni/SSL.java
>> URL:
>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/jni/SSL.java?rev=1587379&r1=1587378&r2=1587379&view=diff
>> ==============================================================================
>> --- tomcat/trunk/java/org/apache/tomcat/jni/SSL.java (original)
>> +++ tomcat/trunk/java/org/apache/tomcat/jni/SSL.java Tue Apr 15 01:14:40 2014
>> @@ -230,6 +230,14 @@ public final class SSL {
>> public static native int initialize(String engine);
>>
>> /**
>> + * Get the status of FIPS Mode.
>> + *
>> + * @return 0 If OpenSSL is not in FIPS mode, 1 if OpenSSL is in FIPS
>> Mode.
>> + * @throws Exception If tcnative was not compiled with FIPS Mode
>> available.
>> + */
>> + public static native int fipsModeGet();
>> +
>> + /**
>> * Enable/Disable FIPS Mode.
>> *
>> * @param mode 1 - enable, 0 - disable
>>
>> Modified: tomcat/trunk/webapps/docs/config/listeners.xml
>> URL:
>> http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/config/listeners.xml?rev=1587379&r1=1587378&r2=1587379&view=diff
>> ==============================================================================
>> --- tomcat/trunk/webapps/docs/config/listeners.xml (original)
>> +++ tomcat/trunk/webapps/docs/config/listeners.xml Tue Apr 15 01:14:40 2014
>> @@ -112,12 +112,22 @@
>> </attribute>
>>
>> <attribute name="FIPSMode" required="false">
>> - <p>Set to <code>on</code> to instruct OpenSSL to go into FIPS mode.
>> + <p>Set to <code>on</code> to request that OpenSSL be in FIPS mode
>> + (if OpenSSL is already in FIPS mode, it will remain in FIPS mode).
>> + Set to <code>enter</code> to force OpenSSL to enter FIPS mode (an
>> error
>> + will occur if OpenSSL is already in FIPS mode).
>> + Set to <code>require</code> to require that OpenSSL <i>already</i>
>> be
>> + in FIPS mode (an error will occur if OpenSSL is not already in FIPS
>> + mode).
>> FIPS mode <em>requires you to have a FIPS-capable OpenSSL library which
>> you must build yourself</em>.
>> - FIPS mode also requires Tomcat native library version 1.1.23 or
>> later,
>> - which <em>must be built against the FIPS-compatible OpenSSL</em>
>> library.
>> - If this attribute is "on", <b>SSLEngine</b> must be enabled as well.
>> + <code>FIPSMode="on"</code> or <code>FIPSMode="require"</code>
>> requires
>> + Tomcat native library version 1.1.30 or later, while
>> + <code>FIPSMode="enter"</code> can probably be done with Tomcat
>> native
>> + library version 1.2.23 or later -- either of which <em>must be built
>> + against the FIPS-compatible OpenSSL</em> library.
>> + If this attribute is set to any of the above values,
>> <b>SSLEngine</b>
>> + must be enabled as well for any effect.
>> The default value is <code>off</code>.</p>
>> </attribute>
>>
>>
>
>
> Why the ‘enter’ and ‘require’ options?
Please read the enhancement request where I've described the reasoning
behind this.
> I can understand ‘on’ == ‘I want FIPS mode’, but I don’t get how
> ‘require’ == ‘I require FIPS mode turned on before Tomcat starts’
> affects Tomcat, and I don’t get how ‘enter’ == ‘Try FIPS mode or fail
> if already set’ is useful.
>
> Also, it seems inconsistent given the above that ‘off’ does nothing
> if TCN is already in FIPS mode at startup?
I can change that if you feel it is important.
> Current behaviour would appear to be ‘on’ == ‘make sure it’s on *with
> bug’, ‘off’ == ‘don’t really care, will take whatever’?
Correct.
> Would on/off still be sufficient, or do we need the four ‘require’,
> ‘support’, ‘not supported’, ‘initiate’ options ala transactions?
I like being able to ensure that FIPS mode was already enabled.
-chris
signature.asc
Description: OpenPGP digital signature
