Skip to content

RH2130351: SunPKCS11 PBE API Enhancements #20

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 24 commits into from
Closed

RH2130351: SunPKCS11 PBE API Enhancements #20

wants to merge 24 commits into from

Conversation

@franferrax
Copy link

@franferrax franferrax commented Oct 25, 2022

RH2130351: SunPKCS11 PBE API Enhancements

As part of the work for RHELPLAN-112612 we implemented support for the following PBE algorithms in SunPKCS11:

Algorithm name Derivation Mac Cipher SecretKeyFactory
"PBEWithHmacSHA1AndAES_128" new (PBKDF2) x x
"PBEWithHmacSHA224AndAES_128" new (PBKDF2) x x
"PBEWithHmacSHA256AndAES_128" new (PBKDF2) x x
"PBEWithHmacSHA384AndAES_128" new (PBKDF2) x x
"PBEWithHmacSHA512AndAES_128" new (PBKDF2) x x
"PBEWithHmacSHA1AndAES_256" new (PBKDF2) x x
"PBEWithHmacSHA224AndAES_256" new (PBKDF2) x x
"PBEWithHmacSHA256AndAES_256" new (PBKDF2) x x
"PBEWithHmacSHA384AndAES_256" new (PBKDF2) x x
"PBEWithHmacSHA512AndAES_256" new (PBKDF2) x x
"HmacPBESHA1" old (PKCS12) x x
"HmacPBESHA224" old (PKCS12) x x
"HmacPBESHA256" old (PKCS12) x x
"HmacPBESHA384" old (PKCS12) x x
"HmacPBESHA512" old (PKCS12) x x
"HmacPBESHA512/224" old (PKCS12) x x
"HmacPBESHA512/256" old (PKCS12) x x
"PBKDF2WithHmacSHA1" new (PBKDF2) x
"PBKDF2WithHmacSHA224" new (PBKDF2) x
"PBKDF2WithHmacSHA256" new (PBKDF2) x
"PBKDF2WithHmacSHA384" new (PBKDF2) x
"PBKDF2WithHmacSHA512" new (PBKDF2) x

Since SunJCE SecretKeyFactory implementations are allowed in FIPS (they don't perform any cryptographic operation) we also left them enabled. A SunJCE SecretKeyFactory implementation is used by the PKCS#12 keystore code, through the "PBE" → "PBEWithMD5AndDES" alias, because "PBEWithMD5AndDES" was not implemented in SunPKCS11.

We decided to implement SecretKeyFactory services in SunPKCS11 (for the algorithms already implemented in other services) in the sake of completeness, and thinking in the PBE upstream proposal we are planning to do. Also, we decided that those SecretKeyFactory services should perform key derivation, as with SunJCE's "PBKDF2*" secret key factories, despite the following SunJCE behavior:

During the analyses performed for some behaviours documented in OPENJDK-991 (in the comments), we determined that we need some improvements for SunPKCS11 SecretKeyFactory implementations to be finished and properly interoperable:

  • "*" secret key factories (all):
    • Special invalid PBEKeySpec corner cases in ::generateSecret() (avoid a JNI call into the PKCS#11 token that we know will fail)
      • If itCount < 1, throw InvalidKeySpecException: Iteration count must be a non-zero positive integer
      • If keySize < 1, throw InvalidKeySpecException: Key length must be a non-zero positive integer
      • If keySize % 8 != 0, throw InvalidKeySpecException: Key length (in bits) must be an 8 multiple
    • Review and re-think ::translateKey()
  • "PBKDF2*" secret key factories:
    • Generated keys should keep the original algorithm (currently, they use "Generic"), although the underlying key type will still be CKK_GENERIC_SECRET
    • Those generated keys only work in Mac algorithms and won't work with Cipher algorithms
      • This is because, for the case that concerns us, NSS expects CKK_AES, so we should stop giving the CKA_ENCRYPT attribute to the key in vain (see P11Util.java:64-68 and P11SecretKeyFactory.java:282-285)
      • This is fine, since SunJCE "PBKDF2*" keys don't work either in their Mac nor Cipher services, the common use case seems to be just calling key.getEncoded() to get the derived key bytes (which can also be considered as a salted hash of the password)
  • "HmacPBE*" & "PBEWithHmac*And*" secret key factories:
    • Try to maintain the original algorithm as the key algorithm (currently, we use "Generic" & "AES" respectively)
    • Generated keys, which are already derived, should work in Mac & Cipher services for the exact same algorithm
      • Currently, this doesn't work, we need to skip the key derivation in such cases

The goal of this enhancement is to implement the aforementioned improvements, including any required test adjustment, and creating new test cases for SecretKeyFactory + Cipher and SecretKeyFactory + Mac use cases.

@franferrax franferrax marked this pull request as ready for review October 26, 2022 01:41
@franferrax franferrax marked this pull request as draft October 26, 2022 01:43
@franferrax franferrax marked this pull request as ready for review October 26, 2022 02:01
@franferrax franferrax marked this pull request as draft October 26, 2022 02:01
@franferrax franferrax changed the base branch from fips-17u to master October 27, 2022 22:21
@franferrax franferrax changed the base branch from master to fips-17u October 27, 2022 22:22
@martinuy martinuy self-requested a review November 7, 2022 23:28
@martinuy
Copy link

martinuy commented Nov 7, 2022

Just for reference, I'm working on a review using the following diff: https://github.com/rh-openjdk/jdk/compare/77d0bae2fd2c623002eba4d0e838dcc3c24da50b...baf361ac70e3cb1730ccc9bb914ed5cf9f1d5aa2.diff

martinuy
martinuy reviewed Nov 8, 2022
View reviewed changes
Copy link

@martinuy martinuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I'm sending my initial set of comments. I still need to look at the tests again.

martinuy
martinuy reviewed Nov 10, 2022
View reviewed changes
@franferrax franferrax force-pushed the RH2130351 branch 3 times, most recently from 88803e5 to 28a7e68 Compare December 19, 2022 22:48
@franferrax
Copy link
Author

franferrax commented Dec 20, 2022

Hi @martinuy, I committed all our code review and later pair programming sessions changes. Then I rebased the branch onto fips-17u at 72d08e3. A single conflict had to be fixed for b6bd190 upstream changes, which was already known to us.

Ignoring the rebase, the range of new (review) changes is 3704a54...28a7e68.

I've also run all the tests in test/jdk/sun/security/pkcs11 with jtreg, getting the same results than with the jdk-17.0.5-ga git tag.

franferrax added 14 commits May 8, 2023 19:55
@franferrax
Add missing trace in p11_util.c
bef6ba0
@franferrax
Rename internal helper method
7efaec5 
Cipher algorithms have an 'AesData' suffix as part of their name, so
Mac ones should also have a 'MacData' suffix, to difference them from
SecretKeyFactory-only algorithms (PBKDF2*).
@franferrax
Implement sun.security.pkcs11.P11Key.P11PBEKey
7e3ef9b 
This key implements javax.crypto.interfaces.PBEKey as well as SunJCE's
com.sun.crypto.provider.PBKDF2KeyImpl, which also performs derivation.

The idea behind this is to improve interoperability with SunJCE, as its
SecretKeyFactory will now accept and translate SunPKCS11's P11PBEKey.

Also, this keeps the full PBE information in the key and provides a
quick way to identify SunPKCS11 PBE keys via instanceof.
@franferrax
P11SKF: preserve the algorithm of PBKDF2* keys
25d53d4 
This is aligned with SunJCE's SecretKeyFactory, which keeps the whole
PBKDF2* algorithm name as the generated SecretKey's algorithm.
@franferrax
Introduce itCount and keySize checks
8ee2c3c 
By doing the first two checks in Java, we prevent lower level errors
(which would look severer to the end user, as shown in OPENJDK-991).

The third check is for consistency, we could just go ahead and return a
shorter key as SunJCE does (because of the integer floor-division by 8,
performed as 'keySize >> 3'), but that would later fail somewhere.
@franferrax
Remove P11Util.KDFData.Operation.GENERIC
11358d3 
GENERIC means both ENCRYPTION (Cipher) and AUTHENTICATION (Mac), but
only AUTHENTICATION works where GENERIC is used.

Keys derived with SunPKCS11's SecretKeyFactory for PBKDF2* algorithms
are of type CKK_GENERIC_SECRET. In NSS, these keys work for signature
mechanisms (used by Mac services), but not for symmetric encryption ones
(used by Cipher services). For example, CKM_AES_CBC expects CKK_AES, see
https://github.com/nss-dev/nss/blob/NSS_3_67_RTM/lib/softoken/pkcs11c.c#L1240-L1243

Since CKK_GENERIC_SECRET is the most intuitive key type, let's keep that
one, and prepare the keys only for AUTHENTICATION operations, by just
setting CKA_SIGN=true, since CKA_ENCRYPT=true would be useless.

I've tested SunJCE's SecretKeyFactory derived PBKDF2* keys, and they
don't work for Mac nor Cipher algorithms, so I guess they are only meant
to get their bytes with key.getEncoded(), which is also supported by
SunPKCS11, leveraging the work of openjdk#1.
@franferrax
Replace Operation enum with direct attributes
94006d9 
With the last commit changes, we ended up having a bijection:
     Operation.ENCRYPTION  <->  CK_ATTRIBUTE.ENCRYPT_TRUE
 Operation.AUTHENTICATION  <->  CK_ATTRIBUTE.SIGN_TRUE

We can simplify the code, as we no longer need an enum to hold that
information, which can be stored as a CK_ATTRIBUTE instance.
@franferrax
Make P11Util.KDFData instances immutable
d5fe023 
Add the final qualifier to all the P11Util.KDFData fields, since we
don't expect them to change during the execution.
@franferrax
Improve and align PBAMac and PBECipher tests
d1638c2 
Tolerate NoSuchAlgorithmException only when the provider is SunJCE (i.e.
when computing the expected results).

Reorder fields to reduce differences among tests, align comments'
wording, make testWith() and compute*() methods static.

Transform getCipher() into computeCipherText() for more similarity with
PBAMac, and implement computeExpectedCipherText() for the same reason.
@franferrax
Extend PBAMac and PBECipher tests
8ce6438 
Implement tests for the new scenarios we want to support: use
SunPKCS11's SecretKeyFactory PBE derived keys with SunPKCS11's
Mac or Cipher services.
@franferrax
Reorder code
58da09b 
Extract some variables, reuse others.
@franferrax
Use proper key types when deriving
f65221e 
When passing the CKA_KEY_TYPE attribute to the token, call
getPKCS11KeyType(), so we only get a valid PKCS#11 CKK_* constant.
This allows us to use the "HMAC" key type for HmacPBE* algorithms,
since getPKCS11KeyType("HMAC") returns CKK_GENERIC.

For the attributes' template manager, call getKeyType(), which keeps
SunPKCS11's 'pseudo' key types (PCKK_* constants, such as PCKK_HMAC).

For the Java key, keep the original algorithm of the SecretKeyFactory,
to align with SunJCE behaviour (this also keeps a trace of the algorithm
used to derive the key).
@franferrax
Make P11SecretKeyFactory usable with PBE
a6b72f0 
Perform derivation in P11Mac (for PBE algorithms) and P11PBECipher,
only if the key is not a P11Key.

When proceeding with P11Key instances (which are already derived), the
key is used as is, and P11SecretKeyFactory.convertKey() gets called: in
the P11Mac case, from the same P11Mac instance; in the P11PBECipher
case, from the underlying P11Cipher instance (*).

In P11SecretKeyFactory.convertKey(), handle keys with known PBE
algorithms as a special case: return P11Key instances intact and
derive javax.crypto.interfaces.PBEKey instances.

(*): these keys reach P11SecretKeyFactory.convertKey(), with algo="AES"
and keyAlgo="PBEWithHmac<Hash>And<Cipher>". Also, kdfData.keyAlgo="AES".
@franferrax
Implement TestPBKD translation cases
0bbfa0b 
Perform derivation by translating a javax.crypto.interfaces.PBEKey
instance providing password, salt and iteration count.

PBKDF2* algorithms don't imply a key length in their names, so we can't
perform derivation through translation, because the key length must be
provided through a PBEKeySpec instance, only accepted in key generation.

Please note that the javax.crypto.interfaces.PBEKey instance's
getAlgorithm() method must return the exact algorithm, "PBE" (used in
PBAMac and PBECipher tests) doesn't work since the key algorithm must
be recognized as a SunPKCS11 known PBE algorithm, by having its entry
in P11Util.kdfDataMap.
franferrax and others added 10 commits May 8, 2023 19:56
@franferrax
Rename printByteArray() method
cfa2649 
This method was originally printing a byte[], but at some point we
replaced that with BigInteger, printHex() is more generic.
@franferrax
Implement SunPKCS11's SKF.getKeySpec()
c5ea3db 
This also improves the javax.crypto.interfaces.PBEKey derivation.

If possible, obtain the key length from the encoded key, as SunJCE in
the case of PBKDF2* algorithms, see PBKDF2Core.engineGetKeySpec():
https://github.com/openjdk/jdk17u/blob/jdk-17.0.3-ga/src/java.base/share/classes/com/sun/crypto/provider/PBKDF2Core.java#L101-L102
@franferrax @martinuy
Review: fix P11PBEKey, rename PBES2Helper
8b675d8 
Give a new serialVersionUID to P11PBEKey and return clones of salt and
password. Rename PBES2Helper to PBES2Params and move the DEFAULT_COUNT
and DEFAULT_SALT_LENGTH constants to it (both SunJCE and SunPKCS11 were
using the same values).

Also reset P11Mac in a single point of engineInit and fix some imports
order and whitespace inconsistencies.

Co-Authored-By: Martin Balao <mbalao@redhat.com>
@franferrax @martinuy
Review: implement AlgorithmParameterSpec check
6041151 
Unify this check for Mac and Cipher services. Allow passing both a
consistent PBEParameterSpec (with the derived key's salt and iteration
count) or directly an IvParameterSpec to PBE Cipher services, when
using an already derived P11PBEKey.

Co-Authored-By: Martin Balao <mbalao@redhat.com>
@franferrax @martinuy
Review: integrate KDFData into P11SecretKeyFactory
6b5e4a7 
Move the P11Util.KDFData map to P11SecretKeyFactory, renaming KDFData to
KeyInfo and merging the map with the keyTypes map. KeyInfo has a
PBEKeyInfo specialization when the algorithm it represents is a PBE one.

Implement all the service and key algorithms consistency checks inside
P11SecretKeyFactory.KeyInfo.checkUse.

Store the PBEKeyInfo entry in advance for the service's algorithm in
P11Mac, P11SecretKeyFactory and P11PBECipher (when the algorithm is a
PBE one, which for P11PBECipher this means always).

Keep a single version of P11SecretKeyFactory.derivePBEKey, which now
receives a PBEKeyInfo instance, instead of the algorithm as a string.

Add some checks in P11SecretKeyFactory.convertKey and remove unnecessary
ones.

Partially undo 'Use proper key types when deriving' changes and pass the
native key type to Token.getAttributes, as P11SecretKeyFactory.createKey
does the same. We've also checked that the template system ignores the
passed SunPKCS11 pseudo key type when getting attributes.

Adjust all the remaining code for these changes and make some cosmetic
changes, such as using pattern matching for instanceof (JEP 305) or
reordering some bits in P11SecretKeyFactory.engineGenerateSecret and
P11SecretKeyFactory.engineGetKeySpec.

Co-Authored-By: Martin Balao <mbalao@redhat.com>
@franferrax @martinuy
Review: minor tests adjustments
2cda36a 
Reorder code, rename variables, methods and constants, slightly change
exception handling, inline some small methods.

Co-Authored-By: Martin Balao <mbalao@redhat.com>
@franferrax @martinuy
Review: improve tests assertion data
1687d37 
Implement centralized AssertionData and leverage it to store associated
non-PBE Cipher/Mac algorithms. Test P11PBEKey with non-PBE Cipher/Mac
supported scenarios.

Also, SunJCE assertion data is now calculated just once, with a fixed
configuration (static data is used when SunJCE is not available).

Fix know -> known typo in plainText and adjust static assertion data for
this change.

Co-Authored-By: Martin Balao <mbalao@redhat.com>
@franferrax @martinuy
Review: increase P11SecretKeyFactory coverage
326482d 
Implement empty password cases. Implement translation of a P11PBEKey
from the same token, which must return exactly the same object.
Implement SecretKeyFactory.getKeySpec test.

Co-Authored-By: Martin Balao <mbalao@redhat.com>
@franferrax @martinuy
Review: add P11SecretKeyFactory invalid cases
c430dc4 
Add test cases in which consistency checks must trigger exceptions:
    translateKey
        Non-PBEKey key to PBE SecretKeyFactory
        PBEKey key to PBE SecretKeyFactory of a different algorithm
        Non-AES PBEKey key to AES SecretKeyFactory
        Inconsistent key length between key and algorithm
        Invalid key length in bits
    generateSecret
        Missing salt and iteration count
        Inconsistent key length between spec and algorithm
        Invalid key length in bits
        PBEKeySpec to non-PBE SecretKeyFactory
    getKeySpec
        null KeySpec class
        Invalid key type for PBEKeySpec
        Invalid PBE key and PBEKeySpec for AES SecretKeyFactory

Co-Authored-By: Martin Balao <mbalao@redhat.com>
@franferrax
Fix broken Mac tests outside PBE new ones
709f49e
@franferrax
Copy link
Author

franferrax commented Jun 6, 2023

This has been proposed upstream as part of JDK-8301553: Support Password-Based Cryptography in SunPKCS11

Integrated as openjdk/jdk@4a75fd4

@franferrax franferrax closed this Jun 6, 2023
@franferrax franferrax deleted the RH2130351 branch June 6, 2023 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martinuy martinuy martinuy left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@franferrax @martinuy