Re: [PATCH V2 3/5] ib-diags/saquery: Fix smkey handling

[Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

On 4/25/2012 7:24 PM, Jim Foraker wrote:
> 
> On Wed, 2012-04-25 at 05:57 -0700, Hal Rosenstock wrote:
>> On 4/24/2012 7:42 PM, Jim Foraker wrote:
>>>
>>> On Tue, 2012-04-24 at 07:17 -0700, Hal Rosenstock wrote:
>>>> On 4/23/2012 8:56 PM, Jim Foraker wrote:
>>>>> smkey is already defined as a global inside saquery.c, so remove
>>>>> broken support for passing it around as a function parameter
>>>>>
>>>>> Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
>>>>> ---
>>>>>  src/saquery.c |   59 ++++++++++++++++++++++++++++-----------------------------
>>>>>  1 file changed, 29 insertions(+), 30 deletions(-)
>>>>>
>>>>> diff --git a/src/saquery.c b/src/saquery.c
>>>>> index e5fdb25..029228c 100644
>>>>> --- a/src/saquery.c
>>>>> +++ b/src/saquery.c
>>>>> @@ -85,7 +85,7 @@ struct query_cmd {
>>>>>  
>>>>>  static char *node_name_map_file = NULL;
>>>>>  static nn_map_t *node_name_map = NULL;
>>>>> -static uint64_t smkey = 1;
>>>>> +static uint64_t smkey = 0;
>>>>
>>>> Why is the default for smkey being changed from 1 to 0 ? Note that even
>>>> though the name is smkey (due to the spec), it is really the default SA key.
>>>      Previous to the patch, smkey was defined as 1, but rarely passed
>>> thru to functions.  In particular, the only SA requests that were using
>>> the default value of 1 were MCMember records, via either the -m option
>>> or the MCMR query.  All other types were hard coded to smkey values of
>>> 0, and hence executing untrusted SA requests, regardless of either the
>>> smkey variable defaulting to 1 or of any "--smkey" being passed on the
>>> command line.
>>
>> In addition to MCMemberRecords, trust is supported for
>> P_KeyTableRecords, PortInfoRecords, and ServiceRecords AFAIT. Patch
>> shortly for InformInfoRecords and InformInfo.
>      Trust may be implemented in OpenSM, but it is not being used by the
> diags.  The vast majority of the patch deals with removing the smkey or
> trusted parameters to calls to get_{all,any}_records() and
> get_and_dump_{all,any}_records().  In almost every one of those cases, a
> value of "0" was being passed in the trusted/smkey field, either of
> which results in sa_query() being called with the smkey set to 0 --
> hence, an untrusted request.
>      In addition, get_and_dump_all_records() did not pass the trusted
> parameter on when it called get_all_records(), so it _always_ generates
> untrusted requests.
> 
>>
>>>      Changing the variable default to 0 causes the patch to effect the
>>> least change in observable behavior.  
>>
>> Not sure what you mean by that.
>      What I mean is that if we leave smkey at 0 in the patch, the output
> that users of saquery will see (without passing a smkey on the command
> line) will change very little post-patch -- only MCMember records will
> change.
>      If we apply the patch but with smkey changed to 1, users will see
> far more new behavior.  If the subnet is partitioned, they will see more
> results for every record type other than MCMember than they did
> previously.  They will also begin to see valid Mkeys, ServiceKeys, and
> QPNs in their results.  Conversely, if they have changed their SA smkey
> away from 1, they will get no results whatsoever, from any request,
> until they pass an smkey on the command line.
> 
>>> In particular, for commands not
>>> specifying a smkey on the command line, the visible changes are limited
>>> to MCMember records.  For subnets not using partitioning, the change in
>>> MCMember records is limited to the specifics for that record type
>>> covered in C15-0.2-1.16.
>>
>> Even without partitioning, this is important for validating all members
>> in MC group (for IPoIB debug).
>      That information would still be available, it just would require
> configuring/passing the right smkey.
> 
>>
>> It also affects the previously aforementioned SA records as well.
>      It only affects them if the user passes an smkey.  Currently, all
> record types other than MCMember are queried for with an smkey of 0,
> even if smkey is passed on the command line.  Fixing the handling and
> then changing the default to 0 means most SA records are still queried
> with an smkey of 0, resulting in identical behavior as before, but now
> the user can actually usefully change what smkey is used.
> 
>>
>>>      Setting the default to 1 may provide better behavior, in terms of
>>> making the diags "just work" for an out-of-the-box OpenSM config, 
>>
>> Yes, that was the original intention.
>>
>>> but it
>>> seems to me that the continued existence of this bug shows that
>>> authenticated requests might not be particularly important for simple
>>> configs.  Plus, it extracts a penalty -- post-patch, if the default is
>>> set to 1, a user who chooses to change their SA smkey will be penalized
>>> in the sense that they will always need to pass an smkey on the command
>>> line, either the correct one or "--smkey 0" to execute an untrusted
>>> request (packets with incorrect smkeys are dropped, not considered
>>> untrusted).  
>>
>> That is the tradeoff and it was the decision made. I can dig out the
>> threads on the list if need be.
>>
>>> With a default of 0, we are not providing users
>>> encouragement to leave their SA smkey (which in turn protects other
>>> authentication keys on the fabric) at a well-known, insecure value.
>>
>> Yes, it comes down ease of use v. "security". Also, changing this now
>> becomes a flag day/backward compatibility issue (at least in terms of
>> support).
>      How does this warrant a flag day?  It's a bug fix to a query
> utility, which as currently implemented, preserves most current
> behavior, and in the two cases it doesn't, the current behavior can be
> recaptured by passing one command line parameter.

I was referring to the user/admin expectation of seeing all
MCMemberRecords without supplying smkey and now to get that he will need
a different command. Changes like these cause confusion (and support).

I didn't realize about the other lacking saquery support for trust in
the other SA records. That definitely changes the picture.

So the question seems to be whether or not to preserve the original user
experience in terms of MCMemberRecord behavior and if so, how to
accomplish that. I think that might mean an additional policy (like
smkey_mcmr).

-- Hal

> 
>      Jim
> 
>>
>>>      A compromise would be for someone to write a patch that adds
>>> support for a default SA smkey to the diags config file.
>>
>> Makes sense.
>>
>>> In that case, I think the right behavior would be for the compiled utility to still
>>> default to 0 so that saquery works on hosts without an smkey set in the
>>> conf (the default config file might set the value to 1), which means
>>> this patch as written does not get in the way.
>>
>> OK but IMO we shouldn't change the smkey value here until this occurs.
>>
>> -- Hal
>>
>>>      Jim
>>>
>>>>
>>>> -- Hal
>>>>
>>>> <snip...>
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html