jdk
jdk copied to clipboard
openjdk
8325513: Export method for Cipher
Add Cipher::exportKey API.
Progress
- [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change requires a CSR request matching fixVersion 26 to be approved (needs to be created)
Issue
- JDK-8325513: Export method for Cipher (Enhancement - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18409/head:pull/18409
$ git checkout pull/18409
Update a local copy of the PR:
$ git checkout pull/18409
$ git pull https://git.openjdk.org/jdk.git pull/18409/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18409
View PR using the GUI difftool:
$ git pr show -t 18409
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18409.diff
Using Webrev
:wave: Welcome back weijun! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.
@wangweij The following label will be automatically applied to this pull request:
security
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.
Webrevs
- 06: Full - Incremental (2da969c0)
- 05: Full - Incremental (9f9cd651)
- 04: Full - Incremental (cd02570c)
- 03: Full - Incremental (11701dce)
- 02: Full - Incremental (bbd97a30)
- 01: Full - Incremental (b8658a76)
- 00: Full (8834f04e)
Why is the algorithm necessary for this new method? Couldn't the new SecretKey take the algorithm from the original SecretKey stored in the Cipher object?
One use case for this method is HPKE key export. Obviously, the exported key won't have algorithm name being "HPKE".
One use case for this method is HPKE key export. Obviously, the exported key won't have algorithm name being "HPKE".
If Cipher::init is passed an AES SecretKey, wouldn't the Cipher::export return an AES SecretKey? I don't see the case where it is anything but the original key's algorithm.
This concern comes from the lack of a requirements on value being an algorithm name, and our provider usage is inconsistent with it to. If my memory is correct, SunJCE requires "AES", but other algorithms we don't. So having the user define this value leaves a grey area where users may enter anything. Or in the worse case, the developer not knowing what the key algorithm is (AES or CC20) and needs to keep track of it.
I don't know if any AES cipher defines an export function. As I said earlier the reason I introduce this method is for HPKE. In HPKE, the cipher algorithm name is "HPKE" and the keys used in initialization are asymmetric keys. While the 3rd step in HPKE is an AEAD cipher, I cannot guarantee the export function would only generate a key for this AEAD algorithm. The fact that a length is needed for the function means the output could be anything. Also, there is an export-only mode that does not require an AEAD cipher at all.
In fact, the original definition of export function returns a byte array. I choose the SecretKey return type here simply because it could be non extractable. I am now wondering if there should be also an exportData method.
If this is an HPKE operation, shouldn't it be part of an HPKE API? Why part of Cipher?
I don't think it's worth inventing a new cryptographic engine for HPKE and it is a cipher.
var g = KeyPairGenerator.getInstance("X25519");
var kp = g.generateKeyPair();
var sender = Cipher.getInstance("HPKE");
var params = HPKEParameterSpec.of()
.info("this_info".getBytes(StandardCharsets.UTF_8));
sender.init(Cipher.ENCRYPT_MODE, kp.getPublic(), params);
var receiver = Cipher.getInstance("HPKE");
receiver.init(Cipher.DECRYPT_MODE, kp.getPrivate(), params.encapsulation(sender.getIV()));
var msg = "Hello World".getBytes(StandardCharsets.UTF_8);
var ct = sender.doFinal(msg);
var pt = receiver.doFinal(ct);
I see that it could work that way, but have we firmly established that HPKE will be implemented in JCE as a Cipher? I see no CSR for this and I would think we'd need one. If we have not firmly established that HPKE is being done as a Cipher, then changing Cipher in advance seems like putting the cart before the horse. I just thought I'd see CSRs in place for JDK-8325513 and/or JDK-8325548 and some kind of consensus on the direction. Maybe I missed an agreement on this approach in the past.
I haven't started JDK-8325548 yet since it requires KDF. Also, I was thinking to get some consensus here on the format of the APIs first and then write the CSR.
As for the cart and horse order, I think you're right. Maybe I should do this in 2 steps:
- Basic HPKE as a cipher.
- Introduce
Cipher::exportKeyAPI and add impl in HPKE.
That seems like a good approach. If Cipher can address all the use cases for HPKE then is seems more prudent to proceed with #2. As for the KDF impact, do you see a potential Cipher-based HPKE needing to know the details of the KDF API? Or would it just need to know a standard algorithm name for a given KDF? If the latter then you may not need to wait for KDF in order to proceed given where it is in its review cycle.
I don't think KDF API is needed for a user of HPKE. They do need to choose a KDF identifier at initialization but it's just a number. I actually have a draft at https://github.com/openjdk/jdk/pull/18411 that calls the internal HKDF implementation.
@wangweij This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@wangweij has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@wangweij please create a CSR request for issue JDK-8325513 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.
@wangweij This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@wangweij This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.
@wangweij This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@wangweij This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@wangweij This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@wangweij This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@wangweij This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.