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

Introduce a more granular, site-based allowlist #14

Merged
merged 1 commit into from
Jul 10, 2024
Merged

Conversation

bcspragu
Copy link
Collaborator

@bcspragu bcspragu commented Jul 9, 2024

This PR updates the allowlist in a few ways:

  1. Move from a comma-delimited list of domains to a structured JSON-formatted file
  2. Adds support for allowlisting individual emails, in addition to domains
  3. Adds support for allowlisting an entity (domain or email) for only a specific set of sites

It also removes the allowlists from the repo, as those will start containing more individual information.

This PR updates the allowlist in a few ways:

1. Move from a comma-delimited list of domains to a structured JSON-formatted file
2. Adds support for allowlisting individual emails, in addition to domains
3. Adds support for allowlisting an entity (domain or email) for only a specific set of sites

It also removes the allowlists from the repo, as those will start containing more individual information.
@bcspragu bcspragu requested a review from gbdubs July 9, 2024 20:04
@gbdubs
Copy link
Collaborator

gbdubs commented Jul 9, 2024

My only concern here is on removing the allowlist config from the repo - it just creates a pain when cloning etc. I know we removed sops as a dep from this repo in #8, but this smells off. Could we move the allowlist into the place that we pull the secrets in from? Could we make the allowlist nonsensitive in some way?

@bcspragu
Copy link
Collaborator Author

bcspragu commented Jul 9, 2024

My only concern here is on removing the allowlist config from the repo - it just creates a pain when cloning etc. I know we removed sops as a dep from this repo in #8, but this smells off. Could we move the allowlist into the place that we pull the secrets in from? Could we make the allowlist nonsensitive in some way?

I originally planned on doing this in this PR, but I decided I'll do it after, it'll look like this:

  1. Re-introduce sops (e.g. add a .sops.yaml, create relevant Azure KMS keys)
  2. Encrypt the JSON files
  3. Update the local test scripts (and any deployment stuff) to decrypt things as part of packaging/running/deploying

There's just a bunch of pieces there, so I figured I'd split it out. Thanks for keeping me honest!

@bcspragu
Copy link
Collaborator Author

(ping for actual approval when you get a chance! this is implemented in #16)

Copy link
Collaborator

@gbdubs gbdubs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!!

@bcspragu bcspragu merged commit db78ddb into main Jul 10, 2024
1 check passed
@bcspragu bcspragu deleted the brandon/site-auth branch July 10, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants