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

Encrypt config.yaml #27

Merged
merged 2 commits into from
Oct 12, 2023
Merged

Encrypt config.yaml #27

merged 2 commits into from
Oct 12, 2023

Conversation

vivian-rook
Copy link
Contributor

@vivian-rook vivian-rook commented Oct 11, 2023

Bug: T348476

I believe that both the worker and web configs can be combine into one file. At which point having it encrypted should simplify deployment as the git clone will have it in place, and then the main purpose of this, having the file stored in git, is achieved.

@vivian-rook vivian-rook force-pushed the T348476 branch 6 times, most recently from efc171a to 212b467 Compare October 11, 2023 17:25
@vivian-rook
Copy link
Contributor Author

@audiodude @siddharthvp I could use some help. I updated the exception to include UnicodeError though that seems to fail, works on a minimal example on py3.7 though not through tox? If I put a bare except py37-pytest will succeed but flake will fail on the bare exception. Any thoughts on what I'm doing wrong?

@siddharthvp
Copy link
Collaborator

@vivian-rook Looks like the error thrown is yaml.reader.ReaderError, which extends yaml.error.YAMLError which extends Exception. So catching UnicodeDecodeError, UnicodeError doesn't solve the issue.

Instead of updating it to catch YAMLError, I think the better solution would be to give a different name to this binary file, say config-prod.yaml so that it doesn't interfere with local testing and unit tests.

@audiodude
Copy link
Collaborator

Agree with @siddharthvp. I was already confused about the comment:

# Is ok if we can not load config.yaml

How is that okay? Won't both development and production be broken?

Copy link
Collaborator

@audiodude audiodude left a comment

Choose a reason for hiding this comment

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

Don't you have to tell the code to read from config-prod.yml instead of config.yml now?

@vivian-rook
Copy link
Contributor Author

Agree with @siddharthvp. I was already confused about the comment:

# Is ok if we can not load config.yaml

How is that okay? Won't both development and production be broken?

In the test code it tends towards the default_config.yaml file, then the code attempts to update to the config.yaml file (which in prod should be there, but in dev it is usually not). Which in looking at it feels like a very awkward way of setting it up, that the code itself tries to select if it is dev or prod? Seems like something that should be made as a startup option for which file to use. Though that would be a cleanup task for another patch (Indeed, it is yet another thing that can go away if we have it in k8s)

@audiodude
Copy link
Collaborator

Right, I see that now, after expanding more lines of the file. It uses default-config.yml and then overlays config.yml if it is present.

What is the plan for deploying the git-crypt key file to the production machines? Just drop it in and do git-crypt unlock?

@vivian-rook
Copy link
Contributor Author

Looks like the checks are passing. @Sd0001 @audiodude the decryption key is on the bastion in /opt/

@vivian-rook
Copy link
Contributor Author

What is the plan for deploying the git-crypt key file to the production machines? Just drop it in and do git-crypt unlock?

Yeah, that's what I usually do. Usually on the bastion system for a given project. Also helps with state files for things like terraform to just have it in one place. Though object storage is nicer if we can set it up. I wonder if object storage would be sufficient for the git-crypt key as well. That has always been an "Ask a maintainer" kind of solution. Perhaps it doesn't have to be. Though that is probably a solution in search of a problem.

@siddharthvp
Copy link
Collaborator

Don't you have to tell the code to read from config-prod.yml instead of config.yml now?

config-prod.yaml is just the name for the encrypted file. As I understand it, in production the file will be decrypted and named config.yaml.

.gitignore Outdated Show resolved Hide resolved
Bug: T348476
@audiodude
Copy link
Collaborator

Don't you have to tell the code to read from config-prod.yml instead of config.yml now?

config-prod.yaml is just the name for the encrypted file. As I understand it, in production the file will be decrypted and named config.yaml.

Ok. I haven't worked with git-crypt at all, but just reading a few articles it sounds to me that it most commonly encrypts files "in place" such that they are decrypted when they are put in the filesystem by git.

@vivian-rook vivian-rook merged commit 50608af into main Oct 12, 2023
1 check passed
@vivian-rook vivian-rook deleted the T348476 branch October 12, 2023 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants