Bouncy Castle FIPS Issue

Thu Jul 15 01:28:19 UTC 2021

Hi Brent,

I think I just need some more information.

When you say output byte[] of the KeyAgreement op, where does that come
from? As in what code is producing that? Would you point me at the file
that's calling the KDFConcat derive() method - referring to the source
is generally the least ambiguous way of sorting these things out.

Thanks,

David

On 15/7/21 10:55 am, Brent Putman wrote:
>
> Hi,
>
> Either I don't think I understand, or maybe you misread our code. 
> AFAIK the KeyAgreement service interface requires the 2 Keys over
> which the key agreement is being performed. For example:
>
> keyAgreement = KeyAgreement.getInstance(agreementAlgo);
> keyAgreement.init(privateKey);
> keyAgreement.doPhase(publicKey, true);
>
> Our KeyDerivation impls like ConcatKDF do not have access to any of
> the key agreement key material.  They are about key derivation from an
> input byte[] only (i.e. the output byte[] of the KeyAgreement op). 
> The do not and can not have access to any key agreement key material,
> because it's out-of-scope for what they do.  So I don't see how we can
> re-implement our ConcatKDF "SecretKey derive(...)" method using the
> KeyAgreement service interface as you suggest.
>
> Please clarify if I am missing something here.
>
> Thanks,
> Brent
>
>
> On 7/14/21 8:21 PM, David Hook wrote:
>>
>> Hi Brent,
>>
>> It will work. The naming convention is also to allow mixing and
>> matching, you just need to add:
>>
>> org.bouncycastle.jcajce.spec.UserKeyingMaterialSpec
>>
>> as a parameter to the KeyAgreement. It's unfortunate that the JCE
>> doesn't define this parameter, so I am not surprised you are not
>> aware of it, but it's impossible to use key agreement properly
>> without it, and it exists in both BC and BCFIPS specifically for
>> situations like this.
>>
>> You use the UserKeyingMaterialSpec to pass in otherInfo, which I can
>> see is being constructed in accordance with SP 800-56A which is the
>> FIPS standard. There is some BC support for otherInfo but we are
>> using the ASN.1 construction, SP 800-56A allows for several others,
>> and I'm not sure what the XML standard settled on. You are probably
>> better off sticking to what you are doing now.
>>
>> For our part I think we'd just need to add some additional KDF
>> support for the non-NIST digests. In the case of the SAML code it
>> would just be a matter of replacing SecretKey derive() I think and
>> allowing for the UserKeyingMaterialSpec construction.
>>
>> Concerning  your comments about BC and BCFIPS. Yes the low level
>> libraries are different, they have to be, but we've been able to keep
>> the JCE layer in sync since 1.58. The upside of this is it means if
>> people swap in BCFIPS rather than BC they can be confident the code
>> is then FIPS compliant. If you interested in finding out why the
>> difference exists, there is actually a document
>>
>> https://www.bouncycastle.org/fips/BCFipsDescription.pdf
>>
>> which provides a summary. I would recommend that over reading the
>> FIPS IG and the 30 or so associated documents that go with it unless
>> you have a specific interest. The same applies for Common Criteria.
>>
>> I don't think there will be any end-user problems as the end result
>> will be a massive broadening of what the OpenSAML project can be used
>> for.
>>
>> Any further questions or issues, please let me know.
>>
>> Thanks,
>>
>> David
>>
>> On 15/7/21 8:59 am, Brent Putman wrote:
>>>
>>> Hi David,
>>>
>>> I took a look and unfortunately what you propose won't work, for at
>>> least 2 reasons.
>>>
>>> First (the main and show-stopping technical issue), those
>>> provider-based algorithms you mention are apparently for the
>>> KeyAgreement service of the Java security framework. It seems those
>>> combine the key agreement op with the KDF op all in one go?
>>>
>>> That won't work for our design because we're modeling the
>>> requirements of XML Encryption 1.1 where the KA and KDF ops are
>>> conceptually completely separate. They are defined separately, have
>>> distinct XML representations, etc, so one can mix-and-match KA algo
>>> and KDF algo.
>>>
>>> So in OpenSAML we have distinct KA and KDF interfaces and then
>>> various impls for each. For example we have interface
>>> org.opensaml.xmlsec.derivation.KeyDerivation.  The ConcatKDF impl of
>>> that in question that threw a NoClassDefFoundError with FIPS was our
>>> org.opensaml.xmlsec.derivation.impl.ConcatKDF, which imports these
>>> KDF-related classes (along with some needed BC digest classes):
>>>
>>> import org.bouncycastle.crypto.agreement.kdf.ConcatenationKDFGenerator;
>>> import org.bouncycastle.crypto.params.KDFParameters;
>>>
>>> It was specifically throwing on not finding
>>> org.bouncycastle.crypto.DerivationParameters, which is the interface
>>> implemented by the above BC KDFParameters.  For reference that
>>> source is here:
>>>
>>> http://git.shibboleth.net/view/?p=java-opensaml.git;a=blob;f=opensaml-xmlsec-impl/src/main/java/org/opensaml/xmlsec/derivation/impl/ConcatKDF.java;hb=refs/heads/main
>>>
>>> By contrast, for PBKDF2 we are using the SecretKeyFactory service
>>> with the JDK-provided algorithm support for "PBKDF2With*" (completed
>>> with the appropriate PRF algo ID).  So if the BC provider had a
>>> SecretKeyFactory impl for ConcatKDF by itself, that would be closer
>>> to our needs. But....
>>>
>>> Second, it's unlikely we could introduce a deployment requirement
>>> for people to add the BC provider declaratively in java.security and
>>> also questionable for us to add the BC provider programmatically by
>>> default.  As a library OpenSAML probably shouldn't muck with
>>> people's environments like that.
>>>
>>> And in XML Encryption 1.1, ConcatKDF is the mandatory-to-implement
>>> KDF (PBKDF2 is optional).  For that reason it is our default KDF for
>>> ECDH, and it needs to work out-of-the-box.  So hopefully you can see
>>> that adding a requirement on configuring a third-party provider
>>> would be an issue for us.  Our team can discuss further the idea of
>>> automagically/programmatically adding in BC.  I personally am not a
>>> fan of the idea.  But unless/until BC has a provider-based impl of
>>> the ConcatKDF by itself, it's essentially a moot question.
>>>
>>> I guess what I would ask about the FIPS version is:  If the provider
>>> there fundamentally supports ConcatKDF via the KeyAgreement
>>> algorithms you mention, then why aren't the KDF interfaces/impls
>>> there as well?  Probably they are there but just with different
>>> package and/or class names, etc?  If they are, then you know, that
>>> means the FIPS version really isn't a drop-in replacement for the
>>> regular library, so it's going to be challenging for us as a
>>> downstream consumer to overcome that.
>>>
>>> Thanks,
>>> Brent
>>>
>>>
>>>
>>> On 7/13/21 10:30 PM, David Hook wrote:
>>>> Hi Brent,
>>>>
>>>> I'd like to suggest that it gets changed to use the BC provider
>>>> rather than the BC low-level API.
>>>>
>>>> For some reason I can't connect to any of the nabble references, so
>>>> I can't easily find the source file concerned, but from what I
>>>> understand V4.1 added support for the ConcatKDF key derivation
>>>> function with ECDH. When I checked the code this looked like
>>>> something the BCFIPS provider supports already (and the BC
>>>> provider) for the JCE. The agreement algorithms ending in CKDF use
>>>> the concat KDF function.
>>>>
>>>> The following ones are currently available in both providers:
>>>>
>>>> ECCDHWITHSHA1CKDF
>>>> ECCDHWITHSHA256CKDF
>>>> ECCDHWITHSHA384CKDF
>>>> ECCDHWITHSHA512CKDF
>>>>
>>>> I have subscribed to the dev list if you would prefer to continue
>>>> the discussion there. If you would include the link to the source
>>>> file either way we should be able to work something out that will
>>>> work for both providers (as I have commit access to both, you'd
>>>> hope so!).
>>>>
>>>> Let me know,
>>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>> On 14/7/21 7:17 am, Brent Putman wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> Sorry David (Hook) that I didn't get back to you when I returned
>>>>> from vacation a couple of weeks ago, I unfortunately overlooked
>>>>> that email todo.
>>>>>
>>>>> I'm interested to hear what you had in mind.  If possible it would
>>>>> be great to have this discussion over on the Shibboleth/OpenSAML
>>>>> developers list, so that other members of the team and community
>>>>> can be aware and participate:
>>>>>
>>>>> https://shibboleth.net/mailman/listinfo/dev
>>>>>
>>>>> Thanks,
>>>>> Brent
>>>>>
>>>>>
>>>>> On 7/9/21 11:54 AM, David Castro wrote:
>>>>>> Brent,
>>>>>>
>>>>>> Just wanted to follow up Re: the following thread on the FIPS
>>>>>> issue we were having.
>>>>>>
>>>>>> https://shibboleth.1660669.n2.nabble.com/Bouncy-Castle-FIPS-Issue-td7649455.html#a7649653
>>>>>> <https://shibboleth.1660669.n2.nabble.com/Bouncy-Castle-FIPS-Issue-td7649455.html#a7649653>
>>>>>>
>>>>>> We spoke with David Hook from Bouncy Castle (included in this
>>>>>> thread) and he had some ideas on resolving the issue. Scott
>>>>>> Cantor recommended you as the point of contact, but let us know
>>>>>> if there is someone else we should reach out to.
>>>>>>
>>>>>> We are also available to assist as needed.
>>>>>>
>>>>>> Cheers,
>>>>>> David
>>>>>>
>>>>>> -- 
>>>>>> CONFIDENTIALITY NOTICE: This transmission, and any attachments,
>>>>>> may contain CONFIDENTIAL, PRIVILEGED or PROPRIETARY information
>>>>>> of HRworx, LLC (dba Intelliworx) that is protected from
>>>>>> disclosure under applicable laws. If you are not the intended
>>>>>> recipient, any disclosure, copying, distribution, or use of any
>>>>>> of the information contained in or attached to this transmission
>>>>>> is STRICTLY PROHIBITED. If you have received this communication
>>>>>> in error, please notify the sender, by reply e-mail, and delete
>>>>>> the original message with any attachments. Thank you for your
>>>>>> cooperation.
>>>>
>>>>
>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://shibboleth.net/pipermail/dev/attachments/20210715/acb0cabb/attachment-0001.htm>