GAM icon indicating copy to clipboard operation
GAM copied to clipboard

GAM should always send the full OAuth Client ID

Open jay0lee opened this issue 1 year ago • 8 comments

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:

  1. Configure GAM to send the full client ID to Google every time by default.
  2. Start storing the full client ID in oauth2.txt and client_secrets.json always.
  3. 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

jay0lee avatar Apr 22 '24 20:04 jay0lee

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.

taers232c avatar Apr 22 '24 21:04 taers232c

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: @.***>

jay0lee avatar Apr 22 '24 21:04 jay0lee

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.

taers232c avatar Apr 22 '24 21:04 taers232c

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: @.***>

jay0lee avatar Apr 22 '24 21:04 jay0lee

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 avatar Apr 22 '24 23:04 taers232c

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.

taers232c avatar Apr 23 '24 00:04 taers232c

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 avatar Apr 23 '24 16:04 jay0lee

actually the real meat of the change was this commit:

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

jay0lee avatar Apr 23 '24 16:04 jay0lee