-
Notifications
You must be signed in to change notification settings - Fork 898
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
[WIP] Generate secret key and JWT token for API auth #23052
[WIP] Generate secret key and JWT token for API auth #23052
Conversation
@@ -176,9 +178,24 @@ def default_environment | |||
{:name => "WORKER_HEARTBEAT_FILE", :value => Rails.root.join("tmp/worker.hb").to_s}, | |||
{:name => "WORKER_HEARTBEAT_METHOD", :value => "file"}, | |||
{:name => "TERRAFORM_RUNNER_URL", :value => "https://opentofu-runner:6000"}, | |||
{:name => "SECRET_KEY", :value => opentofu_runner_secret_key}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this ENV var be prefixed with TERRAFORM_RUNNER_
like the others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, plus this should probably be a secret instead of an ENV var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need SECRET_KEY defined on every container or just the opentofu-runner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I think this should only be added to the opentofu-runner. Yes, I think it needs to be a volume mount otherwise it may be flagged by security scans.
] + database_environment + memcached_environment + messaging_environment | ||
end | ||
|
||
SECRET_KEY_FILE = "/run/secrets/manageiq/application/encryption_key".freeze | ||
def opentofu_runner_secret_key | ||
@opentofu_runner_secret_key ||= File.exist?(file_path) ? File.read(SECRET_KEY_FILE) : "opentofu_runner_key" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does file_path
come from? Should it be SECRET_KEY_FILE
instead?
@@ -1,3 +1,5 @@ | |||
require 'jwt' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this require inside of the def opentofu_runner_token
method since that is the only caller. Only the orchestrator should call that method, so other workers will not load it.
@sudhir-kelkar in order for the tests to pass you have to install libsodium-dev in |
@@ -176,9 +178,24 @@ def default_environment | |||
{:name => "WORKER_HEARTBEAT_FILE", :value => Rails.root.join("tmp/worker.hb").to_s}, | |||
{:name => "WORKER_HEARTBEAT_METHOD", :value => "file"}, | |||
{:name => "TERRAFORM_RUNNER_URL", :value => "https://opentofu-runner:6000"}, | |||
{:name => "SECRET_KEY", :value => opentofu_runner_secret_key}, | |||
{:name => "TERRAFORM_RUNNER_TOKEN", :value => opentofu_runner_token}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also probably be volume mount to avoid the same security scan issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably avoid an ENV var or volume entierly if the token can be generated from the v2_key since all workers have access to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but then every worker needs to load jwt
. Unless the require is moved inside the connection logic.
Gemfile
Outdated
@@ -44,6 +44,7 @@ gem "gettext_i18n_rails_js", "~>1.3.0" | |||
gem "hamlit", "~>2.11.0" | |||
gem "inifile", "~>3.0", :require => false | |||
gem "inventory_refresh", "~>2.1", :require => false | |||
gem "jwt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be :require => false
Checked commits sudhir-kelkar/manageiq@595d0ec~...546d95c with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint lib/container_orchestrator/object_definition.rb
|
file.sync = true | ||
file.write(jwtToken) | ||
end | ||
@opentofu_runner_token ||= TERRAFORM_RUNNER_TOKEN_FILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need an instance var for a frozen constant? Or did you intend to set this to the value in jwtToken
?
I am covering this scenario in PR Instead of generating JWT token here in deployment. Closing this PR as we don't need it anymore |
Generating Secret Key and Token During Worker Deployment
The same secret and token are mounted as environment variables to the worker pod.