From 11b54ce8f12b480ad9e30b4ff282e91c89a6d0f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Gonz=C3=A1lez=20Fern=C3=A1ndez?= Date: Wed, 22 Jul 2026 21:31:58 +0200 Subject: [PATCH] security(config): reject publish_url credentials before they are published MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A [issuer]/[badge_*] URL carrying userinfo (https://user:password@host/) is now rejected when the config is loaded. Those URLs are public identifiers: publish_url is printed by every publish command, urljoin'ed into the ids written to the publicly served organization.json / badge.json / key.json (urljoin preserves the user:pass@), turned into the issuer's did:web, and used to build the credentialStatus list URLs signed into every OB3 credential — so a password there leaked into user-visible output, files served on the open web and the badges delivered to recipients alike. Only one narrow path guarded against it: did_web_from_url, reached solely by [issuer] did = auto. And that guard ran its checks in the wrong order — the scheme check quoted the raw URL before the userinfo check was reached, so http://user:password@host/ echoed the password into the CLI error (and into the 'error' field of --json), publishing the credential via the very error meant to reject it. The check now lives once in ConfParser.read_conf(), after ${...} interpolation so a reference cannot smuggle a credential past it, and every consumer (OB1/OB2/OB3 publishing, DID derivation, status lists) inherits it. The ConfigError names the offending section and key but never the value. did_web_from_url keeps its own guard as defence in depth, with the userinfo check moved ahead of the scheme and host checks and what those quote redacted, so the guarantee no longer depends on the ordering. [smtp] username/password are untouched — they never leave the issuer's machine. Behaviour change: a config that protected a staging host with credentials in a published URL no longer loads. Tests assert the rejection across [issuer]/[badge_*] keys, through an interpolated reference, and end to end (publish -V 1/2/3 exits 1, writes no output tree, and prints no byte of the password); no error message anywhere carries it. Closes #252 --- Changelog.txt | 22 ++++++ openbadgeslib/config.ini.example | 4 + openbadgeslib/confparser.py | 51 ++++++++++++ openbadgeslib/ob3/did.py | 14 +++- tests/test_cli_smoke.py | 36 +++++++++ tests/test_confparser.py | 130 +++++++++++++++++++++++++++++++ tests/test_ob3_did_doc.py | 16 ++++ wiki/Configuration.md | 2 + 8 files changed, 271 insertions(+), 4 deletions(-) diff --git a/Changelog.txt b/Changelog.txt index 59a9058..0c5ea5c 100644 --- a/Changelog.txt +++ b/Changelog.txt @@ -77,6 +77,28 @@ 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): 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 + command, `urljoin`ed into the ids written to the publicly served + organization.json / badge.json / key.json (urljoin preserves the + `user:pass@`), turned into the issuer's did:web, and used to build the + `credentialStatus` list URLs signed into every OB3 credential — so a + password there leaked into user-visible output, files served on the open + web and the badges delivered to recipients alike. Only one narrow path + guarded against it (did:web derivation, reached solely by + `[issuer] did = auto`), and that guard echoed the password back in its own + error for a non-https URL. The check now lives once in + `ConfParser.read_conf()` — after `${...}` interpolation, so a reference + cannot smuggle a credential past it — and every consumer (OB1/OB2/OB3 + publishing, DID derivation, status lists) inherits it; the ConfigError + names the offending section and key but never the value. `did_web_from_url` + keeps its own guard as defence in depth, with the userinfo check moved + ahead of the scheme and host checks and their messages redacted. Behaviour + change: a config that protected a staging host with credentials in a + published URL no longer loads — drop the userinfo and use an allow-list or + a token the library never sees. `[smtp] username` / `password` are + untouched; they never leave the issuer's machine (#252). - security(eudi): document and pin the X.509 / eIDAS trust boundary for SD-JWT VC verification. All x5c chain validation is delegated to openvc-core (path building, temporal validity, SAN<->iss binding); openbadgeslib only diff --git a/openbadgeslib/config.ini.example b/openbadgeslib/config.ini.example index 401d233..f1fb0ba 100644 --- a/openbadgeslib/config.ini.example +++ b/openbadgeslib/config.ini.example @@ -27,6 +27,10 @@ mail_from = no-reply@issuer.badge ;password = ; Configuration of the OpenBadges issuer. +; 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. [issuer] name = OpenBadge issuer url = https://www.issuer.badge diff --git a/openbadgeslib/confparser.py b/openbadgeslib/confparser.py index 5fb7b5b..6378f71 100644 --- a/openbadgeslib/confparser.py +++ b/openbadgeslib/confparser.py @@ -282,6 +282,53 @@ def badge_section_config(conf: ConfigParser, ) +#: Sections whose values are the *public* identifiers of a deployment, checked +#: by :func:`reject_url_userinfo`. [smtp] is deliberately excluded: its +#: username/password are credentials that stay on the issuer's machine. +_PUBLIC_URL_SECTIONS = ('issuer',) +_PUBLIC_URL_SECTION_PREFIX = 'badge_' + + +def reject_url_userinfo(parser: ConfigParser, config_file: str) -> None: + """Reject any [issuer]/[badge_*] value that is a URL carrying userinfo. + + Those sections hold the identifiers the tools publish: ``publish_url`` is + printed by every publish command, joined into the ids written to + organization.json / badge.json / key.json (``urljoin`` keeps the + ``user:pass@``), turned into the issuer's did:web, and used to build the + ``credentialStatus`` list URLs embedded in signed OB3 credentials. A + ``https://user:password@host/`` value would therefore carry the credential + into user-visible output, publicly served files and the credentials + delivered to recipients alike — so it is refused once here, at load time, + and every consumer (OB1/OB2/OB3, DID derivation, status lists) inherits the + guarantee instead of re-checking it. + + 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. + """ + from urllib.parse import urlsplit + for section in parser.sections(): + if (section not in _PUBLIC_URL_SECTIONS + and not section.startswith(_PUBLIC_URL_SECTION_PREFIX)): + continue + for key, value in parser.items(section): + try: + authority = 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: + 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)) + + class ConfParser(): def __init__(self, config_file: str = 'config.ini') -> None: self.config_file = config_file @@ -343,6 +390,10 @@ def read_conf(self) -> Optional[ConfigParser]: raise ConfigError( "Configuration file %s has an invalid value: %s" % (self.config_file, exc)) from exc + # Every value is resolved by now, so a ${...} reference cannot smuggle + # a credential past this check. + reject_url_userinfo(self.parser, self.config_file) + return self.parser diff --git a/openbadgeslib/ob3/did.py b/openbadgeslib/ob3/did.py index 4e00611..016384f 100644 --- a/openbadgeslib/ob3/did.py +++ b/openbadgeslib/ob3/did.py @@ -288,12 +288,18 @@ def did_web_from_url(url: str) -> str: """ from urllib.parse import quote, urlsplit parts = urlsplit(url) - if parts.scheme != 'https': - raise ValueError('did:web requires an https URL, got %r' % (url,)) - if parts.username or parts.password: + # 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() + if parts.scheme != 'https': + raise ValueError('did:web requires an https URL, got %r' % (safe,)) if not parts.hostname: - raise ValueError('URL %r has no host' % (url,)) + raise ValueError('URL %r has no host' % (safe,)) authority = parts.hostname if parts.port is not None: authority += ':%d' % parts.port diff --git a/tests/test_cli_smoke.py b/tests/test_cli_smoke.py index da15e8a..d35c9ec 100644 --- a/tests/test_cli_smoke.py +++ b/tests/test_cli_smoke.py @@ -508,6 +508,42 @@ 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 + from pathlib import Path + from openbadgeslib import openbadges_publish + tests_dir = Path(__file__).parent + password = 'hunter2-zzz' + cfg = tmp_path / 'cfg.ini' + 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, + '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', + 'public_key = %s' % (tests_dir / 'test_verify_rsa.pem'), + 'private_key = %s' % (tests_dir / 'test_sign_rsa.pem'), + ]) + '\n') + for version in ('1', '2', '3'): + out = tmp_path / ('out%s' % version) + argv = ['openbadges-publish', '-c', str(cfg), '-o', str(out), + '-V', version] + with patch.object(sys, 'argv', argv): + with pytest.raises(SystemExit) as exc: + openbadges_publish.main() + assert exc.value.code == 1 + captured = capsys.readouterr() + assert password not in captured.out + assert password not in captured.err + assert not out.exists() + + def test_publish_existing_output_exits_cleanly(tmp_path): # -o pointing at an existing path must exit cleanly (SystemExit) with an # operator-facing message, not a raw FileExistsError. diff --git a/tests/test_confparser.py b/tests/test_confparser.py index a175c53..42842a6 100644 --- a/tests/test_confparser.py +++ b/tests/test_confparser.py @@ -87,6 +87,136 @@ def test_config_dir_with_dollar_sign_round_trips(self, tmp_path): def test_nonexistent_file_returns_none(self, tmp_path): assert ConfParser(str(tmp_path / 'missing.ini')).read_conf() is None + +#: A password distinctive enough that its absence from an error message is +#: meaningful (no substring of the config, the paths or the boilerplate). +_PW = 'hunter2-zzz' + + +class TestUrlUserinfoRejected: + """A publish_url (or any other [issuer]/[badge_*] URL) carrying + ``user:password@`` is refused at load time: that URL is printed by the + CLIs, joined into the ids written to organization.json / badge.json / + key.json, turned into the issuer's did:web and embedded in the + credentialStatus of signed OB3 credentials — so accepting it would leak the + credential into user-visible output, public files and issued badges alike. + No message may echo the password.""" + + def test_publish_url_with_userinfo_rejected(self, tmp_path): + from openbadgeslib.errors import ConfigError + path = _write( + tmp_path, + '[paths]\nbase = .\n\n[issuer]\nname = x\n' + 'publish_url = https://user:%s@issuer.example/issuer/\n' % _PW) + with pytest.raises(ConfigError) as exc: + ConfParser(path).read_conf() + assert '[issuer] publish_url' in str(exc.value) + assert _PW not in str(exc.value) + + def test_password_absent_from_repr_too(self, tmp_path): + # repr() is what a traceback shows; neither it nor str() may carry it. + from openbadgeslib.errors import ConfigError + path = _write( + tmp_path, + '[paths]\nbase = .\n\n[issuer]\nname = x\n' + 'publish_url = https://user:%s@issuer.example/\n' % _PW) + with pytest.raises(ConfigError) as exc: + ConfParser(path).read_conf() + assert _PW not in repr(exc.value) + + @pytest.mark.parametrize('url', [ + 'https://user:%s@issuer.example/' % _PW, # user:password@ + 'https://user@issuer.example/', # bare user@ + 'https://@issuer.example/', # empty userinfo + 'http://user:%s@issuer.example/' % _PW, # any scheme, not just https + ]) + def test_every_userinfo_shape_rejected(self, tmp_path, url): + 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): + ConfParser(path).read_conf() + + @pytest.mark.parametrize('key', ['url', 'image']) + def test_other_issuer_url_keys_rejected(self, tmp_path, key): + # publish_url is the documented leak, but [issuer] url is the OB3 + # issuer-id fallback and image is urljoin'd into published metadata: + # every identifier in the section is public. + from openbadgeslib.errors import ConfigError + path = _write( + tmp_path, + '[paths]\nbase = .\n\n[issuer]\nname = x\n%s = ' + 'https://user:%s@issuer.example/x\n' % (key, _PW)) + with pytest.raises(ConfigError) as exc: + ConfParser(path).read_conf() + assert '[issuer] %s' % key in str(exc.value) + assert _PW not in str(exc.value) + + @pytest.mark.parametrize('key', ['image', 'criteria', 'status_base', + 'hosted_assertions_base']) + def test_badge_section_url_keys_rejected(self, tmp_path, key): + from openbadgeslib.errors import ConfigError + path = _write( + tmp_path, + '[paths]\nbase = .\n\n[badge_1]\n%s = ' + 'https://user:%s@issuer.example/badge_1/\n' % (key, _PW)) + with pytest.raises(ConfigError) as exc: + ConfParser(path).read_conf() + assert '[badge_1] %s' % key in str(exc.value) + assert _PW not in str(exc.value) + + 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. + from openbadgeslib.errors import ConfigError + path = _write( + tmp_path, + '[paths]\nbase = .\nhost = user:%s@issuer.example\n\n' + '[issuer]\nname = x\n' + 'publish_url = https://${paths:host}/issuer/\n' % _PW) + with pytest.raises(ConfigError) as exc: + ConfParser(path).read_conf() + assert _PW not in str(exc.value) + + def test_clean_urls_accepted(self, tmp_path): + path = _write( + tmp_path, + '[paths]\nbase = .\n\n[issuer]\nname = x\n' + 'url = https://www.issuer.example\n' + 'email = issuer_mail@issuer.example\n' + 'publish_url = https://issuer.example:8443/issuer/\n' + 'revocationList = revoked.json\n\n' + '[badge_1]\nimage = https://issuer.example/b/badge1.svg\n' + 'private_key = ${paths:base}/keys/sign.pem\n' + 'mail = ${paths:base}/badge_1_mail.txt\n') + conf = ConfParser(path).read_conf() + # A bare mail address is not userinfo, and neither are paths or ports. + assert conf['issuer']['email'] == 'issuer_mail@issuer.example' + assert conf['issuer']['publish_url'] == 'https://issuer.example:8443/issuer/' + + def test_smtp_credentials_are_not_touched(self, tmp_path): + # [smtp] username/password are legitimate secrets that stay on the + # issuer's machine — they are never published, so they are not checked. + path = _write( + tmp_path, + '[paths]\nbase = .\n\n[smtp]\nsmtp_server = localhost\n' + 'username = postmaster\npassword = %s\n' % _PW) + conf = ConfParser(path).read_conf() + assert conf['smtp']['password'] == _PW + + def test_cli_wrapper_exits_without_echoing_the_password(self, tmp_path, capsys): + from openbadgeslib.confparser import read_config_or_exit + path = _write( + tmp_path, + '[paths]\nbase = .\n\n[issuer]\nname = x\n' + 'publish_url = https://user:%s@issuer.example/issuer/\n' % _PW) + with pytest.raises(SystemExit): + read_config_or_exit(path) + out = capsys.readouterr() + assert out.out.startswith('[!]') + assert _PW not in out.out and _PW not in out.err + def test_bad_interpolation_reference_raises_clean_value_error(self, tmp_path): # ExtendedInterpolation resolves ${...} lazily; a bad reference in a # section other than [paths] must surface here, at load time, as a diff --git a/tests/test_ob3_did_doc.py b/tests/test_ob3_did_doc.py index 879fa84..b35cc2d 100644 --- a/tests/test_ob3_did_doc.py +++ b/tests/test_ob3_did_doc.py @@ -39,6 +39,7 @@ def test_rejects_non_https(self, url): @pytest.mark.parametrize('url', [ 'https://user:secret@example.com/', 'https://user@example.com/badges/', + 'https://@example.com/', ]) def test_rejects_userinfo(self, url): # A user:pass@ credential must never be embedded into the DID (which @@ -46,6 +47,21 @@ def test_rejects_userinfo(self, url): with pytest.raises(ValueError): did_web_from_url(url) + @pytest.mark.parametrize('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/', + ]) + 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. + 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,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 6b5fdf5..9f7b102 100644 --- a/wiki/Configuration.md +++ b/wiki/Configuration.md @@ -100,6 +100,8 @@ 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. + ### `[badge_]` Define one section per badge. The part after `badge_` is the badge id you pass to the scripts (e.g. `[badge_1]` is used as `-b 1` / `-g 1`).