-
Notifications
You must be signed in to change notification settings - Fork 640
Secrets v1 #2564
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
base: main
Are you sure you want to change the base?
Secrets v1 #2564
Conversation
Signed-off-by: Nikhil Sinha <[email protected]>
Signed-off-by: Nikhil Sinha <[email protected]>
Signed-off-by: Nikhil Sinha <[email protected]>
Signed-off-by: Nikhil Sinha <[email protected]>
Signed-off-by: Nikhil Sinha <[email protected]>
Signed-off-by: Nikhil Sinha <[email protected]>
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.
It's a bit weird to me that self._secrets_url will only be present if passed into the __init__. Could the design be to always set self._secrets_url and then handle a None value elsewhere?
class Always:
def __init__(self, bla: str | None):
self.bla = bla
class Sometimes:
def __init__(self, bla: str | None):
if bla is not None:
self.bla = bla
a0 = Always("ok")
a1 = Always(None)
s0 = Sometimes("ok")
s1 = Sometimes(None)
print("a0", a0.bla)
print("a1", a1.bla)
print("s0", s0.bla)
print("s1", s1.bla)$ python ~/tmp/always_sometimes.py
a0 ok
a1 None
s0 ok
Traceback (most recent call last):
File "/Users/me/tmp/always_sometimes.py", line 21, in <module>
print("s1", s1.bla)
^^^^^^
AttributeError: 'Sometimes' object has no attribute 'bla'
python/cog/secret.py
Outdated
| return self.env.get(secret_name, "") | ||
|
|
||
|
|
||
| secret_provider = SecretProvider() |
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.
nit: I would prefer default_secret_provider since that reads more to me as "this is an instance and it is the default and you can make another one if you want" like it's a convenience, but not a strict singleton or borg.
python/cog/secret.py
Outdated
| public_key_path = Path(public_key_path_raw) | ||
| os.makedirs(os.path.basename(public_key_path), exist_ok=True) |
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.
nit
| public_key_path = Path(public_key_path_raw) | |
| os.makedirs(os.path.basename(public_key_path), exist_ok=True) | |
| public_key_path = Path(public_key_path_raw) | |
| public_key_path.parent.mkdir(mode=0700, exist_ok=True) |
python/cog/secret.py
Outdated
| with open(COG_ENV_LOCATION) as f: | ||
| for line in f: | ||
| line = line.strip() | ||
| kv = line.split("=", 1) | ||
| # Skip bad entries with no equals sign | ||
| if len(kv) != 2: | ||
| continue | ||
| self.env[kv[0]] = kv[1] |
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.
It looks like python-dotenv is already pulled in via uvicorn[standard], so I'd opt for that instead of this implementation.
python/cog/secret.py
Outdated
| def set_secret_url(self, secret_url: Optional[str] = None) -> None: | ||
| if self.once: | ||
| return |
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.
Can this be treated as an error condition?
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.
I would love to see COG_ENV_LOCATION and COG_PUBLIC_KEY_LOCATION_ENV_VAR_KEY injected as defaults rather than referencing the module-level variables directly in the __init__.
Is the intended flow for the cog process to generate the RSA key pair and the remote secret_url also has access to the public key? I guess I don't follow the order of operations here, since it seems like the values loaded from .cog/.env need to be encrypted with a key pair that isn't known until just before reading the .cog/.env values 🤔
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.
The existence of both secret and Secret with very different implementations is a source of confusion I would really love to avoid 😅 WDYT about a verb prefix like get_secret or load_secret or rolling it together with non-secret config values like a cog.getenv that automatically handles encrypted values?
Signed-off-by: Nikhil Sinha <[email protected]>
Summary
Start building implementation for passing secrets through. Locally, the secrets are pulled through the
.envfile. In production, cog creates an RSA key pair when it starts up and makes a request to a passed in URL for the secret, which it then decodes and can use.