Skip to content

Conversation

@wangweij
Copy link
Contributor

@wangweij wangweij commented Mar 20, 2024

Implement HPKE as defined in https://datatracker.ietf.org/doc/rfc9180/.
image


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change requires CSR request JDK-8366437 to be approved
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issues

  • JDK-8325448: Hybrid Public Key Encryption (Enhancement - P3)
  • JDK-8366437: Hybrid Public Key Encryption (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18411/head:pull/18411
$ git checkout pull/18411

Update a local copy of the PR:
$ git checkout pull/18411
$ git pull https://git.openjdk.org/jdk.git pull/18411/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18411

View PR using the GUI difftool:
$ git pr show -t 18411

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18411.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 20, 2024

👋 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.

@openjdk
Copy link

openjdk bot commented Mar 20, 2024

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk
Copy link

openjdk bot commented Mar 20, 2024

@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.

@bridgekeeper
Copy link

bridgekeeper bot commented May 16, 2024

@wangweij This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 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
Copy link
Contributor Author

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Jun 28, 2024
@openjdk
Copy link

openjdk bot commented Jun 28, 2024

@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-8325448 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 23, 2024

@wangweij This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 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!

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 18, 2024

@wangweij This pull request has been inactive for more than 16 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.

@bridgekeeper bridgekeeper bot closed this Oct 18, 2024
@wangweij
Copy link
Contributor Author

wangweij commented Nov 6, 2024

/open

@openjdk openjdk bot reopened this Nov 6, 2024
@openjdk
Copy link

openjdk bot commented Nov 6, 2024

@wangweij This pull request is now open

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 1, 2025

@wangweij This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 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 wangweij marked this pull request as ready for review February 25, 2025 21:30
@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 25, 2025
@mlbridge
Copy link

mlbridge bot commented Feb 25, 2025

Webrevs

* of RFC 9180 and the
* <a href="https://www.iana.org/assignments/hpke/hpke.xhtml">IANA HPKE page</a>.
* <p>
* Once an {@code HPKEParameterSpec} object is created, additional methods
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

@seanjmullan
Copy link
Member

seanjmullan commented Aug 27, 2025

Hi Sebastian, the API you suggested is only the KEM step, and it should be made internal inside HPKE.

At the end of the day, HPKE is still a cipher. I understand the key encapsulation message (aka, KEM ciphertext) is different from a traditional IV, but they share some key characteristics: 1) generated by the sender after initialization, 2) cryptographically random, 3) then made public, 4) has critical impact on encryption result.

To avoid some of this potential confusion, I think it could help to expand on the description of Cipher.getIV() to describe this new use case for IV, something like changing this sentence:

"This is useful in the case where a random IV was created, or in the context of password-based encryption or decryption, where the IV is derived from a user-supplied password."

to:

"This is useful in the case where a random IV was created, or in the context of password-based encryption or decryption, where the IV is derived from a user-supplied password, or in the case of HPKE (Hybrid Public Key Encryption) where IV contains the encapsulation of the KEM shared secret."

@wangweij
Copy link
Contributor Author

Hi Sebastian, the API you suggested is only the KEM step, and it should be made internal inside HPKE.
At the end of the day, HPKE is still a cipher. I understand the key encapsulation message (aka, KEM ciphertext) is different from a traditional IV, but they share some key characteristics: 1) generated by the sender after initialization, 2) cryptographically random, 3) then made public, 4) has critical impact on encryption result.

To avoid some of this potential confusion, I think it could help to expand on the description of Cipher.getIV() to describe this new use case for IV, something like changing this sentence:

"This is useful in the case where a random IV was created, or in the context of password-based encryption or decryption, where the IV is derived from a user-supplied password."

to:

"This is useful in the case where a random IV was created, or in the context of password-based encryption or decryption, where the IV is derived from a user-supplied password, or in the case of HPKE (Hybrid Public Key Encryption) where IV contains the encapsulation of the KEM shared secret."

Good idea. Somehow I hesitate to update the base spec directly. Shall we put the whole paragraph into an @apiNote? A similar paragraph also appears in CipherSpi::engineGetIV.

@seanjmullan
Copy link
Member

Hi Sebastian, the API you suggested is only the KEM step, and it should be made internal inside HPKE.
At the end of the day, HPKE is still a cipher. I understand the key encapsulation message (aka, KEM ciphertext) is different from a traditional IV, but they share some key characteristics: 1) generated by the sender after initialization, 2) cryptographically random, 3) then made public, 4) has critical impact on encryption result.

To avoid some of this potential confusion, I think it could help to expand on the description of Cipher.getIV() to describe this new use case for IV, something like changing this sentence:
"This is useful in the case where a random IV was created, or in the context of password-based encryption or decryption, where the IV is derived from a user-supplied password."
to:
"This is useful in the case where a random IV was created, or in the context of password-based encryption or decryption, where the IV is derived from a user-supplied password, or in the case of HPKE (Hybrid Public Key Encryption) where IV contains the encapsulation of the KEM shared secret."

Good idea. Somehow I hesitate to update the base spec directly. Shall we put the whole paragraph into an @apiNote? A similar paragraph also appears in CipherSpi::engineGetIV.

Yes, making this text an API note, which is what it really is, is a really good idea.

@wangweij
Copy link
Contributor Author

wangweij commented Sep 2, 2025

New commit pushed. The with methods can no longer be used to "reset" a field to its default value, for example, calling withInfo(new byte[0]) to remove the non-default info or withAuthKey(null) to revert to mode_base.

Reasons:

  1. Allowing a default value might hide unintended empty user input.
  2. It's hard to manage all the fields in an HPKEParameterSpec object and reuse it back and forth.

AlgorithmParameterSpec params, SecureRandom random)
throws InvalidKeyException, InvalidAlgorithmParameterException {
impl = new Impl(opmode);
if (!(key instanceof AsymmetricKey ak)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a null check needed for key and params? It appears Cipher leaves that to the SPI to accept or reject.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If key is null, you will see "InvalidKeyException: Not an asymmetric key". I assume that's also OK?

I'll deal with params, there is a similar exception but unfortunately I called params.getClass() there.

this.exporter_secret = exporter_secret;
}

SecretKey ExportKey(String algorithm, byte[] exporter_context, int L) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are the methods in this class capitalized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like using the original function names from the RFC. If you don't like, I can modify them to Java-style.

// deriveData must and can be called because all info to
// thw builder are just byte arrays. Any KDF impl can handle this.
var kdf = KDF.getInstance(kdfAlg);
var key_schedule_context = concat(new byte[]{(byte) mode},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is key_sechedule_context worth zero'ing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not? psk_id does not sound like a secret thing. I understand psk is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

csr Pull request needs approved CSR before integration rfr Pull request is ready for review security [email protected]

Development

Successfully merging this pull request may close these issues.

7 participants