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

use stronger cipher for secure storage #285

Merged

Conversation

gireeshpunathil
Copy link
Contributor

move the default from weaker MD5/DES to a stronger SHA/AES which is available in JVMs.
PBEWithMD5AndDES -> PBEWithHmacSHA512AndAES_256

@github-actions
Copy link

github-actions bot commented Jun 28, 2023

Test Results

     24 files  ±0       24 suites  ±0   12m 26s ⏱️ -5s
2 150 tests ±0  2 105 ✔️  - 1  44 💤 ±0  1 +1 
2 194 runs  ±0  2 149 ✔️  - 1  44 💤 ±0  1 +1 

For more details on these failures, see this check.

Results for commit 7188c80. ± Comparison against base commit e419739.

♻️ This comment has been updated with latest results.

@gireeshpunathil
Copy link
Contributor Author

tests did not fail when ran locally, but 2 failed here, not sure why. are those known issues?

@akurtakov
Copy link
Member

The failures look caused by the patch as I don't remember seeing these failing .
Tests run on linux so that could be one reason why you dont see it locally if you run on other OS.

@gireeshpunathil
Copy link
Contributor Author

thanks @akurtakov - I will investigate.

@gireeshpunathil
Copy link
Contributor Author

The decrypt was throwing different error code with the new cipher.
old: Input length must be multiple of 8 when decrypting with padded cipher, (code:3)
new: Wrong parameter type: IV expected, (code: 0)

So I adjusted the test to accommodate the new scenario.

@laeubi laeubi requested a review from tjwatson July 3, 2023 06:50
Copy link
Contributor

@tjwatson tjwatson left a comment

Choose a reason for hiding this comment

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

We need micro-version increases to the bundles. Otherwise looks good to me.

move the default from weaker MD5/DES to a stronger SHA/AES
which is available in JVMs.

PBEWithMD5AndDES -> PBEWithHmacSHA512AndAES_256
@gireeshpunathil gireeshpunathil force-pushed the secure-storage-default-algo branch from 4f1b32c to 7188c80 Compare September 21, 2023 13:30
@gireeshpunathil
Copy link
Contributor Author

@tjwatson - just pushed a change to up the bundle versions, PTAL. thanks!

@tjwatson tjwatson merged commit 049e4ce into eclipse-equinox:master Sep 22, 2023
14 of 19 checks passed
@tjwatson
Copy link
Contributor

tjwatson commented Sep 22, 2023

@tjwatson - just pushed a change to up the bundle versions, PTAL. thanks!

Thanks, I merged it.

@iloveeclipse
Copy link
Member

FYI, this was one from the root causes of eclipse-platform/eclipse.platform.releng.aggregator#1388.

iloveeclipse added a commit that referenced this pull request Sep 23, 2023
The #285 changed
IStorageConstants.DEFAULT_CIPHER that is inlined by TabAdvanced in
org.eclipse.equinox.security.ui but which was not touched.

See eclipse-platform/eclipse.platform.releng.aggregator#1388
@gireeshpunathil
Copy link
Contributor Author

thanks @iloveeclipse for catching this!

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.

4 participants