Feature #2391
closed
Add support for changing AES-256 confidential storage keys
Added by Petr Fišer almost 4 years ago.
Updated over 3 years ago.
Description
Presently, we already have working AES-256 for the confidential storage. It all boils down to supplying correct-length secret key when constructing the SecretKeySpec Java object (details: #2214).
However, we are lacking support for changing 256b (=32B) keys with the ChangeConfidentialStorageKeyTaskExecutor LRT. The LRT has a hardcoded limit to 128b (=16B) key.
To fully support AES-256 in IdM, we should change this LRT so that:
- It supports transition between various keys and lengths: 16B->16B, 32B->32B, 16B->32B a 32B->16B. I duscussed it briefly with Ondra and it should not be hard.
We also have to:
- Update installation HOWTOs. For details on what to put into them, see: #2214#note-7 .
Once this is done, we need a mention in release notes and changelog.
- Estimated time set to 16.00 h
- Subject changed from Add support for the AES-256 for the confidential storage to Add support for changing AES-256 confidential storage keys
- Category set to Password
- Target version set to 10.6.0
- Status changed from New to In Progress
- Assignee set to Ondřej Kopr
- Related to Task #2355: Confidential storage cipher uses hardcoded initialization vector added
- % Done changed from 0 to 20
- % Done changed from 20 to 80
Support for AES256 was added. Whole functionality is implemented just with removed condional statement for key length. Because 16 length key = AES128, 32 length key = AES256. Store the algorithm type isn't required.
In this ticket I also removed check for empty key in ChangeConfidentialStorageKeyTaskExecutor, because when the project started without defined cipher secred key isn't possible fix the empty key without restore all stored values.
I spent more time for solving the ticket because when I first run my changes I just destroyed my local development environment.
The feature was implemented together with ticket #2355
Commit: https://github.com/bcvsolutions/CzechIdMng/commit/e78c526be8c8a3b7e9bb0b53d24c59c838f15fc0 (branch: okopr/2355-dynamic-vector)
Now I have trouble with one failed test and I must check mssql flyway script correctly
Now I started with check MSSQL script.
After last merge from develop I have still issue with failed tests. Failed only ChangeIdentityPermissionTest now. I will check only develop without my changes.
ChangeIdentityPermissionTest was broken in #2485, it's fixed now, sorry.
- Status changed from In Progress to Needs feedback
- Assignee changed from Ondřej Kopr to Radek Tomiška
- % Done changed from 80 to 90
I made last check with #2355.
Please @tomiskar could you provide me a feedback?
- Status changed from Needs feedback to In Progress
- Assignee changed from Radek Tomiška to Ondřej Kopr
- % Done changed from 90 to 70
I did test and code review, stronger key works and code looks nice, thx!
Review notes:
- when empty key (cipher.crypt.secret.key=) is configured from the first time (~ not encrypted), new key is defined, ChangeConfidentialStorageKeyTaskExecutor is exetuted with failure => is not posible to change key from nothing to something (I think issue is in DefaultCryptService#decryptWithKey where currently configured key is used instead "empty" old key from LRT parameter).
- AES-256 confidential storage key can be configured for some test profile (~ application-test.properties)
Note: After the first try, I destroyed my local development too - when wrong (but valid) old key is given, then values are messed out :) Changing the key is really dangerous operation => key should be configured properly, when application is installed.
- Assignee changed from Ondřej Kopr to Radek Tomiška
- Status changed from In Progress to Needs feedback
- Assignee changed from Radek Tomiška to Vít Švanda
- % Done changed from 70 to 90
- Assignee changed from Vít Švanda to Petr Fišer
I did reivew and tested last changes. LGTM
- Status changed from Needs feedback to Resolved
- % Done changed from 90 to 100
As I said in ticket #2355. We just add some information into the changelog.
Now I mark ticket as resolved. Thank you all for participate in the feature.
- Assignee changed from Petr Fišer to Ondřej Kopr
- Status changed from Resolved to Closed
Also available in: Atom
PDF
