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`).