New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: query by hashed signature only on access code table #3593

Merged

alnr merged 3 commits into master from query-raw-sig

Aug 8, 2023

Contributor

alnr commented Aug 3, 2023

No description provided.

alnr self-assigned this

codecov bot commented Aug 3, 2023 •

edited

Loading

Codecov Report

Merging #3593 (2c465f5) into master (1a1f504) will decrease coverage by 0.08%.
The diff coverage is 63.01%.

❗ Current head 2c465f5 differs from pull request most recent head cfd4f0f. Consider uploading reports for the commit cfd4f0f to get more accurate results

@@            Coverage Diff             @@
##           master    #3593      +/-   ##
==========================================
- Coverage   76.32%   76.24%   -0.08%     
==========================================
  Files         132      132              
  Lines        9879     9901      +22     
==========================================
+ Hits         7540     7549       +9     
- Misses       1824     1837      +13     
  Partials      515      515

Files Changed	Coverage Δ
cmd/server/handler.go	`1.33% <0.00%> (-0.04%)`	⬇️
consent/handler.go	`65.07% <ø> (+0.90%)`	⬆️
x/clean_sql.go	`90.90% <0.00%> (-0.21%)`	⬇️
persistence/sql/persister_oauth2.go	`82.09% <60.78%> (-3.44%)`	⬇️
consent/strategy_default.go	`68.97% <100.00%> (+0.47%)`	⬆️

hperl requested changes

View reviewed changes

Contributor

hperl left a comment

You found all the right places, but the hasing only applies to access tokens (and the respective table), not all the other tables (refresh, id, code, pkce, oidc).

persistence/sql/persister_oauth2.go Show resolved Hide resolved

persistence/sql/persister_oauth2.go Show resolved Hide resolved

persistence/sql/persister_oauth2.go Outdated

               	err = sqlcon.HandleError(
               		p.QueryWithNetwork(ctx).
-              			Where("signature IN (?, ?)", signature, SignatureHash(signature)).
+              			Where("signature = ?", SignatureHash(signature)).

Contributor

hperl Aug 3, 2023

Suggested change

      
            			Where("signature = ?", SignatureHash(signature)).
          
            			Where("signature = ?", signature).

persistence/sql/persister_oauth2.go Outdated

    
              				fmt.Sprintf("UPDATE %s SET active=false WHERE signature=? AND nid = ?", OAuth2RequestSQL{Table: sqlTableCode}.TableName()),

              				signature,

              				fmt.Sprintf("UPDATE %s SET active = false WHERE signature = ? AND nid = ?", OAuth2RequestSQL{Table: sqlTableCode}.TableName()),

              				SignatureHash(signature),

Contributor

hperl Aug 3, 2023

No need to hash because authorization code signatures are not hashed.

persistence/sql/persister_nid_test.go Outdated

               			require.NoError(t, r.Persister().InvalidateAuthorizeCodeSession(s.t2, sig))
               			actual := persistencesql.OAuth2RequestSQL{Table: "code"}
-              			require.NoError(t, r.Persister().Connection(context.Background()).Find(&actual, sig))
+              			require.NoError(t, r.Persister().Connection(context.Background()).Find(&actual, persistencesql.SignatureHash(sig)))

Contributor

hperl Aug 3, 2023

Change not needed.

persistence/sql/persister_nid_test.go Outdated

@@ @@ -1748,10 +1750,10 @@ func (s *PersisterTestSuite) TestRevokeRefreshToken() { @@
               			actual := persistencesql.OAuth2RequestSQL{Table: "refresh"}
               			require.NoError(t, r.Persister().RevokeRefreshToken(s.t2, request.ID))
-              			require.NoError(t, r.Persister().Connection(context.Background()).Find(&actual, signature))
+              			require.NoError(t, r.Persister().Connection(context.Background()).Find(&actual, persistencesql.SignatureHash(signature)))

Contributor

hperl Aug 3, 2023

Change not needed.

persistence/sql/persister_nid_test.go Outdated

               			require.Equal(t, true, actual.Active)
               			require.NoError(t, r.Persister().RevokeRefreshToken(s.t1, request.ID))
-              			require.NoError(t, r.Persister().Connection(context.Background()).Find(&actual, signature))
+              			require.NoError(t, r.Persister().Connection(context.Background()).Find(&actual, persistencesql.SignatureHash(signature)))

Contributor

hperl Aug 3, 2023

Change not needed.

persistence/sql/persister_nid_test.go Outdated

               			}
               			require.NoError(t, store.RevokeRefreshTokenMaybeGracePeriod(s.t2, request.ID, signature))
-              			require.NoError(t, r.Persister().Connection(context.Background()).Find(&actual, signature))
+              			require.NoError(t, r.Persister().Connection(context.Background()).Find(&actual, persistencesql.SignatureHash(signature)))

Contributor

hperl Aug 3, 2023

Change not needed.

persistence/sql/persister_nid_test.go Outdated

               			require.Equal(t, true, actual.Active)
               			require.NoError(t, store.RevokeRefreshTokenMaybeGracePeriod(s.t1, request.ID, signature))
-              			require.NoError(t, r.Persister().Connection(context.Background()).Find(&actual, signature))
+              			require.NoError(t, r.Persister().Connection(context.Background()).Find(&actual, persistencesql.SignatureHash(signature)))

Contributor

hperl Aug 3, 2023

Change not needed.

persistence/sql/persister_nid_test.go Show resolved Hide resolved

Member

aeneasr commented Aug 3, 2023

Doesn't this break BC @hperl ?

alnr force-pushed the query-raw-sig branch from 09da66e to 8b0fae6 Compare

August 3, 2023 15:34

alnr changed the title ~~fix: don't query by raw signature~~ fix: query by hashed signature only on access code table

alnr marked this pull request as ready for review

August 4, 2023 08:34

alnr requested a review from aeneasr as a code owner

August 4, 2023 08:34

alnr force-pushed the query-raw-sig branch from 8b0fae6 to 654224a Compare

August 4, 2023 08:39

alnr commented

View reviewed changes

consent/handler.go Show resolved Hide resolved

Contributor

hperl commented Aug 4, 2023

Doesn't this break BC @hperl ?

This should not not break backwards compatibility. We introduced undconditionally hashing all new access token signatures (before, we hashed only JWT access tokens). We kept the lookup for the raw signature in place so that old access tokens could still be found.

Since some time has passed, all current access tokens should have a hashed signature already, so removing the additional lookup for the raw signature can be removed safely.

alnr force-pushed the query-raw-sig branch from 654224a to 6aa52a0 Compare

August 4, 2023 09:17

hperl previously approved these changes

View reviewed changes

Contributor

hperl left a comment

LGTM!

alnr dismissed hperl’s stale review via

b42f389

August 4, 2023 09:34

alnr force-pushed the query-raw-sig branch from 6aa52a0 to b42f389 Compare

August 4, 2023 09:34

Member

aeneasr commented Aug 4, 2023 •

edited

Loading

Since some time has passed, all current access tokens should have a hashed signature already, so removing the additional lookup for the raw signature can be removed safely.

So it does have BC implications and those simply don't apply ONLY if you no longer have active tokens in the store. Scenarios where this isn't the case:

Someone upgrading from hydra pre-hash to this version without running the interim version for a while. We have at least one customer I know that could run into this.
Someone running (unreasonably high) TTLs for their access tokens

Have we verified that there is indeed no active tokens in Ory Network any more that would be affected by this?

alnr force-pushed the query-raw-sig branch from d1b28dc to 3d10f57 Compare

August 8, 2023 07:53

hperl previously approved these changes

View reviewed changes

Contributor

hperl left a comment

LGTM!

alnr added 2 commits

August 8, 2023 10:24


          feat: bump fosite and add some more tracing

d527fe8


          fix: only query access tokens by hashed signature

886af56

alnr dismissed hperl’s stale review via

886af56

August 8, 2023 08:25

alnr force-pushed the query-raw-sig branch from 3d10f57 to 886af56 Compare

August 8, 2023 08:25

aeneasr previously approved these changes

View reviewed changes


          fix: deflake ttl test

cfd4f0f

alnr dismissed aeneasr’s stale review via

cfd4f0f

August 8, 2023 09:14

alnr requested review from aeneasr and hperl

August 8, 2023 09:23

hperl approved these changes

View reviewed changes

aeneasr approved these changes

View reviewed changes

alnr merged commit 6741a49 into master

alnr deleted the query-raw-sig branch

August 8, 2023 14:09

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet