[FOLIO-3448] SPIKE: Prevent FolioExecutionContext from being initialized with wrong tenant id Created: 14/Mar/22 Updated: 05/Mar/23 Resolved: 02/Feb/23 |
|
| Status: | Closed |
| Project: | FOLIO |
| Components: | None |
| Affects versions: | None |
| Fix versions: | None |
| Type: | Tech Debt | Priority: | P1 |
| Reporter: | Julian Ladisch | Assignee: | Julian Ladisch |
| Resolution: | Done | Votes: | 0 |
| Labels: | security, security-reviewed | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original estimate: | Not Specified | ||
| Issue links: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Sprint: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Development Team: | Spring Force | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| RCA Group: | TBD | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Description |
|
How could it happen that a wrong tenant id was used? Is it possible that other code has similar tenant id issues? How can we prevent this from happening again? Can we make the library that handles tenant ids for spring bullet-proof? Can we publish a how-to or best practices on wiki.folio.org, dev.folio.org or the relevant spring github repositories? Tenant separation is a critical security feature that must be reliable. |
| Comments |
| Comment by Julian Ladisch [ 14/Mar/22 ] |
|
Oleksandr Bozhko Can you advise how FOLIO can prevent a wrong tenant id in spring based modules? |
| Comment by Jakub Skoczen [ 17/Mar/22 ] |
|
Oleksii Petrenko who from the Spring Way team should investigate this? |
| Comment by Axel Dörrer [ 24/Mar/22 ] |
|
Oleksii Petrenko the security team would like to know if this issue is already in investigation somehow, thanks. |
| Comment by Oleksandr Bozhko [ 24/Mar/22 ] |
|
Julian Ladisch From what I understood during the work on
|
| Comment by Andrei Makaranka [ 19/Apr/22 ] |
|
Priority was increased to P1 because it is affect EDIFACT and BULK_EDIT schedulers in the mod-data-export-spring. |
| Comment by Axel Dörrer [ 21/Apr/22 ] |
|
Jakub Skoczen since this got a P1 the security team would like to ask you if you could expedite it |
| Comment by Craig McNally [ 28/Apr/22 ] |
|
Jakub Skoczen please see above. |
| Comment by Craig McNally [ 12/May/22 ] |
|
Oleksii Petrenko Pavlo Smahin what needs to happen to make progress on this? |
| Comment by Mikhail Fokanov [ 02/Jun/22 ] |
|
Julian Ladisch I added documentation to readme.md and Javadoc in corresponding java classes. This documentation addresses the following questions:
Taras Spashchenko is working on wiki page for documentation of the Spring approach (e.g. Quick start guide, Guidelines etc.). In scope of these work the following question will be addressed:
|
| Comment by Pavlo Smahin [ 02/Jun/22 ] |
|
Mikhail Fokanov, please review issue
|
| Comment by Julian Ladisch [ 03/Jun/22 ] |
|
I've created
|
| Comment by Julian Ladisch [ 03/Jun/22 ] |
How can we ensure that all existing code that uses FolioExecutionContext and runs asynchronously properly sets and unsets the FolioExecutionContext? Do we need a code review Jira for each module that might be affected? |
| Comment by Julian Ladisch [ 05/Jun/22 ] |
|
This GitHub search for beginFolioExecutionContext lists all modules and all code files that might be affected: https://github.com/search?q=org%3Afolio-org+beginFolioExecutionContext&type=code |
| Comment by Mikhail Fokanov [ 08/Jun/22 ] |
Julian Ladisch I analyzed all the code that can be found by it. It is used in a right way everywhere.
I arranged a meeting for this Friday in order to explain all the details to Epam Team leads. I will collect questions and outcome of this meeting and then I will arrange a meeting for other community Team leads. There was a gap in the documentation regarding the FolioExecutionContext , that is why this mistake was done by developers. Julian Ladisch What additional actions would you suggest? |
| Comment by Julian Ladisch [ 09/Jun/22 ] |
|
https://github.com/folio-org/mod-data-export-spring/blob/v1.3.5/src/main/java/org/folio/des/config/FolioExecutionContextHelper.java#L50-L73 |
| Comment by Julian Ladisch [ 09/Jun/22 ] |
|
https://github.com/folio-org/mod-data-export-spring/blob/v1.3.5/src/main/java/org/folio/des/config/kafka/KafkaConsumerInterceptor.java#L33 |
| Comment by Julian Ladisch [ 09/Jun/22 ] |
|
https://github.com/folio-org/mod-data-export-worker/blob/v1.3.5/src/main/java/org/folio/dew/config/kafka/KafkaConsumerInterceptor.java#L34 |
| Comment by Julian Ladisch [ 09/Jun/22 ] |
|
https://github.com/folio-org/mod-remote-storage/blob/v1.5.0/src/main/java/org/folio/rs/service/AccessionQueueService.java#L97 https://github.com/folio-org/mod-remote-storage/blob/v1.5.0/src/main/java/org/folio/rs/integration/KafkaMessageListener.java#L27 |
| Comment by Julian Ladisch [ 09/Jun/22 ] |
|
https://github.com/folio-org/mod-data-export-worker/pull/217/files#diff-f1d0438cfd3ca0dc21fbf1e14570cecd7322e0179e3ff153e0d41350cadf43c9R96 |
| Comment by Mikhail Fokanov [ 09/Jun/22 ] |
|
Julian Ladisch, after you comment about endFolioExecutionContext() I added this information about it to readme.md, but actually calling endFolioExecutionContext() is not mandatory because when some one process/request will be executed, the headers will be rewritten with the new ones from the process/request (after beginFolioExecutionContext() call). |
| Comment by Julian Ladisch [ 10/Jun/22 ] |
|
It's a bug in the code when the tentant id (in FolioExecutionContext) is read from thread local but it hasn't been set for the current task. |
| Comment by Mikhail Fokanov [ 15/Jun/22 ] |
|
Julian Ladisch OK. I will ask Firebird (Siarhei Charniak, Viachaslau Khandramai) to fix the code, that you mentioned. I created a draft story: https://folio-org.atlassian.net/browse/MODEXPW-163 |
| Comment by Julian Ladisch [ 21/Jun/22 ] |
|
Mikhail Fokanov Thanks for tje Jira to improve the MODEXPW code. Is there agreement that the FolioExecutionContextSetter should be merged and that it should replace beginFolioExecutionContext() and endFolioExecutionContext()?
|
| Comment by Mikhail Fokanov [ 30/Jun/22 ] |
|
Hello Julian Ladisch. We had a call with Epam guys regarding FolioExecutionContext usage. We agreed that in each case if the context is opened, it must be closed. Guys are ready to fix the issues that you highlighted in comments to this jira on June 9th as soon as they can (e.g. this week).
|
| Comment by Julian Ladisch [ 30/Jun/22 ] |
|
Good plan, thanks! |
| Comment by Craig McNally [ 15/Sep/22 ] |
|
Waiting to merge this so it doesn't end up in Morning Glory (a workaround is used in that release), but does end up in Nolana. |
| Comment by Craig McNally [ 02/Feb/23 ] |
|
See continuation story |