feat(auth): implement iap auth token #637

jonathan-johnston · 2023-09-14T14:25:47Z

Summary

Implement an IAP auth token class IapToken which acts in a similar way to our OAuth 2.0 access token class Token. I've refactored out the basic components of the two as BaseToken, which includes:

Top-level methods like get
Reading in and interpreting service files
Refreshing the token before the stated expiry
General token management
Handling the Session, ex. context manager dunder methods, close, etc.

In terms of the IapToken class, its general behaviour is:

Manage tokens for a single IAP-secured service, since IAP is secured on a per-service level
When running in the Google infrastructure, directly fetch the identity token
When running with a SA file, fetch the IAP audience (client ID) then fetch the identity token for that IAP audience
When running as a user, similarly fetch the IAP audience (client ID) before impersonating a service account to generate the identity token

Other changes include:

Implementing a HEAD method on Session and piping through allow_redirects, for this specific application it aids in explicitly stopping at the 3xx response to parse out the location header
Implementing a TokenResponse dataclass to handle differences between the HTTP response formats of various requests (ex. GCE metadata server for IAP vs. GCE metadata server for OAuth) so that they can all be handled similarly by the BaseToken class' token management

Checklist:

Included references to GCP docs in comments and docstrings
Tested this manually via alpha version (except for IapToken against GCE metadata server; however I verified that the URL is correct through manual testing)

jonathan-johnston · 2023-09-14T21:04:50Z

@TheKevJames Thoughts on building out a thin IAP-secured service on dialpad-oss to allow us to write an integration test for IapToken?

TheKevJames

Please update the usage docs to include the IapToken workflow.

auth/gcloud/aio/auth/session.py

TheKevJames · 2023-09-15T14:07:48Z

auth/gcloud/aio/auth/token.py

+@dataclass
+class TokenResponse:
+    value: str
+    expires_in: int = GCLOUD_TOKEN_DURATION


Looks like there are fairly limited places where this default gets used, all within the new IAP flow. It might be a bit easier to reason about if this value is passed in explicitly in that case, since (if I'm reading this correctly!) it looks like there might be two values at play here: previously, we passed this value in and Google created tokens of the designated duration. With IAP, though, it seems like we're finding the tokens have this duration even though we aren't passing this value in (for gce metadata and authorized users)?

For some reason the responses from IAP token requests are all over the place and they don't provide you with an expiry. Maybe there's some way to force them to, but in practice the tokens do expire within 1 hour.

I've moved this default TTL to the class-level and removed the default from this TokenResponse dataclass. LMK what you think!

TheKevJames · 2023-09-15T14:09:46Z

auth/gcloud/aio/auth/token.py

+
+        https://cloud.google.com/iap/docs/authentication-howto#obtaining_an_oidc_token_in_all_other_cases
+        """
+        async def get_access_token(timeout: int) -> str:


nit: consider de-nesting this method to allow us to more easily test it on its own.

I am with Kevin here, unless there are compelling reasons not to.

I'll go with the majority on this one and de-nest this one, but I personally find inline functions to be great for readability when a chunk of code doesn't deserve its own method but represents a block of functionality which serves a specific purpose. I.e. it can be represented as an input/output contract via a function.

auth/gcloud/aio/auth/token.py

TheKevJames · 2023-09-15T14:15:42Z

Thoughts on building out a thin IAP-secured service on dialpad-oss to allow us to write an integration test for IapToken?

I like the idea in principle, but this repo has a problem with integration tests that we've not yet solved: external contributors can't auth correctly, so tests fail for their PRs. It'd be nice if we could avoid introducing more "failures" like that until we can solve the underlying problem, I think...

The long term solution is probably to solve #578 and use workload identity via CircleCI to authenticate so it isn't tied to org secrets, but that's a much bigger task here. I wonder if mocking out the integration pieces would be a good enough test in the meantime?

That said, I guess e2e tests which only work for us are still better than no e2e tests at all...

caseydialpad

I have added my comments inline.

caseydialpad · 2023-09-15T15:17:52Z

auth/gcloud/aio/auth/session.py

+        timeout: float, params: Optional[Mapping[str, Union[int, str]]],
+        allow_redirects: bool,
+    ) -> Response:
+        pass


Perhaps https://docs.python.org/3/library/exceptions.html#NotImplementedError?

That could definitely work, although I would like to limit the sprawl of my changes and this is an extension of existing form. Since this is an abstract base class and this is an abstract method, you can't instantiate an object of this class so raising is redundant:

>>> import abc >>> class Base(abc.ABC): ... def fn(self): ... return 42 ... @abc.abstractmethod ... def abstract(self): ... pass ... >>> b = Base() Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: Can't instantiate abstract class Base with abstract method abstract

caseydialpad · 2023-09-15T15:20:18Z

auth/gcloud/aio/auth/session.py

+            timeout: Timeout = 10,
+            params: Optional[Mapping[str, Union[int, str]]] = None,
+            allow_redirects: bool = False,
+        ) -> aiohttp.ClientResponse:


This is imported as from aiohttp import ClientResponse as Response. Perhaps change?

Same as the previous comment about wanting to avoid changing unrelated parts of the code.

@TheKevJames might be able to tell you more about this choice.

caseydialpad · 2023-09-15T15:23:03Z

auth/gcloud/aio/auth/token.py

    # pylint: disable=too-many-instance-attributes
+    __metaclass__ = ABCMeta


Do you have any good references on metaclass and working with metaclasses in Python? Interest only.

The only place that has served me well on this has been the official Python docs unfortunately! I'm happy to field questions if you have specific ones :)

https://docs.python.org/3/library/abc.html

caseydialpad · 2023-09-15T15:23:27Z

auth/gcloud/aio/auth/token.py

-    async def _refresh_authorized_user(self, timeout: int) -> Response:
+    @abstractmethod
+    async def refresh(self, *, timeout: int) -> TokenResponse:
+        pass


Same as https://docs.python.org/3/library/exceptions.html#NotImplementedError.

Ditto for my comment above.
#637 (comment)

caseydialpad · 2023-09-15T15:24:27Z

auth/gcloud/aio/auth/token.py

+
+        https://cloud.google.com/iap/docs/authentication-howto#obtaining_an_oidc_token_in_all_other_cases
+        """
+        async def get_access_token(timeout: int) -> str:


I am with Kevin here, unless there are compelling reasons not to.

auth/gcloud/aio/auth/token.py

shaundialpad

Looks pretty good to me! More tests would be phenomenal but I saw Kevin's comment re: integration tests and I see we don't have much at the moment. As long as this had been tested again for the various auth modes for both Token and IapToken then I'm happy with this.

TheKevJames

doc updates LGTM, but you'll need to md->rst them and move the README updates into the init.py file as well once you rebase. Had to switch that as part of the pdoc->sphinx changeover.

auth/gcloud/aio/auth/token.py

jonathan-johnston changed the title ~~checkpoint~~ feat(auth): implement iap auth token Sep 14, 2023

jonathan-johnston requested a review from juanamari94 September 14, 2023 14:26

jonathan-johnston force-pushed the jonjon/auth-iap branch 4 times, most recently from b6b8a8e to e44bc43 Compare September 14, 2023 18:40

feat(auth): implement iap auth token

9ef32ac

jonathan-johnston force-pushed the jonjon/auth-iap branch from e44bc43 to 9ef32ac Compare September 14, 2023 19:39

jonathan-johnston added 4 commits September 14, 2023 15:43

chore(auth): release alpha version for testing

3786764

fix(auth): minor tweaks from manual testing

bcabef3

chore(auth): bump minor version

2a77ecf

refactor(auth): use BaseToken in unit test

850a90c

jonathan-johnston marked this pull request as ready for review September 15, 2023 13:48

jonathan-johnston requested review from TheKevJames and a team as code owners September 15, 2023 13:48

jonathan-johnston requested review from aherrada-dialpad and removed request for a team September 15, 2023 13:48

TheKevJames reviewed Sep 15, 2023

View reviewed changes

caseydialpad reviewed Sep 15, 2023

View reviewed changes

jonathan-johnston added 3 commits September 15, 2023 14:17

docs(auth): include IapToken documentation

893030c

refactor(auth): use class-level default token ttl

3e5a225

refactor(session): remove allow_redirects on get method

8cfc9a9

shaundialpad reviewed Sep 18, 2023

View reviewed changes

auth/gcloud/aio/auth/token.py Outdated Show resolved Hide resolved

shaundialpad approved these changes Sep 18, 2023

View reviewed changes

TheKevJames reviewed Sep 19, 2023

View reviewed changes

auth/gcloud/aio/auth/token.py Outdated Show resolved Hide resolved

jonathan-johnston added 2 commits September 21, 2023 13:36

refactor(auth): clarify impersonating sa arg name

fc5d958

Merge branch 'master' into jonjon/auth-iap

e7aa915

jonathan-johnston requested a review from TheKevJames September 21, 2023 17:57

jonathan-johnston requested a review from caseydialpad September 21, 2023 17:57

caseydialpad approved these changes Sep 21, 2023

View reviewed changes

jonathan-johnston merged commit 70f2e9d into master Sep 22, 2023

jonathan-johnston deleted the jonjon/auth-iap branch September 22, 2023 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): implement iap auth token #637

feat(auth): implement iap auth token #637

jonathan-johnston commented Sep 14, 2023 •

edited

Loading

jonathan-johnston commented Sep 14, 2023

TheKevJames left a comment

TheKevJames Sep 15, 2023

jonathan-johnston Sep 15, 2023

TheKevJames Sep 15, 2023

caseydialpad Sep 15, 2023

jonathan-johnston Sep 15, 2023

TheKevJames commented Sep 15, 2023

caseydialpad left a comment

caseydialpad Sep 15, 2023

jonathan-johnston Sep 21, 2023

caseydialpad Sep 15, 2023

jonathan-johnston Sep 21, 2023

caseydialpad Sep 15, 2023

jonathan-johnston Sep 21, 2023

caseydialpad Sep 15, 2023

jonathan-johnston Sep 21, 2023

caseydialpad Sep 15, 2023

shaundialpad left a comment

TheKevJames left a comment

		# pylint: disable=too-many-instance-attributes
		__metaclass__ = ABCMeta

feat(auth): implement iap auth token #637

feat(auth): implement iap auth token #637

Conversation

jonathan-johnston commented Sep 14, 2023 • edited Loading

Summary

jonathan-johnston commented Sep 14, 2023

TheKevJames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheKevJames commented Sep 15, 2023

caseydialpad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaundialpad left a comment

Choose a reason for hiding this comment

TheKevJames left a comment

Choose a reason for hiding this comment

jonathan-johnston commented Sep 14, 2023 •

edited

Loading