Skip to content
This repository was archived by the owner on Nov 24, 2024. It is now read-only.

plexus-cipher 3.0.0 #45

Merged
merged 8 commits into from
Sep 28, 2024
Merged

plexus-cipher 3.0.0 #45

merged 8 commits into from
Sep 28, 2024

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Sep 27, 2024

Cipher is a small simple (nearly trivial) component that is able to encrypt/decrypt strings using provided "master password".

This commit moves it to version 3.0.0 denoting a major (and backward incompatible jump.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@cstamas cstamas requested a review from slachiewicz September 27, 2024 20:57
@cstamas cstamas self-assigned this Sep 27, 2024
@cstamas cstamas added this to the 3.0.0 milestone Sep 27, 2024
@cstamas cstamas changed the title Updates plexus-cipher 3.0.0 Sep 27, 2024
Copy link
Member

@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

LGTM. If I summarise:

  • change package name to org.codehaus.plexus (which sounds weird given Codehaus does not exist anymore...)
  • change the default cipher from AES/CBC/PKCS5Padding to AES/GCM/NoPadding, why ?

@@ -88,6 +91,7 @@ void testDefaultAlgorithmExists() throws Exception {

// -------------------------------------------------------------

@Disabled("This test is not really a test")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe delete it then ?

@@ -34,7 +34,7 @@ class PBECipherTest {

final String clearText = "veryOpenText";

final String encryptedText = "F7eMV2QRQF4H0ODCA1nrTGUWacCXVvPemSjaQjGbO6U=";
final String encryptedText = "ce/l2ofOiSELRT1WAjOyNoZbG+2FQcrlOKEdDr5mi6esyR2LfvBY855yxW5bqHZi";
Copy link
Member

Choose a reason for hiding this comment

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

Why has the encrypted value changed ?

@cstamas
Copy link
Member Author

cstamas commented Sep 28, 2024

LGTM. If I summarise:

  • change package name to org.codehaus.plexus (which sounds weird given Codehaus does not exist anymore...)
  • change the default cipher from AES/CBC/PKCS5Padding to AES/GCM/NoPadding, why ?

The package name change is IMO okay, many (today GH, former Codehaus) orgs still publish under org.codehaus package, and this has been well established (like older groovy versions that are still maintained). I don't find this problematic. This GH org "owns" the org.codehaus.plexus G on Central, so using same package is okay as well IMO. OTOH, I find it more problematic to use package name of existing private entity/company.

The cipher changes have been undone, I was just reading and trying out some last night. Now there is only one change, they secret key alg is changed to PBKDF2WithHmacSHA512 as recommended since Java 8.

The encrypted value changes really does not matter, as 2.1.0 release already broke backward compat, see #23 If we'd move from 2.0 to 2.1.0, users would need to reencrypt everything they have encrypted anyway. So IMO this change is ok as well.

Finally, to signify all that above, but also the fact of moving to Java 17, am moving version to 3.0.0.

@cstamas
Copy link
Member Author

cstamas commented Sep 28, 2024

In fact, just checked: nothing mandates for Java 17, so I undid that bit as well. Having this lib 8+ is just extra benefit.

@slachiewicz
Copy link
Member

To me GCM/No padding also looks good.
We can assume that version 2.1.0 was never used in our components

@slachiewicz
Copy link
Member

slachiewicz commented Sep 28, 2024

If not under codehaus, what other alternatives?
Move it to any other foundation or an organisation? Together with all other components like Modello?

@cstamas cstamas merged commit 08ed987 into master Sep 28, 2024
16 checks passed
@cstamas cstamas deleted the updates branch September 28, 2024 14:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants