diff --git a/Changelog.txt b/Changelog.txt index 0c5ea5c..8a2984d 100644 --- a/Changelog.txt +++ b/Changelog.txt @@ -77,6 +77,20 @@ Newest first. Dates are ISO 8601 (YYYY-MM-DD). entry (the official schema pins only the first two and allows extras); the context is pinned in the fail-closed allowlist and covered by the drift watchdog (#239). + - security(config): close the bypass in the userinfo guard below: checking + `urlsplit(...).netloc` only catches a credential whose `@` lands inside the + authority, and a password holding a `/`, `?` or `#` pushes it out. Both + layers accepted `https://user:12345/x@host/` — which `did_web_from_url` + then turned into the issuer id `did:web:user%3A12345:x%40host`, carried by + every signed credential — and `https://user:secret/x@host/`, which surfaced + the password through urllib's own `Port could not be cast to integer value + as 'secret'` (a message the redaction added below could not reach, since + urllib raises it). Both layers now refuse an `@` or `%40` *anywhere* in a + URL-shaped value, and `did_web_from_url` wraps the port parse and quotes + nothing at all. Values with no authority — `[issuer] email`, local paths — + still accept an `@`. BREAKING (narrow): a `publish_url` whose path contains + an `@`, e.g. `https://host/@name/`, is now refused; once parsed it is + indistinguishable from one hiding a credential (#255). - security(config): reject a `[issuer]` / `[badge_*]` URL that embeds userinfo (`https://user:password@host/`) when the config is loaded. Those URLs are public identifiers: `publish_url` is printed by every publish diff --git a/openbadgeslib/config.ini.example b/openbadgeslib/config.ini.example index f1fb0ba..60a1877 100644 --- a/openbadgeslib/config.ini.example +++ b/openbadgeslib/config.ini.example @@ -30,7 +30,10 @@ mail_from = no-reply@issuer.badge ; Every URL here and in the badge sections is public: it is printed by the ; tools, published in the badge metadata and embedded in signed credentials. ; A URL that embeds credentials (https://user:password@host/) is rejected when -; the config is loaded, so the password cannot leak into any of them. +; the config is loaded, so the password cannot leak into any of them. The rule +; is blunt on purpose: an '@' (or %40) ANYWHERE in one of those URLs is +; refused, because a password holding a '/' hides the '@' from the parser. +; Values that are not URLs (the e-mail below, local paths) are unaffected. [issuer] name = OpenBadge issuer url = https://www.issuer.badge diff --git a/openbadgeslib/confparser.py b/openbadgeslib/confparser.py index 6378f71..e111ef6 100644 --- a/openbadgeslib/confparser.py +++ b/openbadgeslib/confparser.py @@ -290,7 +290,7 @@ def badge_section_config(conf: ConfigParser, def reject_url_userinfo(parser: ConfigParser, config_file: str) -> None: - """Reject any [issuer]/[badge_*] value that is a URL carrying userinfo. + """Reject any [issuer]/[badge_*] value that is a URL containing an ``@``. Those sections hold the identifiers the tools publish: ``publish_url`` is printed by every publish command, joined into the ids written to @@ -303,6 +303,18 @@ def reject_url_userinfo(parser: ConfigParser, config_file: str) -> None: and every consumer (OB1/OB2/OB3, DID derivation, status lists) inherits the guarantee instead of re-checking it. + A published identifier has no legitimate use for an ``@``, so the whole URL + is searched rather than the userinfo component alone: ``urlsplit`` only + reports userinfo when the ``@`` sits inside the authority, and a password + holding a ``/``, ``?`` or ``#`` pushes it into the path, where the parser + stops seeing it as a credential but every downstream consumer still + publishes it verbatim. ``%40`` is refused for the same reason. The cost is + that an innocent ``https://host/@name/`` is refused too — deliberate: after + parsing, the two are the same shape. + + A value with no authority is left alone: an ``[issuer] email`` or a local + path may legitimately hold an ``@`` and is not a dereferenceable URL. + The message names the offending section and key but never echoes the value: printing it would leak the very password the check exists to protect. """ @@ -312,21 +324,22 @@ def reject_url_userinfo(parser: ConfigParser, config_file: str) -> None: and not section.startswith(_PUBLIC_URL_SECTION_PREFIX)): continue for key, value in parser.items(section): + if '@' not in value and '%40' not in value.lower(): + continue try: - authority = urlsplit(value).netloc + has_authority = bool(urlsplit(value).netloc) except ValueError: - # Too malformed for urlsplit (an unbalanced IPv6 bracket). - # Read the authority textually rather than skip the value: - # it is still written out verbatim downstream. - authority = value.partition('//')[2].split('/', 1)[0] - if '@' in authority: + # Too malformed for urlsplit (an unbalanced IPv6 bracket) to + # report an authority. A '//' still makes it a URL, and it is + # written out verbatim downstream, so judge it as one. + has_authority = '//' in value + if has_authority: raise ConfigError( - "Configuration file %s: [%s] %s must not carry userinfo " - "(user:password@host) in its URL. That URL is printed by " - "the tools, published in badge metadata and embedded in " - "signed credentials, so the credential would leak; drop " - "the userinfo and protect the host another way." - % (config_file, section, key)) + "Configuration file %s: [%s] %s must not contain '@' in " + "its URL. That URL is printed by the tools, published in " + "badge metadata and embedded in signed credentials, so a " + "'user:password@' there would leak; a published identifier " + "has no legitimate use for an '@'." % (config_file, section, key)) class ConfParser(): diff --git a/openbadgeslib/ob3/did.py b/openbadgeslib/ob3/did.py index 016384f..0f1b9d8 100644 --- a/openbadgeslib/ob3/did.py +++ b/openbadgeslib/ob3/did.py @@ -280,29 +280,52 @@ def did_web_from_url(url: str) -> str: Exact inverse of the resolution above: the host keeps any port percent-encoded, path segments join with ':', and a bare host resolves at ``/.well-known/did.json`` while a path resolves at ``/did.json``. - Raises ValueError for a non-HTTPS or hostless URL — did:web trusts TLS, - so there is nothing an http:// identifier could safely mean — and for a - URL carrying userinfo, which a did:web authority must not contain (and - which would otherwise leak a ``user:password@`` credential into the DID - embedded in every issued credential). + Raises ValueError for a non-HTTPS, hostless or bad-port URL — did:web + trusts TLS, so there is nothing an http:// identifier could safely mean — + and for any URL carrying an ``@`` (or ``%40``) *anywhere*, which would + otherwise leak a ``user:password@`` credential into the DID embedded in + every issued credential. No rejection quotes the URL back. + + The ``@`` rule is deliberately blunt: it also refuses an innocent + ``https://host/@name/`` base, because once urlsplit has parsed the URL that + shape is indistinguishable from one hiding a credential (see below). """ from urllib.parse import quote, urlsplit parts = urlsplit(url) - # Userinfo first: the two checks below quote the URL in their messages, and - # a CLI prints those to stdout (and into the 'error' field of --json), so - # 'http://user:password@host/' used to publish its own password on the way - # out. What they quote is redacted too, so the guarantee no longer depends - # on this ordering surviving a future edit. - if '@' in parts.netloc: - raise ValueError('did:web URL must not contain userinfo (user:pass@)') - safe = parts._replace(netloc=parts.netloc.rpartition('@')[2]).geturl() + # None of the messages below quotes the URL. The input is publish_url, + # which may carry a credential, and a CLI prints these to stdout and into + # the 'error' field of --json: a check that exists to keep a password out + # of the DID must not publish it on the way out. + # + # The whole URL is searched, not the userinfo component, because urlsplit + # only reports userinfo when the '@' sits inside the authority. A password + # holding a '/', '?' or '#' pushes the marker into the path, query or + # fragment, and the credential then resurfaces either through parts.port — + # whose ValueError quotes the offending value verbatim — or, when what + # precedes the ':' parses as a port, percent-encoded straight into the + # returned DID ('https://user:12345/x@host/' used to yield + # 'did:web:user%3A12345:x%40host'). A did:web base URL has no legitimate + # use for an '@' anywhere, so the blunt rule costs nothing real. + if '@' in url or '%40' in url.lower(): + raise ValueError('did:web URL must not contain "@" (userinfo)') + if not parts.netloc: + # No '//' at all, so urlsplit read everything before the first ':' as + # the scheme — on a schemeless 'user:password@host/' that "scheme" IS + # the username, which is the other reason nothing here is quoted. + raise ValueError('did:web requires an https:// URL with a host') if parts.scheme != 'https': - raise ValueError('did:web requires an https URL, got %r' % (safe,)) + raise ValueError('did:web requires an https URL') + try: + port = parts.port + except ValueError: + # urllib's own message quotes the value it could not parse, which on a + # 'https://user:secret/x@host/' is the password. + raise ValueError('did:web URL has an invalid port') from None if not parts.hostname: - raise ValueError('URL %r has no host' % (safe,)) + raise ValueError('did:web URL has no host') authority = parts.hostname - if parts.port is not None: - authority += ':%d' % parts.port + if port is not None: + authority += ':%d' % port pieces = [quote(authority, safe='')] pieces += [quote(seg, safe='') for seg in parts.path.split('/') if seg] return 'did:web:' + ':'.join(pieces) diff --git a/tests/test_cli_smoke.py b/tests/test_cli_smoke.py index d35c9ec..c607049 100644 --- a/tests/test_cli_smoke.py +++ b/tests/test_cli_smoke.py @@ -3,6 +3,8 @@ import sys from unittest.mock import patch +import pytest + # ── openbadges-init ───────────────────────────────────────────────────────────── @@ -508,13 +510,18 @@ def test_publish_ob2_missing_badge_key_exits_cleanly(tmp_path): assert 'missing required config key' in str(exc.value) -def test_publish_never_leaks_publish_url_userinfo(tmp_path, capsys): - # End-to-end guarantee behind the confparser check: with a - # 'https://user:password@host/' publish_url, no -V publishes anything and - # no byte of the password reaches stdout/stderr or the output tree. Before - # the check, publish printed the URL on success and urljoin'd it into the - # ids of organization.json / badge.json / key.json. - import pytest +@pytest.mark.parametrize('publish_url', [ + 'https://user:hunter2-zzz@example.com/issuer/', # userinfo in the authority + 'https://user:hunter2-zzz/x@example.com/issuer/', # '/' pushes '@' out of it + 'https://user:12345/x@example.com/issuer/', # numeric: encoded into the DID +]) +def test_publish_never_leaks_publish_url_userinfo(tmp_path, capsys, publish_url): + # End-to-end guarantee behind the confparser check: with a credential in + # publish_url, no -V publishes anything and no byte of the password reaches + # stdout/stderr or the output tree. Before the check, publish printed the + # URL on success and urljoin'd it into the ids of organization.json / + # badge.json / key.json. The last two forms are the bypass: urlsplit only + # reports userinfo when the '@' is inside the authority. from pathlib import Path from openbadgeslib import openbadges_publish tests_dir = Path(__file__).parent @@ -523,7 +530,7 @@ def test_publish_never_leaks_publish_url_userinfo(tmp_path, capsys): cfg.write_text('\n'.join([ '[paths]', 'base = %s' % tmp_path, '[issuer]', 'name = I', 'url = https://example.com', - 'publish_url = https://user:%s@example.com/issuer/' % password, + 'publish_url = %s' % publish_url, 'revocationList = revoked.json', 'image = logo.png', 'email = i@x.example', '[badge_1]', 'name = B', 'description = d', 'image = badge.svg', 'criteria = https://example.com/c', 'status_lists = revocation', @@ -539,8 +546,9 @@ def test_publish_never_leaks_publish_url_userinfo(tmp_path, capsys): openbadges_publish.main() assert exc.value.code == 1 captured = capsys.readouterr() - assert password not in captured.out - assert password not in captured.err + for secret in (password, '12345', publish_url): + assert secret not in captured.out + assert secret not in captured.err assert not out.exists() diff --git a/tests/test_confparser.py b/tests/test_confparser.py index 42842a6..3f0e5dc 100644 --- a/tests/test_confparser.py +++ b/tests/test_confparser.py @@ -166,6 +166,54 @@ def test_badge_section_url_keys_rejected(self, tmp_path, key): assert '[badge_1] %s' % key in str(exc.value) assert _PW not in str(exc.value) + @pytest.mark.parametrize('url', [ + 'https://user:12345/x@host/', # numeric "port": encoded into the DID + 'https://user:%s/x@host/' % _PW, # '/' in the password moves the '@' + 'https://user:%s?q@host/' % _PW, # ... and so do '?' and '#' + 'https://user:%s#f@host/' % _PW, + 'https://user%%40host/badges/', # '@' written as %40 ('%%' = literal %) + 'https://host/x?u=a@b', # '@' in the query + ]) + def test_at_sign_outside_the_authority_rejected(self, tmp_path, url): + # urlsplit only reports userinfo when the '@' sits inside the + # authority, so checking netloc alone left a bypass: these all loaded + # cleanly and then leaked — 'https://user:12345/x@host/' became the + # issuer id 'did:web:user%3A12345:x%40host', carried by every signed + # credential, and the non-numeric ones surfaced the password through + # urllib's own "Port could not be cast..." message. + from openbadgeslib.errors import ConfigError + path = _write( + tmp_path, + '[paths]\nbase = .\n\n[issuer]\nname = x\npublish_url = %s\n' % url) + with pytest.raises(ConfigError) as exc: + ConfParser(path).read_conf() + assert _PW not in str(exc.value) + assert '12345' not in str(exc.value) + + def test_at_sign_in_path_rejected_even_when_innocent(self, tmp_path): + # Deliberate false positive, documented on reject_url_userinfo: once + # parsed, 'https://host/@name/' is the same shape as one hiding a + # credential, so the blunt rule wins over the rare legitimate use. + from openbadgeslib.errors import ConfigError + path = _write( + tmp_path, + '[paths]\nbase = .\n\n[issuer]\nname = x\n' + 'publish_url = https://issuer.example/@name/\n') + with pytest.raises(ConfigError): + ConfParser(path).read_conf() + + def test_email_and_paths_with_at_sign_still_accepted(self, tmp_path): + # A value with no authority is not a dereferenceable URL: an address or + # a local path may hold an '@' and must keep loading. + path = _write( + tmp_path, + '[paths]\nbase = .\n\n[issuer]\nname = x\n' + 'email = issuer_mail@issuer.badge\n\n' + '[badge_1]\nmail = ${paths:base}/@inbox/badge_1.txt\n') + conf = ConfParser(path).read_conf() + assert conf['issuer']['email'] == 'issuer_mail@issuer.badge' + assert conf['badge_1']['mail'].endswith('/@inbox/badge_1.txt') + def test_interpolated_userinfo_rejected(self, tmp_path): # The check runs after ${...} resolution, so a credential cannot be # smuggled in through a reference to an unchecked section. diff --git a/tests/test_ob3_did_doc.py b/tests/test_ob3_did_doc.py index b35cc2d..64b2fbe 100644 --- a/tests/test_ob3_did_doc.py +++ b/tests/test_ob3_did_doc.py @@ -51,17 +51,51 @@ def test_rejects_userinfo(self, url): 'http://user:hunter2-zzz@example.com/', # fails the scheme check too 'https://user:hunter2-zzz@', # fails the host check too 'ftp://user:hunter2-zzz@example.com/', + 'user:hunter2-zzz@example.com/', # schemeless: "scheme" == user ]) def test_no_message_echoes_the_password(self, url): - # The userinfo check must run *before* the scheme and host checks, - # which quote the URL: a CLI prints the ValueError to stdout (and into - # the 'error' field of --json), so a password reaching those messages - # would be published by the very error meant to reject it. + # The userinfo check must run *before* the scheme and host checks: a + # CLI prints the ValueError to stdout (and into the 'error' field of + # --json), so a password reaching those messages would be published by + # the very error meant to reject it. with pytest.raises(ValueError) as exc: did_web_from_url(url) assert 'hunter2-zzz' not in str(exc.value) assert 'hunter2-zzz' not in repr(exc.value) + @pytest.mark.parametrize('url,secret', [ + # urlsplit only reports userinfo when the '@' sits inside the + # authority. A password holding a '/', '?' or '#' pushes it into the + # path, and the credential then resurfaced two ways: percent-encoded + # into the returned DID when what precedes the ':' parses as a port + # (this yielded 'did:web:user%3A12345:x%40host'), or through + # parts.port's own ValueError, which quotes the value verbatim. + ('https://user:12345/x@host/', '12345'), + ('https://user:hunter2-zzz/x@host/', 'hunter2-zzz'), + ('https://user:hunter2-zzz?q@host/', 'hunter2-zzz'), + ('https://user:hunter2-zzz#f@host/', 'hunter2-zzz'), + ('https://user%40host/badges/', 'user%40host'), # '@' written %40 + ]) + def test_at_sign_outside_the_authority_is_refused(self, url, secret): + with pytest.raises(ValueError) as exc: + did_web_from_url(url) + assert secret not in str(exc.value) + assert secret not in repr(exc.value) + + def test_invalid_port_does_not_quote_the_value(self): + # urllib's own "Port could not be cast to integer value as 'x'" quotes + # what it could not parse — which on a 'user:password/...' URL is the + # password. The parse must be wrapped and re-raised without it. + with pytest.raises(ValueError) as exc: + did_web_from_url('https://host:hunter2-zzz/badges/') + assert 'hunter2-zzz' not in str(exc.value) + + def test_at_sign_in_path_is_refused_even_when_innocent(self): + # Deliberate false positive, documented on the function: after parsing, + # 'https://host/@name/' is the same shape as one hiding a credential. + with pytest.raises(ValueError): + did_web_from_url('https://example.com/@name/') + @pytest.mark.parametrize('url,doc_url', [ ('https://example.com', 'https://example.com/.well-known/did.json'), ('https://example.com/badges/', 'https://example.com/badges/did.json'), diff --git a/wiki/Configuration.md b/wiki/Configuration.md index 9f7b102..65376cc 100644 --- a/wiki/Configuration.md +++ b/wiki/Configuration.md @@ -100,7 +100,7 @@ revocationList = revoked.json For OB3 the issuer `id` is taken from `publish_url`, falling back to `url` if `publish_url` is absent — unless `did` is set (see the table above). For OB1/OB2, `openbadges-publish` writes an `organization.json` and joins `image` and `revocationList` onto `publish_url`; `-V 2` additionally writes a `CryptographicKey` `key.json` per badge and a conformant `RevocationList`. -> **No credentials in URLs.** Every URL in `[issuer]` and `[badge_]` is a *public* identifier: the tools print it, publish it inside `organization.json` / `badge.json` / `key.json`, derive the issuer's did:web from it and embed it (as `credentialStatus`) in the credentials delivered to recipients. A URL carrying userinfo — `https://user:password@host/` — would leak that password into all of them, so the config is **rejected at load time** with an error naming the key (never the value). Protect a staging host with an IP allow-list or a token the library never sees, not with credentials in `publish_url`. `[smtp] username` / `password` are unaffected: they stay on the issuer's machine. +> **No `@` in published URLs.** Every URL in `[issuer]` and `[badge_]` is a *public* identifier: the tools print it, publish it inside `organization.json` / `badge.json` / `key.json`, derive the issuer's did:web from it and embed it (as `credentialStatus`) in the credentials delivered to recipients. A URL carrying userinfo — `https://user:password@host/` — would leak that password into all of them, so the config is **rejected at load time** with an error naming the key (never the value). The check refuses an `@` (or `%40`) **anywhere** in such a URL, not just in the userinfo slot: a password containing a `/`, `?` or `#` moves the `@` out of the authority, where the parser stops calling it a credential but every consumer still publishes it. The cost is a deliberate false positive — an innocent `https://host/@name/` base is refused too. Protect a staging host with an IP allow-list or a token the library never sees, not with credentials in `publish_url`. Values with no authority are untouched, so `[issuer] email` and local paths may hold an `@`; `[smtp] username` / `password` are unaffected too — they stay on the issuer's machine. ### `[badge_]`