Tim, On 4/15/14, 5:30 PM, Tim Whittington wrote: > > On 16/04/2014, at 8:30 am, Christopher Schultz <ch...@christopherschultz.net> > wrote: > >> 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. > > OK, I had a look at the TCN implementation and see it doesn’t do much. > Maybe it’d be cleaner to deal with the openssl API there, and thunk the > possible values in SSL.fipsModeGet/Set to 0/1.
Please file a bug. It doesn't seem high-priority to make that change
right now. It also will require a change to tcnative which generally has
a slower release cycle.
>>>> +
>>>> + 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.
>
> I was more thinking about making the SSL.fipsMode* API accept/return only
> 0/1, which would make these comments unnecessary.
> Sorry, wasn’t super-clear on that.
Okay.
>>>> + 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 read 56027 and your description of the options, but it doesn’t
> explain why those options are useful beyond a simple “I want FIPS”, “I
> don’t want/care about FIPS” toggle.
>
> i.e. enter and require both enable FIPS mode if successful
No, "require" will not ever enter FIPS mode: it requires that OpenSSL
already be in FIPS mode. If it's not, it will fail. "enter" will enter
FIPS mode if necessary, but skip the FIPS_mode_enter call if we're
already in FIPS mode.
> so what do you gain by being able to ensure that something prior to
> Tomcat startup has or hasn’t already enabled FIPS mode?
You may want to validate that the system is in the state you expect:
FIPS-mode already enabled. Perhaps you don't want to start-up if that's
not the case. It's mostly a sanity check.
> (I haven’t dealt with a lot of FIPS site requirements, so there may
> well be something I’m missing here).
The original report was that calling FIPS_mode_set(1) when the library
is already in FIPS mode causes OpenSSL complain and the
AprLifecycleListener to fail to start. This basically makes it not
possible to "ensure" the server is in FIPS mode. Since I was going to
modify the behavior, I figured I would give the user some options.
Do you think it makes things too confusing?
Most people will just use FIPSmode="on" which is completely
backwards-compatible -- except that this one failure scenario is now
eliminated. The "enter" and "require" options were opportunities I
thought made sense to take.
>>> 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.
>
> If the server was booted in FIPS mode and I explicitly (implicitly?)
> set FIPSMode=“off” then it would be unexpected for openssl to be
> limiting the connector to FIPS mode cipher suites.
I wouldn't say that /not/ specifying FIPSMode should be the same as
FIPSMode="off". I think you should have to explicitly disable FIPSMode.
Honestly, I can't imagine anyone intentionally disabling FIPS mode, but
I suppose if I'm going to bother to support some ... esoteric options,
then explicitly /exiting/ FIPS mode could be one of them.
Does this feel very important to you? Nobody ever asked for
FIPS-mode-exit, while the options I've added with this patch do address
some reasonable use-cases that have been brought up by a real person
asking for help.
>>> 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.
>
> This sounds like ‘require’ means ‘verify the server was booted in
> FIPS mode', which sounds reassuring but not something that affects
> how Tomcat will operate?
Well, it will cause the AprLifecycleListener to fail if OpenSSL isn't
already in FIPS mode, so the connector won't work properly. So it's both
reassuring and will affect how Tomcat will operate.
-chris
signature.asc
Description: OpenPGP digital signature
