From 45171c354de5fac834a96b6a5053bace1163df98 Mon Sep 17 00:00:00 2001 From: Matthias Dellweg Date: Thu, 5 Sep 2024 09:46:20 +0200 Subject: [PATCH 1/4] Stop asking for a username --- CHANGES/+no_username_prompt.removal | 3 +++ pulpcore/cli/common/generic.py | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 CHANGES/+no_username_prompt.removal diff --git a/CHANGES/+no_username_prompt.removal b/CHANGES/+no_username_prompt.removal new file mode 100644 index 000000000..e3d743555 --- /dev/null +++ b/CHANGES/+no_username_prompt.removal @@ -0,0 +1,3 @@ +Removed the prompt for a username. +Starting with this release the user needs to provide the username in the settings, or via `--username` to allow http basic auth. +If no authentication is needed or another authentication mechanism should be used, it can be omitted. diff --git a/pulpcore/cli/common/generic.py b/pulpcore/cli/common/generic.py index 7e043bb37..bc010087a 100644 --- a/pulpcore/cli/common/generic.py +++ b/pulpcore/cli/common/generic.py @@ -221,7 +221,8 @@ def __init__(self, pulp_ctx: PulpCLIContext): def basic_auth(self, scopes: t.List[str]) -> t.Optional[requests.auth.AuthBase]: if self.pulp_ctx.username is None: - self.pulp_ctx.username = click.prompt("Username") + # No username -> No basic auth. + return None if self.pulp_ctx.password is None: # TODO give the user a chance to opt out. if SECRET_STORAGE: @@ -236,7 +237,8 @@ def oauth2_client_credentials_auth( self, flow: t.Any, scopes: t.List[str] ) -> t.Optional[requests.auth.AuthBase]: if self.pulp_ctx.username is None: - self.pulp_ctx.username = click.prompt("Username/ClientID") + # No username -> No basic auth. + return None if self.pulp_ctx.password is None: self.pulp_ctx.password = click.prompt("Password/ClientSecret") From b60051514a0cc78576c5090d9cf5b0cc25cdeb99 Mon Sep 17 00:00:00 2001 From: Matthias Dellweg Date: Thu, 5 Sep 2024 12:35:32 +0200 Subject: [PATCH 2/4] Change oauth2 client parameters This introduces --client-id and --client-auth. Users can now infer the authentication to use in face of multiple available security proposals. --- CHANGES/+oauth2_client_credentials.feature | 2 ++ pulp_cli/__init__.py | 8 +++++-- pulp_cli/config.py | 10 ++++++++- pulpcore/cli/common/generic.py | 25 +++++++++++++++------- 4 files changed, 34 insertions(+), 11 deletions(-) create mode 100644 CHANGES/+oauth2_client_credentials.feature diff --git a/CHANGES/+oauth2_client_credentials.feature b/CHANGES/+oauth2_client_credentials.feature new file mode 100644 index 000000000..35904baac --- /dev/null +++ b/CHANGES/+oauth2_client_credentials.feature @@ -0,0 +1,2 @@ +Changed the way OAuth2 Client Credentials are provided to give the user some choice over the authentication to use. +The new parameters `--client-id` and `--client-secret` were added and `--username`, `--password` are now restricted to HTTP Basic. diff --git a/pulp_cli/__init__.py b/pulp_cli/__init__.py index c1c7bf2b1..7b57d22a2 100644 --- a/pulp_cli/__init__.py +++ b/pulp_cli/__init__.py @@ -188,6 +188,8 @@ def main( headers: t.List[str], username: t.Optional[str], password: t.Optional[str], + client_id: t.Optional[str], + client_secret: t.Optional[str], cert: t.Optional[str], key: t.Optional[str], verify_ssl: bool, @@ -206,8 +208,6 @@ def _debug_callback(level: int, x: str) -> None: api_kwargs = dict( base_url=base_url, headers=dict((header.split(":", maxsplit=1) for header in headers)), - username=username, - password=password, cert=cert, key=key, validate_certs=verify_ssl, @@ -224,6 +224,10 @@ def _debug_callback(level: int, x: str) -> None: format=format, background_tasks=background, timeout=timeout, + username=username, + password=password, + oauth2_client_id=client_id, + oauth2_client_secret=client_secret, ) diff --git a/pulp_cli/config.py b/pulp_cli/config.py index 07d8d61b0..fd4c08062 100644 --- a/pulp_cli/config.py +++ b/pulp_cli/config.py @@ -29,6 +29,8 @@ "headers", "username", "password", + "client_id", + "client_secret", "cert", "key", "verify_ssl", @@ -59,6 +61,8 @@ ), click.option("--username", default=None, help=_("Username on pulp server")), click.option("--password", default=None, help=_("Password on pulp server")), + click.option("--client-id", default=None, help=_("OAuth2 client ID")), + click.option("--client-secret", default=None, help=_("OAuth2 client secret")), click.option("--cert", default="", help=_("Path to client certificate")), click.option( "--key", @@ -153,7 +157,11 @@ def validate_config(config: t.Dict[str, t.Any], strict: bool = False) -> None: if unknown_settings: errors.append(_("Unknown settings: '{}'.").format("','".join(unknown_settings))) if strict: - missing_settings = set(SETTINGS) - set(config.keys()) - {"plugins", "username", "password"} + missing_settings = ( + set(SETTINGS) + - set(config.keys()) + - {"plugins", "username", "password", "client_id", "client_secret"} + ) if missing_settings: errors.append(_("Missing settings: '{}'.").format("','".join(missing_settings))) if errors: diff --git a/pulpcore/cli/common/generic.py b/pulpcore/cli/common/generic.py index bc010087a..bfe0f584b 100644 --- a/pulpcore/cli/common/generic.py +++ b/pulpcore/cli/common/generic.py @@ -132,9 +132,15 @@ def __init__( timeout: int, format: str, domain: str = "default", + username: t.Optional[str] = None, + password: t.Optional[str] = None, + oauth2_client_id: t.Optional[str] = None, + oauth2_client_secret: t.Optional[str] = None, ) -> None: - self.username = api_kwargs.pop("username", None) - self.password = api_kwargs.pop("password", None) + self.username = username + self.password = password + self.oauth2_client_id = oauth2_client_id + self.oauth2_client_secret = oauth2_client_secret if not api_kwargs.get("cert"): api_kwargs["auth_provider"] = PulpCLIAuthProvider(pulp_ctx=self) super().__init__( @@ -170,6 +176,8 @@ def output_result(self, result: t.Any) -> None: class SecretStorageBasicAuth(requests.auth.AuthBase): def __init__(self, pulp_ctx: PulpCLIContext): self.pulp_ctx = pulp_ctx + assert self.pulp_ctx.username is not None + self.attr: t.Dict[str, str] = { "service": "pulp-cli", "base_url": self.pulp_ctx.api.base_url, @@ -199,6 +207,7 @@ def response_hook(self, response: requests.Response, **kwargs: t.Any) -> request return response def __call__(self, request: requests.PreparedRequest) -> requests.PreparedRequest: + assert self.pulp_ctx.username is not None with closing(secretstorage.dbus_init()) as connection: collection = secretstorage.get_default_collection(connection) item = next(collection.search_items(self.attr), None) @@ -236,15 +245,15 @@ def basic_auth(self, scopes: t.List[str]) -> t.Optional[requests.auth.AuthBase]: def oauth2_client_credentials_auth( self, flow: t.Any, scopes: t.List[str] ) -> t.Optional[requests.auth.AuthBase]: - if self.pulp_ctx.username is None: - # No username -> No basic auth. + if self.pulp_ctx.oauth2_client_id is None: + # No client_id -> No oauth2 client credentials. return None - if self.pulp_ctx.password is None: - self.pulp_ctx.password = click.prompt("Password/ClientSecret") + if self.pulp_ctx.oauth2_client_secret is None: + self.pulp_ctx.oauth2_client_secret = click.prompt("Client Secret") return OAuth2ClientCredentialsAuth( - client_id=self.pulp_ctx.username, - client_secret=self.pulp_ctx.password, + client_id=self.pulp_ctx.oauth2_client_id, + client_secret=self.pulp_ctx.oauth2_client_secret, token_url=flow["tokenUrl"], # Try to request all possible scopes. scopes=flow["scopes"], From c49ad4d224c9e66c3e89cf37e97ff9ab1cbd9549 Mon Sep 17 00:00:00 2001 From: Matthias Dellweg Date: Mon, 9 Sep 2024 20:32:52 +0200 Subject: [PATCH 3/4] BasicAuth instead of post data for OAuth2 token According to RFC6749 Section 2.3.1 all token servers are required to support http basic auth. Instead supporting the credentials as post data is specified as optional. Furthermore the RCF discourages using the latter. --- CHANGES/pulp-glue/+oauth2_token_basicauth.bugfix | 1 + pulp-glue/pulp_glue/common/authentication.py | 7 ++----- pulp-glue/tests/test_authentication.py | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) create mode 100644 CHANGES/pulp-glue/+oauth2_token_basicauth.bugfix diff --git a/CHANGES/pulp-glue/+oauth2_token_basicauth.bugfix b/CHANGES/pulp-glue/+oauth2_token_basicauth.bugfix new file mode 100644 index 000000000..da1581b22 --- /dev/null +++ b/CHANGES/pulp-glue/+oauth2_token_basicauth.bugfix @@ -0,0 +1 @@ +Use BasicAuth for token retrieval to comply with RFC6749. diff --git a/pulp-glue/pulp_glue/common/authentication.py b/pulp-glue/pulp_glue/common/authentication.py index 4e198b4b8..32f8c1d16 100644 --- a/pulp-glue/pulp_glue/common/authentication.py +++ b/pulp-glue/pulp_glue/common/authentication.py @@ -17,8 +17,7 @@ def __init__( token_url: str, scopes: t.Optional[t.List[str]] = None, ): - self.client_id = client_id - self.client_secret = client_secret + self.token_auth = requests.auth.HTTPBasicAuth(client_id, client_secret) self.token_url = token_url self.scopes = scopes @@ -76,15 +75,13 @@ def handle401( def retrieve_token(self) -> None: data = { - "client_id": self.client_id, - "client_secret": self.client_secret, "grant_type": "client_credentials", } if self.scopes: data["scope"] = " ".join(self.scopes) - response: requests.Response = requests.post(self.token_url, data=data) + response: requests.Response = requests.post(self.token_url, data=data, auth=self.token_auth) response.raise_for_status() diff --git a/pulp-glue/tests/test_authentication.py b/pulp-glue/tests/test_authentication.py index 6a1f1f08c..56c271fb4 100644 --- a/pulp-glue/tests/test_authentication.py +++ b/pulp-glue/tests/test_authentication.py @@ -16,7 +16,7 @@ def raise_for_status(self): def json(self): return {"expires_in": 1, "access_token": "aaa"} - def _requests_post_mocked(url: str, data: t.Dict[str, t.Any]): + def _requests_post_mocked(url: str, data: t.Dict[str, t.Any], **kwargs: t.Any): assert "scope" not in data return OAuth2MockResponse() From e9535ded3434c93d7f3546388d77fc3b7bdea3e7 Mon Sep 17 00:00:00 2001 From: Matthias Dellweg Date: Tue, 10 Sep 2024 13:30:43 +0200 Subject: [PATCH 4/4] Memoize auth objects Auth objects provided by the pulpcli auth provider are memoized. This way, no password needs to be written back to the pulp_ctx variable and the oauth token can be cached in memory for the lifetime of the context. --- CHANGES/+memoize_auth.feature | 1 + pulpcore/cli/common/generic.py | 106 ++++++++++++++++++++------------- 2 files changed, 64 insertions(+), 43 deletions(-) create mode 100644 CHANGES/+memoize_auth.feature diff --git a/CHANGES/+memoize_auth.feature b/CHANGES/+memoize_auth.feature new file mode 100644 index 000000000..fbe99e6a0 --- /dev/null +++ b/CHANGES/+memoize_auth.feature @@ -0,0 +1 @@ +Added memoization to CLI auth provider. This helps to reuse a retrieved oauth token for the lifetime of the process. diff --git a/pulpcore/cli/common/generic.py b/pulpcore/cli/common/generic.py index bfe0f584b..6cb5bf81a 100644 --- a/pulpcore/cli/common/generic.py +++ b/pulpcore/cli/common/generic.py @@ -177,6 +177,7 @@ class SecretStorageBasicAuth(requests.auth.AuthBase): def __init__(self, pulp_ctx: PulpCLIContext): self.pulp_ctx = pulp_ctx assert self.pulp_ctx.username is not None + self.password: t.Optional[str] = None self.attr: t.Dict[str, str] = { "service": "pulp-cli", @@ -186,78 +187,97 @@ def __init__(self, pulp_ctx: PulpCLIContext): } def response_hook(self, response: requests.Response, **kwargs: t.Any) -> requests.Response: + # Example adapted from: # https://docs.python-requests.org/en/latest/_modules/requests/auth/#HTTPDigestAuth if 200 <= response.status_code < 300 and not self.password_in_manager: if click.confirm(_("Add password to password manager?")): - assert isinstance(self.pulp_ctx.password, str) + assert isinstance(self.password, str) with closing(secretstorage.dbus_init()) as connection: collection = secretstorage.get_default_collection(connection) collection.create_item( - "Pulp CLI", self.attr, self.pulp_ctx.password.encode(), replace=True + "Pulp CLI", self.attr, self.password.encode(), replace=True ) elif response.status_code == 401 and self.password_in_manager: - with closing(secretstorage.dbus_init()) as connection: - collection = secretstorage.get_default_collection(connection) - item = next(collection.search_items(self.attr), None) - if item: - if click.confirm(_("Remove failed password from password manager?")): + if click.confirm(_("Remove failed password from password manager?")): + with closing(secretstorage.dbus_init()) as connection: + collection = secretstorage.get_default_collection(connection) + item = next(collection.search_items(self.attr), None) + if item is not None: item.delete() - self.pulp_ctx.password = None + self.password = None return response def __call__(self, request: requests.PreparedRequest) -> requests.PreparedRequest: assert self.pulp_ctx.username is not None - with closing(secretstorage.dbus_init()) as connection: - collection = secretstorage.get_default_collection(connection) - item = next(collection.search_items(self.attr), None) - if item: - self.pulp_ctx.password = item.get_secret().decode() - self.password_in_manager = True - else: - self.pulp_ctx.password = str(click.prompt("Password", hide_input=True)) - self.password_in_manager = False + if self.password is None: + with closing(secretstorage.dbus_init()) as connection: + collection = secretstorage.get_default_collection(connection) + item = next(collection.search_items(self.attr), None) + if item: + self.password = item.get_secret().decode() + self.password_in_manager = True + else: + self.password = str(click.prompt("Password", hide_input=True)) + self.password_in_manager = False request.register_hook("response", self.response_hook) # type: ignore [no-untyped-call] return requests.auth.HTTPBasicAuth( # type: ignore [no-any-return] - self.pulp_ctx.username, - self.pulp_ctx.password, + self.pulp_ctx.username, self.password )(request) class PulpCLIAuthProvider(AuthProviderBase): def __init__(self, pulp_ctx: PulpCLIContext): self.pulp_ctx = pulp_ctx + self._memoized: t.Dict[str, t.Optional[requests.auth.AuthBase]] = {} def basic_auth(self, scopes: t.List[str]) -> t.Optional[requests.auth.AuthBase]: - if self.pulp_ctx.username is None: - # No username -> No basic auth. - return None - if self.pulp_ctx.password is None: - # TODO give the user a chance to opt out. - if SECRET_STORAGE: - # We could just try to fetch the password here, - # but we want to get a grip on the response_hook. - return SecretStorageBasicAuth(self.pulp_ctx) + if "BASIC_AUTH" not in self._memoized: + if self.pulp_ctx.username is None: + # No username -> No basic auth. + self._memoized["BASIC_AUTH"] = None + elif self.pulp_ctx.password is None: + # TODO give the user a chance to opt out. + if SECRET_STORAGE: + # We could just try to fetch the password here, + # but we want to get a grip on the response_hook. + self._memoized["BASIC_AUTH"] = SecretStorageBasicAuth(self.pulp_ctx) + else: + self._memoized["BASIC_AUTH"] = requests.auth.HTTPBasicAuth( + self.pulp_ctx.username, click.prompt("Password", hide_input=True) + ) else: - self.pulp_ctx.password = click.prompt("Password", hide_input=True) - return requests.auth.HTTPBasicAuth(self.pulp_ctx.username, self.pulp_ctx.password) + self._memoized["BASIC_AUTH"] = requests.auth.HTTPBasicAuth( + self.pulp_ctx.username, self.pulp_ctx.password + ) + return self._memoized["BASIC_AUTH"] def oauth2_client_credentials_auth( self, flow: t.Any, scopes: t.List[str] ) -> t.Optional[requests.auth.AuthBase]: - if self.pulp_ctx.oauth2_client_id is None: - # No client_id -> No oauth2 client credentials. - return None - if self.pulp_ctx.oauth2_client_secret is None: - self.pulp_ctx.oauth2_client_secret = click.prompt("Client Secret") - - return OAuth2ClientCredentialsAuth( - client_id=self.pulp_ctx.oauth2_client_id, - client_secret=self.pulp_ctx.oauth2_client_secret, - token_url=flow["tokenUrl"], - # Try to request all possible scopes. - scopes=flow["scopes"], - ) + token_url = flow["tokenUrl"] + key = "OAUTH2_CLIENT_CREDENTIALS;" + token_url + ";" + ":".join(scopes) + if key not in self._memoized: + if self.pulp_ctx.oauth2_client_id is None: + # No client_id -> No oauth2 client credentials. + self._memoized[key] = None + elif self.pulp_ctx.oauth2_client_secret is None: + self._memoized[key] = OAuth2ClientCredentialsAuth( + client_id=self.pulp_ctx.oauth2_client_id, + client_secret=click.prompt("Client Secret"), + token_url=flow["tokenUrl"], + # Try to request all possible scopes. + scopes=flow["scopes"], + ) + else: + self._memoized[key] = OAuth2ClientCredentialsAuth( + client_id=self.pulp_ctx.oauth2_client_id, + client_secret=self.pulp_ctx.oauth2_client_secret, + token_url=flow["tokenUrl"], + # Try to request all possible scopes. + scopes=flow["scopes"], + ) + return self._memoized[key] ##############################################################################