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.

Sign up for GitHub

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

Warn if api_key in Config #1049

Merged
merged 7 commits into from
Jan 7, 2025
Merged

Warn if api_key in Config #1049

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a7fbe6c
Look for the presence of `api_key` as a key when loading config files…
erickgalinkin Dec 20, 2024
8da4fe2
Remove warning exception, check recursively.
erickgalinkin Dec 20, 2024
68d123b
Remove import
erickgalinkin Dec 20, 2024
e1ca672
Additional, minor improvements. Use `command.hint`, clarify message.
erickgalinkin Jan 2, 2025
f02d735
Add expansion for lists
erickgalinkin Jan 6, 2025
5013642
Remove circular dependency
erickgalinkin Jan 6, 2025
da78594
Add alternative warning message for Windows users. Add unit test in t…
erickgalinkin Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion garak/_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import importlib
import logging
import os
import stat
import pathlib
from typing import List
import yaml
Expand Down Expand Up @@ -115,14 +116,23 @@ def _nested_dict():
# placeholder
# generator, probe, detector, buff = {}, {}, {}, {}

def _key_exists(d: dict, key: str) -> bool:
# Check for the presence of a key in a nested dict.
if not isinstance(d, dict):
return False
Copy link
Collaborator

@jmartin-tech jmartin-tech Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think there are any current places where this would occur, however should also process any list that may contain a dict objects?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively one could go the other way and raise ValueError("_key_exists() can only process dicts"). I think it's probably beneficial to pick one way (dicts+lists of dicts) or the other (dicts or gtfo)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While I like the idea of enforcing dict, the way this is written is looking for something at the root which is a value and returns False at the base of the nested dict. I don't think we should be processing list objects here at all, since we're explicitly looking for a key that will end up being used by the config.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My concern would be if some future plugin offered something like:

detectors:
  fake:
    MultiCallDetector:
      endpoints:
        - uri: https://localhost:4000/
          api_key: value1
        - uri: https://localhost:4001/
          api_key: value2
        - uri: https://localhost:4002/
          api_key: value3

This under the current pattern would be reported as not having any api_key values when in fact there are multiple.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair. That's pretty easy to solve, I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed lists in f02d735 @jmartin-tech

Would love your review here.

if key in d.keys():
return True
else:
return any([_key_exists(val, key) for val in d.values()])


def _set_settings(config_obj, settings_obj: dict):
for k, v in settings_obj.items():
setattr(config_obj, k, v)
return config_obj


def _combine_into(d: dict, combined: dict) -> None:
def _combine_into(d: dict, combined: dict) -> dict:
if d is None:
return combined
for k, v in d.items():
Expand All @@ -141,6 +151,11 @@ def _load_yaml_config(settings_filenames) -> dict:
with open(settings_filename, encoding="utf-8") as settings_file:
settings = yaml.safe_load(settings_file)
if settings is not None:
if _key_exists(settings, "api_key"):
logging.info(f"API key found in {settings_filename}. Checking readability...")
res = os.stat(settings_filename)
if res.st_mode & stat.S_IROTH or res.st_mode & stat.S_IRGRP:
erickgalinkin marked this conversation as resolved.
Show resolved Hide resolved
logging.warn(f"A possibly secret value (`api_key`) was detected in {settings_filename}, which is readable by users other than yourself.")
erickgalinkin marked this conversation as resolved.
Show resolved Hide resolved
config = _combine_into(settings, config)
return config

Expand Down
Loading