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

fix(ephemerals): Quick fix for ephemeral spin-up #23857

Merged
merged 1 commit into from
Apr 27, 2023

Conversation

craig-rueda
Copy link
Member

SUMMARY

Quick fix to add support for the "new" requirement to include a SECRET_KEY

@rusackas rusackas merged commit 33bb27b into apache:master Apr 27, 2023
@cwegener
Copy link
Contributor

@craig-rueda @rusackas Why not store an actual SECRET key in this Github Repo and let the Github runner pass it to ECS via the very same 'render-task-definition' action using the environment-variables parameter? https://github.com/aws-actions/amazon-ecs-render-task-definition/blob/b451ae8d4af191e94b479ca69435c85a0bd25140/action.yml#L16-L18

@rusackas
Copy link
Member

rusackas commented Apr 28, 2023

@cwegener We might still do that... but it requires an Apache Infra task to set that up, and we wanted to unblock the broken ephemerals as expediently as possible. While it would be a nice thing to do, I'm not particularly worried about the security of these workspaces, as everyone on the internet knows the admin credentials anyway :)

@cwegener
Copy link
Contributor

I suspected that something like that would be the reason. 🙂

I disagree with with the security stance though. It's not the fact that these workspace are public that I am lamenting, but the fact that the actual practice for publicizing secret keys is unchanged as a result of this PR. Totally understand the need for expediting of this fix in this case of course.

Another option would be to use an actual random string with high entropy by following the Flask documentation recommendations: https://flask.palletsprojects.com/en/2.3.x/quickstart/#sessions
https://flask.palletsprojects.com/en/2.3.x/config/#SECRET_KEY

sebastianliebscher pushed a commit to sebastianliebscher/superset that referenced this pull request Apr 28, 2023
@rusackas
Copy link
Member

rusackas commented May 4, 2023

We could definitely provide a randomized secret key on these ephemeral environments, but it seems a bit moot when we publish the admin credentials publicly on the thread as they're spun up. Maybe we can add an extra warning on the PR comment that lists those credentials with a "⚠️ This instance is public: do not connect production databases! ⚠️" warning, or something similar.

@craig-rueda craig-rueda deleted the craig/fixing_ephemerals branch May 4, 2023 16:23
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants