GAM should always send the full OAuth Client ID

GAM-team / GAM

command line management for Google Workspace

https://github.com/GAM-team/GAM/wiki

Apache License 2.0

3.44k stars 466 forks source link

GAM should always send the full OAuth Client ID #1686

Open jay0lee opened 2 months ago

jay0lee commented 2 months ago

Historically, GAM has truncated the OAuth 2.0 client id by removing .apps.googleusercontent.com when storing it to client_secrets.json and oauth2.txt because:

OAuth authorization URLs end up being very long due to the number of scopes GAM uses
it still works.

However, the recent GAM outage occurred because GAM was truncating the client ID in this way. That's why authoriztion started throwing 403 and 500 errors for GAM but other OAuth apps did not see issues.

At this point, I don't think we have OAuth authorization issues with long URLs to the point that .apps.googleusercontent.com is significant and we should avoid doing non-standard / undocumented things like client ID truncation wherever possible to avoid breaking things like this in the future.

Lets:

Configure GAM to send the full client ID to Google every time by default.
Start storing the full client ID in oauth2.txt and client_secrets.json always.
Add a config option to use truncated client IDs. This is a "just in case" we need to revert to current behavior without admins needing to upgrade.

@taers232c fyi

taers232c commented 2 months ago

Jay,

client_secrets.json currently holds the full client_id.

Currently, the client_id is truncated when it's read from client_secrets.json and written to oauth2.txt. Should the config option control that truncation or should the full value always be written to oauth2.txt and the config option controls truncation when the value from oauth2.txt is read on each command?

Ross

Ross Scroggs @.***

On Apr 22, 2024, at 1:19 PM, Jay Lee @.***> wrote:

Historically, GAM has truncated the OAuth 2.0 client id by removing .apps.googleusercontent.com when storing it to client_secrets.json and oauth2.txt because:

OAuth authorization URLs end up being very long due to the number of scopes GAM uses it still works. However, the recent GAM outage occurred because GAM was truncating the client ID in this way. That's why authoriztion started throwing 403 and 500 errors for GAM but other OAuth apps did not see issues.

At this point, I don't think we have OAuth authorization issues with long URLs to the point that .apps.googleusercontent.com is significant and we should avoid doing non-standard / undocumented things like client ID truncation wherever possible to avoid breaking things like this in the future.

Lets:

Configure GAM to send the full client ID to Google every time by default. Start storing the full client ID in oauth2.txt and client_secrets.json always. Add a config option to use truncated client IDs. This is a "just in case" we need to revert to current behavior without admins needing to upgrade. @taers232c https://github.com/taers232c fyi

— Reply to this email directly, view it on GitHub https://github.com/GAM-team/GAM/issues/1686, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCTYL377PBXRWNI7SCELVTY6VWETAVCNFSM6AAAAABGTOKWNOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGI2TOMZZGE3TEOA. You are receiving this because you were mentioned.

jay0lee commented 2 months ago

I'd say we should always store the full value. If anyone sees the need to re-enable truncation that's easy enough to do on read.

On Mon, Apr 22, 2024, 5:27 PM Ross Scroggs @.***> wrote:

Jay,

client_secrets.json currently holds the full client_id.

Currently, the client_id is truncated when it's read from client_secrets.json and written to oauth2.txt. Should the config option control that truncation or should the full value always be written to oauth2.txt and the config option controls truncation when the value from oauth2.txt is read on each command?

Ross

Ross Scroggs @.***

On Apr 22, 2024, at 1:19 PM, Jay Lee @.***> wrote:

Historically, GAM has truncated the OAuth 2.0 client id by removing . apps.googleusercontent.com when storing it to client_secrets.json and oauth2.txt because:

OAuth authorization URLs end up being very long due to the number of scopes GAM uses it still works. However, the recent GAM outage occurred because GAM was truncating the client ID in this way. That's why authoriztion started throwing 403 and 500 errors for GAM but other OAuth apps did not see issues.

At this point, I don't think we have OAuth authorization issues with long URLs to the point that .apps.googleusercontent.com is significant and we should avoid doing non-standard / undocumented things like client ID truncation wherever possible to avoid breaking things like this in the future.

Lets:

Configure GAM to send the full client ID to Google every time by default. Start storing the full client ID in oauth2.txt and client_secrets.json always. Add a config option to use truncated client IDs. This is a "just in case" we need to revert to current behavior without admins needing to upgrade. @taers232c https://github.com/taers232c fyi

— Reply to this email directly, view it on GitHub < https://github.com/GAM-team/GAM/issues/1686>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/ACCTYL377PBXRWNI7SCELVTY6VWETAVCNFSM6AAAAABGTOKWNOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGI2TOMZZGE3TEOA>.

You are receiving this because you were mentioned.

— Reply to this email directly, view it on GitHub https://github.com/GAM-team/GAM/issues/1686#issuecomment-2070980964, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDIZMEFHZYCPC47GE74GZDY6V6DLAVCNFSM6AAAAABGTOKWNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZQHE4DAOJWGQ . You are receiving this because you authored the thread.Message ID: @.***>

taers232c commented 2 months ago

With the existing code (a truncated client_id in oauth2.txt) the decoded_id_token shows the full client_id in "aud" and "azp". Maybe we're already sending the full client_id.?

Ross Scroggs @.***

On Apr 22, 2024, at 1:19 PM, Jay Lee @.***> wrote:

Historically, GAM has truncated the OAuth 2.0 client id by removing .apps.googleusercontent.com when storing it to client_secrets.json and oauth2.txt because:

OAuth authorization URLs end up being very long due to the number of scopes GAM uses it still works. However, the recent GAM outage occurred because GAM was truncating the client ID in this way. That's why authoriztion started throwing 403 and 500 errors for GAM but other OAuth apps did not see issues.

At this point, I don't think we have OAuth authorization issues with long URLs to the point that .apps.googleusercontent.com is significant and we should avoid doing non-standard / undocumented things like client ID truncation wherever possible to avoid breaking things like this in the future.

Lets:

Configure GAM to send the full client ID to Google every time by default. Start storing the full client ID in oauth2.txt and client_secrets.json always. Add a config option to use truncated client IDs. This is a "just in case" we need to revert to current behavior without admins needing to upgrade. @taers232c https://github.com/taers232c fyi

— Reply to this email directly, view it on GitHub https://github.com/GAM-team/GAM/issues/1686, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCTYL377PBXRWNI7SCELVTY6VWETAVCNFSM6AAAAABGTOKWNOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGI2TOMZZGE3TEOA. You are receiving this because you were mentioned.

jay0lee commented 2 months ago

No, we definitely truncate it and use the truncated version in the auth URL and on refresh.

On Mon, Apr 22, 2024, 5:43 PM Ross Scroggs @.***> wrote:

With the existing code (a truncated client_id in oauth2.txt) the decoded_id_token shows the full client_id in "aud" and "azp". Maybe we're already sending the full client_id.?

Ross Scroggs @.***

On Apr 22, 2024, at 1:19 PM, Jay Lee @.***> wrote:

Historically, GAM has truncated the OAuth 2.0 client id by removing . apps.googleusercontent.com when storing it to client_secrets.json and oauth2.txt because:

OAuth authorization URLs end up being very long due to the number of scopes GAM uses it still works. However, the recent GAM outage occurred because GAM was truncating the client ID in this way. That's why authoriztion started throwing 403 and 500 errors for GAM but other OAuth apps did not see issues.

At this point, I don't think we have OAuth authorization issues with long URLs to the point that .apps.googleusercontent.com is significant and we should avoid doing non-standard / undocumented things like client ID truncation wherever possible to avoid breaking things like this in the future.

Lets:

Configure GAM to send the full client ID to Google every time by default. Start storing the full client ID in oauth2.txt and client_secrets.json always. Add a config option to use truncated client IDs. This is a "just in case" we need to revert to current behavior without admins needing to upgrade. @taers232c https://github.com/taers232c fyi

— Reply to this email directly, view it on GitHub < https://github.com/GAM-team/GAM/issues/1686>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/ACCTYL377PBXRWNI7SCELVTY6VWETAVCNFSM6AAAAABGTOKWNOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGI2TOMZZGE3TEOA>.

You are receiving this because you were mentioned.

— Reply to this email directly, view it on GitHub https://github.com/GAM-team/GAM/issues/1686#issuecomment-2071002288, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDIZME5B5B6WWPB3YAGWFDY6V767AVCNFSM6AAAAABGTOKWNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZRGAYDEMRYHA . You are receiving this because you authored the thread.Message ID: @.***>

taers232c commented 2 months ago

If I try to truncate on read I get: AttributeError: property 'client_id' of 'Credentials' object has no setter

Ross Scroggs @.***

On Apr 22, 2024, at 2:57 PM, Jay Lee @.***> wrote:

No, we definitely truncate it and use the truncated version in the auth URL and on refresh.

On Mon, Apr 22, 2024, 5:43 PM Ross Scroggs @.***> wrote:

With the existing code (a truncated client_id in oauth2.txt) the decoded_id_token shows the full client_id in "aud" and "azp". Maybe we're already sending the full client_id.?

Ross Scroggs @.***

On Apr 22, 2024, at 1:19 PM, Jay Lee @.***> wrote:

Historically, GAM has truncated the OAuth 2.0 client id by removing . apps.googleusercontent.com when storing it to client_secrets.json and oauth2.txt because:

OAuth authorization URLs end up being very long due to the number of scopes GAM uses it still works. However, the recent GAM outage occurred because GAM was truncating the client ID in this way. That's why authoriztion started throwing 403 and 500 errors for GAM but other OAuth apps did not see issues.

At this point, I don't think we have OAuth authorization issues with long URLs to the point that .apps.googleusercontent.com is significant and we should avoid doing non-standard / undocumented things like client ID truncation wherever possible to avoid breaking things like this in the future.

Lets:

Configure GAM to send the full client ID to Google every time by default. Start storing the full client ID in oauth2.txt and client_secrets.json always. Add a config option to use truncated client IDs. This is a "just in case" we need to revert to current behavior without admins needing to upgrade. @taers232c https://github.com/taers232c fyi

— Reply to this email directly, view it on GitHub < https://github.com/GAM-team/GAM/issues/1686>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/ACCTYL377PBXRWNI7SCELVTY6VWETAVCNFSM6AAAAABGTOKWNOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGI2TOMZZGE3TEOA>.

You are receiving this because you were mentioned.

— Reply to this email directly, view it on GitHub https://github.com/GAM-team/GAM/issues/1686#issuecomment-2071002288, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDIZME5B5B6WWPB3YAGWFDY6V767AVCNFSM6AAAAABGTOKWNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZRGAYDEMRYHA . You are receiving this because you authored the thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/GAM-team/GAM/issues/1686#issuecomment-2071020141, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCTYLZOQUCU6QQVORQDKA3Y6WBVNAVCNFSM6AAAAABGTOKWNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZRGAZDAMJUGE. You are receiving this because you were mentioned.

taers232c commented 2 months ago

Jay,

I figured out what to change and uploaded a PR

Rebuild your oauth2.txt; the full client_id should be present gam oauth create

Truncate gam config truncate_client_id true info customer

No truncate, this is the default gam config truncate_client_id false info customer

Ross

Ross Scroggs @.***

On Apr 22, 2024, at 2:57 PM, Jay Lee @.***> wrote:

No, we definitely truncate it and use the truncated version in the auth URL and on refresh.

On Mon, Apr 22, 2024, 5:43 PM Ross Scroggs @.***> wrote:

With the existing code (a truncated client_id in oauth2.txt) the decoded_id_token shows the full client_id in "aud" and "azp". Maybe we're already sending the full client_id.?

Ross Scroggs @.***

On Apr 22, 2024, at 1:19 PM, Jay Lee @.***> wrote:

Historically, GAM has truncated the OAuth 2.0 client id by removing . apps.googleusercontent.com when storing it to client_secrets.json and oauth2.txt because:

OAuth authorization URLs end up being very long due to the number of scopes GAM uses it still works. However, the recent GAM outage occurred because GAM was truncating the client ID in this way. That's why authoriztion started throwing 403 and 500 errors for GAM but other OAuth apps did not see issues.

At this point, I don't think we have OAuth authorization issues with long URLs to the point that .apps.googleusercontent.com is significant and we should avoid doing non-standard / undocumented things like client ID truncation wherever possible to avoid breaking things like this in the future.

Lets:

Configure GAM to send the full client ID to Google every time by default. Start storing the full client ID in oauth2.txt and client_secrets.json always. Add a config option to use truncated client IDs. This is a "just in case" we need to revert to current behavior without admins needing to upgrade. @taers232c https://github.com/taers232c fyi

— Reply to this email directly, view it on GitHub < https://github.com/GAM-team/GAM/issues/1686>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/ACCTYL377PBXRWNI7SCELVTY6VWETAVCNFSM6AAAAABGTOKWNOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGI2TOMZZGE3TEOA>.

You are receiving this because you were mentioned.

— Reply to this email directly, view it on GitHub https://github.com/GAM-team/GAM/issues/1686#issuecomment-2071002288, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDIZME5B5B6WWPB3YAGWFDY6V767AVCNFSM6AAAAABGTOKWNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZRGAYDEMRYHA . You are receiving this because you authored the thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/GAM-team/GAM/issues/1686#issuecomment-2071020141, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCTYLZOQUCU6QQVORQDKA3Y6WBVNAVCNFSM6AAAAABGTOKWNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZRGAZDAMJUGE. You are receiving this because you were mentioned.

jay0lee commented 2 months ago

FYI, a PR (pull request) goes through a review before being committed to the source, you did a direct commit here:

https://github.com/GAM-team/GAM/commit/b4b9bd243600ab40d209d71cbd64da86163321e6

for future reference, PRs and noting the issue # in the PR make it easier to follow the issue, PR and fix status.

jay0lee commented 2 months ago

actually the real meat of the change was this commit:

https://github.com/GAM-team/GAM/commit/b384bdb50309fe038bdd5463c5d237e8a2099099