Skip to content

Commit efa7aed

Browse files
benoit74benoit74
committed
Enhance URL secrets removal
- redact password which might be present in URL netloc - rename the function to remove_url_secrets since it does not remove only S3 secret - handle properly the reconstruction of URL query - prefer urlsplit and urlunsplit - instead of urlparse - since this is the future / we do not need to separate parameters from parts
1 parent 8382219 commit efa7aed

File tree

2 files changed

+77
-27
lines changed

2 files changed

+77
-27
lines changed

‎dispatcher/backend/src/routes/utils.py

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import logging
22
from typing import Any, Dict, List
3-
from urllib.parse import parse_qs, urlparse
3+
from urllib.parse import parse_qs, urlencode, urlsplit, urlunsplit
44

55
from common.constants import SECRET_REPLACEMENT
66
from common.schemas.models import ScheduleConfigSchema
@@ -26,7 +26,7 @@ def has_dict_sub_key(data: Dict[str, Any], keys: List[str]):
2626
def remove_secrets_from_response(response: dict):
2727
"""replaces or removes (in-place) all occurences of secrets"""
2828
remove_secret_flags(response)
29-
remove_s3_secrets(response)
29+
remove_url_secrets(response)
3030

3131

3232
def remove_secret_flags(response: dict):
@@ -87,26 +87,45 @@ def remove_secret_flag_from_command(
8787
] = f'--{field_name}="{SECRET_REPLACEMENT}"'
8888

8989

90-
def remove_s3_secrets(response: dict):
91-
"""remove keyId and secretAccessKey query params from any URL we might find"""
90+
def remove_url_secrets(response: dict):
91+
"""remove secrets from any URL we might find
92+
93+
For now we remove:
94+
- password if passed in the network location
95+
- keyId and secretAccessKey query params (probably used for S3)
96+
"""
9297
for key in response.keys():
9398
if not response[key]:
9499
continue
95100
if isinstance(response[key], dict):
96-
remove_s3_secrets(response[key])
101+
remove_url_secrets(response[key])
97102
else:
98103
if not isinstance(response[key], str) or "://" not in response[key]:
99104
continue
100-
for part in [part for part in response[key].split() if "://" in part]:
101-
url = urlparse(part)
102-
url = url._replace(
103-
query="&".join(
104-
[
105-
f"{key}={value}"
106-
for key, values in parse_qs(url.query).items()
105+
for url in [word for word in response[key].split() if "://" in word]:
106+
urlparts = urlsplit(url)
107+
newquery = urlencode(
108+
{
109+
key: (
110+
value
107111
if str(key).lower() not in ["keyid", "secretaccesskey"]
108-
for value in values
109-
]
112+
else SECRET_REPLACEMENT
113+
)
114+
for key, values in parse_qs(urlparts.query).items()
115+
for value in values
116+
},
117+
doseq=True,
118+
)
119+
newnetloc: str | None = urlparts.netloc
120+
if newnetloc and urlparts.password:
121+
newnetloc = newnetloc.replace(urlparts.password, SECRET_REPLACEMENT)
122+
secured_url = urlunsplit(
123+
(
124+
urlparts.scheme,
125+
newnetloc,
126+
urlparts.path,
127+
newquery,
128+
urlparts.fragment,
110129
)
111-
).geturl()
112-
response[key] = response[key].replace(part, url)
130+
)
131+
response[key] = response[key].replace(url, secured_url)

‎dispatcher/backend/src/tests/unit/routes/test_utils.py

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -443,14 +443,18 @@
443443
"expiration": 60,
444444
"upload_uri": (
445445
"s3://s3.us-west-1.wasabisys.com/"
446-
"?bucketName=org-kiwix-zimfarm-logs"
446+
"?keyId=********"
447+
"&secretAccessKey=********"
448+
"&bucketName=org-kiwix-zimfarm-logs"
447449
),
448450
},
449451
"artifacts": {
450452
"expiration": 20,
451453
"upload_uri": (
452454
"s3://s3.us-west-1.wasabisys.com/"
453-
"?bucketName=org-kiwix-zimfarm-artifacts"
455+
"?keyId=********"
456+
"&secretAccessKey=********"
457+
"&bucketName=org-kiwix-zimfarm-artifacts"
454458
),
455459
},
456460
},
@@ -470,7 +474,7 @@
470474
"please_clean_me": (
471475
"something\nwhat s3://s3.us-west-1.wasabisys.com/"
472476
"?keyId=this_is_super_secret"
473-
"&secretAccessKey=this_is_super_secret"
477+
"&secretAccessKey=this_is_a_super_secret"
474478
"&bucketName=org-kiwix-zimfarm-logs what\n"
475479
"something\n"
476480
),
@@ -489,7 +493,9 @@
489493
"response_but": {
490494
"please_clean_me": (
491495
"something\nwhat s3://s3.us-west-1.wasabisys.com/"
492-
"?bucketName=org-kiwix-zimfarm-logs what\n"
496+
"?keyId=********"
497+
"&secretAccessKey=********"
498+
"&bucketName=org-kiwix-zimfarm-logs what\n"
493499
"something\n"
494500
),
495501
},
@@ -566,46 +572,71 @@ def test_remove_secrets(response, expected_response):
566572
"&keyId=this_is_super_secret \n"
567573
"something"
568574
),
575+
"please_clean_me8": (
576+
" ftp://username:password@hostname:123/path not encoded?"
577+
"param=val%26ue#anchor "
578+
),
569579
},
570580
{
571581
"please_clean_me1": (
572582
"s3://s3.us-west-1.wasabisys.com/"
573-
"?bucketName=org-kiwix-zimfarm-logs"
583+
"?keyId=********"
584+
"&secretAccessKey=********"
585+
"&bucketName=org-kiwix-zimfarm-logs"
574586
),
575587
"please_clean_me2": (
576588
"s3://s3.us-west-1.wasabisys.com/"
577589
"?bucketName=org-kiwix-zimfarm-logs"
590+
"&keyId=********"
591+
"&secretAccessKey=********"
578592
),
579593
"please_clean_me3": (
580594
"s3://s3.us-west-1.wasabisys.com/"
581595
"?bucketName=org-kiwix-zimfarm-logs"
596+
"&keyId=********"
597+
"&secretAccessKey=********"
582598
"&something=somevalue"
583599
),
584600
"please_clean_me4": (
585601
"s3://s3.us-west-1.wasabisys.com/"
586602
"?bucketName=org-kiwix-zimfarm-logs"
603+
"&secretAccessKey=********"
587604
"&something=somevalue"
605+
"&keyId=********"
588606
"&something2=somevalue2"
589607
),
590608
"please_clean_me5": (
591609
" s3://s3.us-west-1.wasabisys.com/"
592-
"?bucketName=org-kiwix-zimfarm-logs"
610+
"?keyId=********"
611+
"&secretAccessKey=********"
612+
"&bucketName=org-kiwix-zimfarm-logs"
593613
),
594614
"please_clean_me6": (
595615
"s3://s3.us-west-1.wasabisys.com/"
596-
"?bucketName=org-kiwix-zimfarm-logs "
616+
"?keyId=********"
617+
"&secretAccessKey=********"
618+
"&bucketName=org-kiwix-zimfarm-logs "
597619
),
598620
"please_clean_me7": (
599621
"something s3://s3.us-west-1.wasabisys.com/"
600-
"?bucketName=org-kiwix-zimfarm-logs \n"
622+
"?keyId=********"
623+
"&secretAccessKey=********"
624+
"&bucketName=org-kiwix-zimfarm-logs \n"
601625
"something s3://s3.us-west-1.wasabisys.com/"
602-
"?bucketName=org-kiwix-zimfarm-logs \n"
626+
"?secretAccessKey=********"
627+
"&bucketName=org-kiwix-zimfarm-logs \n"
603628
"something s3://s3.us-west-1.wasabisys.com/"
604-
"?bucketName=org-kiwix-zimfarm-logs \n"
629+
"?keyId=********"
630+
"&bucketName=org-kiwix-zimfarm-logs \n"
605631
"something s3://s3.us-west-1.wasabisys.com/"
606-
"?bucketName=org-kiwix-zimfarm-logs \n"
632+
"?bucketName=org-kiwix-zimfarm-logs"
633+
"&keyId=******** \n"
607634
"something"
608635
),
636+
"please_clean_me8": (
637+
" ftp://username:--------@hostname:123/path not encoded?param="
638+
"val%26ue#anchor "
639+
),
609640
},
610641
),
611642
],

0 commit comments

Comments
 (0)