From b599c7249d495389cf4519e8e69ac3141c94ad5e Mon Sep 17 00:00:00 2001 From: Matthieu Huin Date: Mon, 11 Mar 2019 11:18:48 +0100 Subject: [PATCH] authentication config: add optional max_validity_time, skew The Zuul admin can configure authenticators with an optional "max_validity_time" field, which is the maximum age in seconds for a valid authentication token. By default there is no maximum age set for tokens, except the one deduced from the token's "exp" claim. If "max_validity" is set, tokens without an "iat" claim will be rejected. This is meant as an extra security to avoid accidentally issueing very long lived tokens through the CLI. The "skew" field can be used to mitigate clocks discrepancies between Zuul and a JWT emitter. Change-Id: I9351ca016b60050b5f3b3950b840d5f719e919ce --- doc/source/admin/components.rst | 7 + .../specs/tenant-scoped-admin-web-API.rst | 7 +- etc/zuul.conf-sample | 2 + .../fixtures/zuul-admin-web-token-expiry.conf | 32 ++++ tests/unit/test_web.py | 156 ++++++++++++++++++ zuul/cmd/client.py | 4 +- zuul/driver/auth/jwt.py | 31 +++- 7 files changed, 236 insertions(+), 3 deletions(-) create mode 100644 tests/fixtures/zuul-admin-web-token-expiry.conf diff --git a/doc/source/admin/components.rst b/doc/source/admin/components.rst index a6cdaebb4b..10c99bba11 100644 --- a/doc/source/admin/components.rst +++ b/doc/source/admin/components.rst @@ -991,6 +991,13 @@ protected endpoints and configure JWT validation: of time in seconds. This is useful if the Zuul operator has no control over the service issueing JWTs, and the tokens are too long-lived. + .. attr:: skew + :default: 0 + + Optional integer value to compensate for skew between Zuul's and the + JWT emitter's respective clocks. Use a negative value if Zuul's clock + is running behind. + This section can be repeated as needed with different authenticators, allowing access to privileged API actions from several JWT issuers. diff --git a/doc/source/developer/specs/tenant-scoped-admin-web-API.rst b/doc/source/developer/specs/tenant-scoped-admin-web-API.rst index fe060bbf9a..fb60aa2e50 100644 --- a/doc/source/developer/specs/tenant-scoped-admin-web-API.rst +++ b/doc/source/developer/specs/tenant-scoped-admin-web-API.rst @@ -501,6 +501,8 @@ Here is an example defining the three supported types of authenticators: realm=openstack # (optional) Ensure a Token cannot be valid for longer than this amount of time, in seconds max_validity_time = 1800000 + # (optional) Account for skew between clocks, in seconds + skew = 3 # asymmetrical encryption [auth "my_oidc_idp"] @@ -517,6 +519,8 @@ Here is an example defining the three supported types of authenticators: realm=openstack # (optional) Ensure a Token cannot be valid for longer than this amount of time, in seconds max_validity_time = 1800000 + # (optional) Account for skew between clocks, in seconds + skew = 3 # asymmetrical encryption using JWKS for validation # The signing secret being known to the Identity Provider only, this @@ -532,7 +536,8 @@ Here is an example defining the three supported types of authenticators: uid_claim=name # Auth realm, used in 401 error messages realm=openstack - + # (optional) Account for skew between clocks, in seconds + skew = 3 Implementation ============== diff --git a/etc/zuul.conf-sample b/etc/zuul.conf-sample index c0916c6c43..dd1d37e5ae 100644 --- a/etc/zuul.conf-sample +++ b/etc/zuul.conf-sample @@ -52,6 +52,8 @@ default=true client_id=zuul.example.com issuer_id=zuul_operator secret=NoDanaOnlyZuul +max_validity_time=36000 +skew=0 [connection gerrit] driver=gerrit diff --git a/tests/fixtures/zuul-admin-web-token-expiry.conf b/tests/fixtures/zuul-admin-web-token-expiry.conf new file mode 100644 index 0000000000..7123455a0b --- /dev/null +++ b/tests/fixtures/zuul-admin-web-token-expiry.conf @@ -0,0 +1,32 @@ +[gearman] +server=127.0.0.1 + +[scheduler] +tenant_config=main.yaml +relative_priority=true + +[merger] +git_dir=/tmp/zuul-test/merger-git +git_user_email=zuul@example.com +git_user_name=zuul + +[executor] +git_dir=/tmp/zuul-test/executor-git + +[connection gerrit] +driver=gerrit +server=review.example.com +user=jenkins +sshkey=fake_id_rsa_path + +[web] +static_cache_expiry=1200 + +[auth zuul_operator] +driver=HS256 +allow_authz_override=true +realm=zuul.example.com +client_id=zuul.example.com +issuer_id=zuul_operator +secret=NoDanaOnlyZuul +max_validity_time=5 diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py index 836e94c2ed..d8443c50e2 100644 --- a/tests/unit/test_web.py +++ b/tests/unit/test_web.py @@ -1716,6 +1716,162 @@ class TestTenantScopedWebApiWithAuthRules(BaseTestWeb): "%s got %s" % (authz['sub'], data)) +class TestTenantScopedWebApiTokenWithExpiry(BaseTestWeb): + config_file = 'zuul-admin-web-token-expiry.conf' + + def test_iat_claim_mandatory(self): + """Test that the 'iat' claim is mandatory when + max_validity_time is set""" + authz = {'iss': 'zuul_operator', + 'sub': 'testuser', + 'aud': 'zuul.example.com', + 'zuul': { + 'admin': ['tenant-one', ] + }, + 'exp': time.time() + 3600} + token = jwt.encode(authz, key='NoDanaOnlyZuul', + algorithm='HS256').decode('utf-8') + resp = self.post_url( + "api/tenant/tenant-one/project/org/project/autohold", + headers={'Authorization': 'Bearer %s' % token}, + json={'job': 'project-test1', + 'count': 1, + 'reason': 'because', + 'node_hold_expiration': 36000}) + self.assertEqual(401, resp.status_code) + resp = self.post_url( + "api/tenant/tenant-one/project/org/project/enqueue", + headers={'Authorization': 'Bearer %s' % token}, + json={'trigger': 'gerrit', + 'change': '2,1', + 'pipeline': 'check'}) + self.assertEqual(401, resp.status_code) + resp = self.post_url( + "api/tenant/tenant-one/project/org/project/enqueue", + headers={'Authorization': 'Bearer %s' % token}, + json={'trigger': 'gerrit', + 'ref': 'abcd', + 'newrev': 'aaaa', + 'oldrev': 'bbbb', + 'pipeline': 'check'}) + self.assertEqual(401, resp.status_code) + + def test_token_from_the_future(self): + authz = {'iss': 'zuul_operator', + 'sub': 'testuser', + 'aud': 'zuul.example.com', + 'zuul': { + 'admin': ['tenant-one', ], + }, + 'exp': time.time() + 7200, + 'iat': time.time() + 3600} + token = jwt.encode(authz, key='NoDanaOnlyZuul', + algorithm='HS256').decode('utf-8') + resp = self.post_url( + "api/tenant/tenant-one/project/org/project/autohold", + headers={'Authorization': 'Bearer %s' % token}, + json={'job': 'project-test1', + 'count': 1, + 'reason': 'because', + 'node_hold_expiration': 36000}) + self.assertEqual(401, resp.status_code) + resp = self.post_url( + "api/tenant/tenant-one/project/org/project/enqueue", + headers={'Authorization': 'Bearer %s' % token}, + json={'trigger': 'gerrit', + 'change': '2,1', + 'pipeline': 'check'}) + self.assertEqual(401, resp.status_code) + resp = self.post_url( + "api/tenant/tenant-one/project/org/project/enqueue", + headers={'Authorization': 'Bearer %s' % token}, + json={'trigger': 'gerrit', + 'ref': 'abcd', + 'newrev': 'aaaa', + 'oldrev': 'bbbb', + 'pipeline': 'check'}) + self.assertEqual(401, resp.status_code) + + def test_token_expired(self): + authz = {'iss': 'zuul_operator', + 'sub': 'testuser', + 'aud': 'zuul.example.com', + 'zuul': { + 'admin': ['tenant-one', ], + }, + 'exp': time.time() + 3600, + 'iat': time.time()} + token = jwt.encode(authz, key='NoDanaOnlyZuul', + algorithm='HS256').decode('utf-8') + time.sleep(10) + resp = self.post_url( + "api/tenant/tenant-one/project/org/project/autohold", + headers={'Authorization': 'Bearer %s' % token}, + json={'job': 'project-test1', + 'count': 1, + 'reason': 'because', + 'node_hold_expiration': 36000}) + self.assertEqual(401, resp.status_code) + resp = self.post_url( + "api/tenant/tenant-one/project/org/project/enqueue", + headers={'Authorization': 'Bearer %s' % token}, + json={'trigger': 'gerrit', + 'change': '2,1', + 'pipeline': 'check'}) + self.assertEqual(401, resp.status_code) + resp = self.post_url( + "api/tenant/tenant-one/project/org/project/enqueue", + headers={'Authorization': 'Bearer %s' % token}, + json={'trigger': 'gerrit', + 'ref': 'abcd', + 'newrev': 'aaaa', + 'oldrev': 'bbbb', + 'pipeline': 'check'}) + self.assertEqual(401, resp.status_code) + + def test_autohold(self): + """Test that autohold can be set through the admin web interface""" + args = {"reason": "some reason", + "count": 1, + 'job': 'project-test2', + 'change': None, + 'ref': None, + 'node_hold_expiration': None} + authz = {'iss': 'zuul_operator', + 'aud': 'zuul.example.com', + 'sub': 'testuser', + 'zuul': { + 'admin': ['tenant-one', ], + }, + 'exp': time.time() + 3600, + 'iat': time.time()} + token = jwt.encode(authz, key='NoDanaOnlyZuul', + algorithm='HS256').decode('utf-8') + req = self.post_url( + 'api/tenant/tenant-one/project/org/project/autohold', + headers={'Authorization': 'Bearer %s' % token}, + json=args) + self.assertEqual(200, req.status_code, req.text) + data = req.json() + self.assertEqual(True, data) + + # Check result in rpc client + client = zuul.rpcclient.RPCClient('127.0.0.1', + self.gearman_server.port) + self.addCleanup(client.shutdown) + + autohold_requests = client.autohold_list() + self.assertNotEqual([], autohold_requests) + self.assertEqual(1, len(autohold_requests)) + + ah_request = autohold_requests[0] + self.assertEqual('tenant-one', ah_request['tenant']) + self.assertIn('org/project', ah_request['project']) + self.assertEqual('project-test2', ah_request['job']) + self.assertEqual(".*", ah_request['ref_filter']) + self.assertEqual("some reason", ah_request['reason']) + + class TestWebMulti(BaseTestWeb): config_file = 'zuul-gerrit-github.conf' diff --git a/zuul/cmd/client.py b/zuul/cmd/client.py index 8e01f8084a..82487ff2a1 100755 --- a/zuul/cmd/client.py +++ b/zuul/cmd/client.py @@ -540,7 +540,9 @@ class Client(zuul.cmd.ZuulApp): print('"%s" authenticator configuration not found.' % self.args.auth_config) sys.exit(1) - token = {'exp': time.time() + self.args.expires_in, + now = time.time() + token = {'iat': now, + 'exp': now + self.args.expires_in, 'iss': get_default(self.config, auth_section, 'issuer_id'), 'aud': get_default(self.config, auth_section, 'client_id'), 'sub': self.args.user, diff --git a/zuul/driver/auth/jwt.py b/zuul/driver/auth/jwt.py index 2579c3a33d..3517ff3e5b 100644 --- a/zuul/driver/auth/jwt.py +++ b/zuul/driver/auth/jwt.py @@ -14,6 +14,7 @@ # under the License. import logging +import math import time import jwt import requests @@ -36,11 +37,21 @@ class JWTAuthenticator(AuthenticatorInterface): self.audience = conf.get('client_id') self.realm = conf.get('realm') self.allow_authz_override = conf.get('allow_authz_override', False) + try: + self.skew = int(conf.get('skew', 0)) + except Exception: + raise ValueError( + 'skew must be an integer, got %s' % conf.get('skew')) if isinstance(self.allow_authz_override, str): if self.allow_authz_override.lower() == 'true': self.allow_authz_override = True else: self.allow_authz_override = False + try: + self.max_validity_time = float(conf.get('max_validity_time', + math.inf)) + except ValueError: + raise ValueError('"max_validity_time" must be a numerical value') def _decode(self, rawToken): raise NotImplementedError @@ -68,13 +79,31 @@ class JWTAuthenticator(AuthenticatorInterface): raise exceptions.AuthTokenUnauthorizedException( realm=self.realm, msg=e) + # Missing claim tests if not all(x in decoded for x in ['aud', 'iss', 'exp', 'sub']): raise exceptions.MissingClaimError(realm=self.realm) + if self.max_validity_time < math.inf and 'iat' not in decoded: + raise exceptions.MissingClaimError( + msg='Missing "iat" claim', + realm=self.realm) if self.uid_claim not in decoded: raise exceptions.MissingUIDClaimError(realm=self.realm) + # Time related tests expires = decoded.get('exp', 0) - if expires < time.time(): + issued_at = decoded.get('iat', 0) + now = time.time() + if issued_at + self.skew > now: + raise exceptions.AuthTokenUnauthorizedException( + msg='"iat" claim set in the future', + realm=self.realm + ) + if now - issued_at > self.max_validity_time: + raise exceptions.TokenExpiredError( + msg='Token was issued too long ago', + realm=self.realm) + if expires + self.skew < now: raise exceptions.TokenExpiredError(realm=self.realm) + # Zuul-specific claims tests zuul_claims = decoded.get('zuul', {}) admin_tenants = zuul_claims.get('admin', []) if not isinstance(admin_tenants, list):