tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher Schultz <ch...@christopherschultz.net>
Subject Re: svn commit: r1587379 - in /tomcat/trunk: java/org/apache/catalina/core/AprLifecycleListener.java java/org/apache/catalina/core/LocalStrings.properties java/org/apache/tomcat/jni/SSL.java webapps/docs/config/listeners.xml
Date Tue, 15 Apr 2014 21:47:07 GMT
Tim,

On 4/15/14, 5:30 PM, Tim Whittington wrote:
> 
> On 16/04/2014, at 8:30 am, Christopher Schultz <chris@christopherschultz.net> wrote:
> 
>> Tim,
>>
>> On 4/14/14, 10:45 PM, Tim Whittington wrote:
>>>
>>> On 15/04/2014, at 1:14 pm, schultz@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


Mime
View raw message