Skip to content
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.

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

PoC for separate session keys for password based encryption #1313

Closed
wants to merge 3 commits into from
Closed

PoC for separate session keys for password based encryption #1313

wants to merge 3 commits into from

Conversation

pschichtel
Copy link

While implementing my pgcrypto-kt library (a Kotlin reimplementation of the pgcrypto postgres extension for client side encryption) I faced 2 problems:

  1. implementing the sess-key option was not directly¹ possible, since PBE encryption method does not trigger session info generation
  2. implementing the s2k-cipher-algo option was not directly¹ possible, since the PBE encryption method will always use the cipher algo used for data.

¹: I eventually found dirty workarounds to trick BC into allowing it anyway, as posted on mailing list.

This PR extends the PGPKeyEncryptionMethodGenerator interface by the wantsSessionInfo(): boolean method, which is default-implemented to return true. The base class PBEKeyEncryptionMethodGenerator overrides that method and makes it configurable, while defaulting to false. Unless I'm missing something here, I'd argue this should be compatible even with user customizations of the method generators. This covers problem 1.

For problem 2 I added getSessionInfoAlgo(): Integer to PGPKeyEncryptionMethodGenerator, also default-implemented to return null. PBEKeyEncryptionMethodGenerator and PublicKeyKeyEncryptionMethodGenerator make it configurable. If the method returns a non-null value, it will be used to encrypt the session key. That covers problem 2.

Method names are definitely open for discussion and I left out docs for the moment, until the actual code changes are accepted.

@pschichtel
Copy link
Author

@dghgit did you have a chance to look at this?

@dghgit
Copy link
Contributor

dghgit commented Jan 16, 2023

I've added a setForceSessionKey method for the forcing of the session key. That should show up here on github in a minute.

Now looking at the algorithm customisation

pull bot pushed a commit to 86dh/bc-java that referenced this pull request Jan 16, 2023
@dghgit
Copy link
Contributor

dghgit commented Jan 17, 2023

Okay, so for the last one I added setSessionKeyWrapperAlgorithm() to PBEKeyEncryptionMethodGenerator. Although I did end up with fairly minimal changes, doing this was cleanly was a bit more complicated than I thought it would be, I can see how you ended up where you did. The updated code for this should appear on github shortly.

End result:

  • for 1: PGPEncryptedDataGenerator.setForceSessionKey(true)
  • for 2: PBEKeyEncryptionMethodGenerator.setSessionKeyWrapperAlgorithm(PGPEncryptedData.AES_256)

If I've understood correctly this should both items in this PR. Let me know.

@pschichtel
Copy link
Author

@dghgit looks good to me, thanks for working on this, especially since it's a bit of a niche request! I can see why you went for more minimal changes.

@pschichtel pschichtel closed this Jan 17, 2023
pull bot pushed a commit to 86dh/bc-java that referenced this pull request Jan 17, 2023
pull bot pushed a commit to 86dh/bc-java that referenced this pull request Jan 17, 2023
sergejskozlovics pushed a commit to LUMII-Syslab/tls-injection-mechanism that referenced this pull request Jul 7, 2023
sergejskozlovics pushed a commit to LUMII-Syslab/tls-injection-mechanism that referenced this pull request Jul 7, 2023
sergejskozlovics pushed a commit to LUMII-Syslab/tls-injection-mechanism that referenced this pull request Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants