Skip to content
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.

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

feat: Refresh token expiration window #2827

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
751dbb5
add refresh_token grace period
bill-robbins-ss Oct 27, 2021
219a58e
add migration for used on refresh token
bill-robbins-ss Oct 29, 2021
efa3b8e
add refresh token grace period
bill-robbins-ss Oct 29, 2021
7a52b36
replace migration with ory dev CLI cmd
bill-robbins-ss Nov 1, 2021
45b9aea
use existing GetRefreshtTokenSession
bill-robbins-ss Nov 1, 2021
7d572aa
make FositeStorer include oauth2.TokenRevocationStorage
bill-robbins-ss Nov 1, 2021
9573041
WIP mabye grace period tests
bill-robbins-ss Nov 1, 2021
a203f4b
use test run instead of named functions
bill-robbins-ss Nov 1, 2021
1ebc99d
add documentation for example config
bill-robbins-ss Nov 1, 2021
3bb1c20
add grace period to internal config
bill-robbins-ss Nov 1, 2021
8de23dc
add refresh token grace period to token-expiration doc
bill-robbins-ss Nov 1, 2021
249264f
prettier --write
bill-robbins-ss Nov 2, 2021
821fba4
Update persistence/sql/persister_oauth2.go
bill-robbins-ss Nov 22, 2021
2e14277
update docs: consequences of reusing a used refresh token
bill-robbins-ss Nov 22, 2021
07375a9
add parent key for refresh_token_rotation
bill-robbins-ss Nov 22, 2021
09d6801
move refresh token rotation to proper parent
bill-robbins-ss Nov 22, 2021
89527fd
update documentation
bill-robbins-ss Nov 22, 2021
69ca392
remove unneeded file
bill-robbins-ss Nov 22, 2021
8fa6c8c
make encryption of session more obvious
bill-robbins-ss Nov 22, 2021
bd8a15e
rename used to in_grace_period
bill-robbins-ss Nov 22, 2021
13cdea8
when deactivating a refresh token, in_grace_period should be false
bill-robbins-ss Nov 22, 2021
1f642d3
add testing the refresh token store when grace period is configured
bill-robbins-ss Dec 3, 2021
f244b61
npx prettier --write {test,cypress}/**/*.js
bill-robbins-ss Jan 4, 2022
73556c7
Merge remote-tracking branch 'origin/master' into refresh-token-expir…
aeneasr Jan 11, 2022
a487706
cchore: format
aeneasr Jan 11, 2022
e992907
chore: update fosite
aeneasr Jan 11, 2022
d1db135
fix linting errors
bill-robbins-ss Jan 20, 2022
45a8cff
Merge remote-tracking branch 'origin/master' into refresh-token-expir…
aeneasr Feb 14, 2022
2c7b95f
fix: add max lifetime
aeneasr Feb 14, 2022
b1d37ca
fix: move migration to latest
aeneasr Feb 14, 2022
42de645
remove reflection
bill-robbins-ss Feb 16, 2022
bd2d446
Merge remote-tracking branch 'upstream/master' into refresh-token-exp…
bill-robbins-ss Feb 24, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions driver/config/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ const (
KeyOAuth2LegacyErrors = "oauth2.include_legacy_error_fields"
KeyExcludeNotBeforeClaim = "oauth2.exclude_not_before_claim"
KeyAllowedTopLevelClaims = "oauth2.allowed_top_level_claims"
KeyRefreshTokenRotationGracePeriod = "oauth2.refresh_token_rotation.grace_period"
)

const DSNMemory = "memory"
Expand Down Expand Up @@ -428,3 +429,7 @@ func (p *Provider) CGroupsV1AutoMaxProcsEnabled() bool {
func (p *Provider) GrantAllClientCredentialsScopesPerDefault() bool {
return p.p.Bool(KeyGrantAllClientCredentialsScopesPerDefault)
}

func(p *Provider) RefreshTokenRotationGracePeriod() time.Duration {
return p.p.DurationF(KeyRefreshTokenRotationGracePeriod, 0)
aeneasr marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE hydra_oauth2_refresh DROP COLUMN used;
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE hydra_oauth2_refresh ADD used bool DEFAULT false;
1 change: 1 addition & 0 deletions persistence/sql/models/models.go
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package models
bill-robbins-ss marked this conversation as resolved.
Show resolved Hide resolved
142 changes: 129 additions & 13 deletions persistence/sql/persister_oauth2.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,11 @@ const (
)

func (r OAuth2RequestSQL) TableName() string {
return "hydra_oauth2_" + string(r.Table)
return r.Table.TableName()
}

func (table tableName) TableName() string {
return "hydra_oauth2_" + string(table)
}

func (p *Persister) sqlSchemaFromRequest(rawSignature string, r fosite.Requester, table tableName) (*OAuth2RequestSQL, error) {
Expand All @@ -67,19 +71,11 @@ func (p *Persister) sqlSchemaFromRequest(rawSignature string, r fosite.Requester
subject = r.GetSession().GetSubject()
}

session, err := json.Marshal(r.GetSession())
session, err := p.marshalSession(r.GetSession())
if err != nil {
return nil, errorsx.WithStack(err)
}

if p.config.EncryptSessionData() {
ciphertext, err := p.r.KeyCipher().Encrypt(session)
if err != nil {
return nil, errorsx.WithStack(err)
}
session = []byte(ciphertext)
}

var challenge sql.NullString
rr, ok := r.GetSession().(*oauth2.Session)
if !ok && r.GetSession() != nil {
Expand Down Expand Up @@ -108,6 +104,31 @@ func (p *Persister) sqlSchemaFromRequest(rawSignature string, r fosite.Requester
}, nil
}

func (p *Persister) marshalSession(session fosite.Session) ([]byte, error) {
sessionBytes, err := json.Marshal(session)
if err != nil {
return nil, err
}

if sessionBytes, err = p.maybeEncryptSession(sessionBytes); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make this explicit? A simple := here would disable encryption, a significant security feature. Thanks!

return nil, err
}
return sessionBytes, nil
}

// MaybeEncryptSession encrypt a session if configuration indicates it should
func (p *Persister) maybeEncryptSession(session []byte) ([]byte, error) {
if !p.config.EncryptSessionData() {
return session, nil
}

ciphertext, err := p.r.KeyCipher().Encrypt(session)
if err != nil {
return nil, err
}
return []byte(ciphertext), nil
}

func (r *OAuth2RequestSQL) toRequest(ctx context.Context, session fosite.Session, p *Persister) (*fosite.Request, error) {
sess := r.Session
if !gjson.ValidBytes(sess) {
Expand Down Expand Up @@ -219,6 +240,30 @@ func (p *Persister) createSession(ctx context.Context, signature string, request
return nil
}

func (p *Persister) updateRefreshSession(ctx context.Context, requestId string, session fosite.Session, used bool) error {
_, ok := session.(*oauth2.Session)
if !ok && session != nil {
return errors.Errorf("Expected session to be of type *Session, but got: %T", session)
bill-robbins-ss marked this conversation as resolved.
Show resolved Hide resolved
}
sessionBytes, err := p.marshalSession(session)
if err != nil {
return err
}

updateSql := fmt.Sprintf("UPDATE %s SET session_data = ?, used = ? WHERE request_id = ?",
sqlTableRefresh.TableName())

return p.transaction(ctx, func(ctx context.Context, c *pop.Connection) error {
err := p.Connection(ctx).RawQuery(updateSql, sessionBytes, used, requestId).Exec()
if errors.Is(err, sql.ErrNoRows) {
return errorsx.WithStack(fosite.ErrNotFound)
} else if err != nil {
return sqlcon.HandleError(err)
}
return nil
})
}

func (p *Persister) findSessionBySignature(ctx context.Context, rawSignature string, session fosite.Session, table tableName) (fosite.Requester, error) {
rawSignature = p.hashSignature(rawSignature, table)

Expand Down Expand Up @@ -247,6 +292,31 @@ func (p *Persister) findSessionBySignature(ctx context.Context, rawSignature str
})
}

func (p *Persister) findRequesterBySignature(ctx context.Context, signature string, tableName tableName) (fosite.Requester, error) {
bill-robbins-ss marked this conversation as resolved.
Show resolved Hide resolved
r := OAuth2RequestSQL{Table: tableName}

var fr fosite.Requester
session := new(oauth2.Session)

return fr, p.transaction(ctx, func(ctx context.Context, c *pop.Connection) error {
err := p.Connection(ctx).Where("signature = ?", signature).First(&r)
if errors.Is(err, sql.ErrNoRows) {
return errorsx.WithStack(fosite.ErrNotFound)
} else if err != nil {
return sqlcon.HandleError(err)
} else if !r.Active {
fr, err = r.toRequest(ctx, session, p)
if err != nil {
return err
}
return errorsx.WithStack(fosite.ErrInactiveToken)
}

fr, err = r.toRequest(ctx, session, p)
return err
})
}

func (p *Persister) deleteSessionBySignature(ctx context.Context, signature string, table tableName) error {
signature = p.hashSignature(signature, table)

Expand Down Expand Up @@ -280,13 +350,27 @@ func (p *Persister) deactivateSessionByRequestID(ctx context.Context, id string,
return sqlcon.HandleError(
p.Connection(ctx).
RawQuery(
fmt.Sprintf("UPDATE %s SET active=false WHERE request_id=?", OAuth2RequestSQL{Table: table}.TableName()),
fmt.Sprintf("UPDATE %s SET active=false, used=true WHERE request_id=?", OAuth2RequestSQL{Table: table}.TableName()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually need used when we have active already?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, findSessionBySignature will return errors when a session is marked inactive.

id,
).
Exec(),
)
}

func (p *Persister) getRefreshTokenUsedStatusBySignature(ctx context.Context, signature string) (bool, error) {
bill-robbins-ss marked this conversation as resolved.
Show resolved Hide resolved
var used bool
return used, p.transaction(ctx, func(ctx context.Context, c *pop.Connection) error {
query := fmt.Sprintf("SELECT used FROM %s WHERE signature = ?", sqlTableRefresh.TableName())
err := p.Connection(ctx).RawQuery(query, signature).First(&used)
if errors.Is(err, sql.ErrNoRows) {
return errorsx.WithStack(fosite.ErrNotFound)
} else if err != nil {
return sqlcon.HandleError(err)
}
return err
})
}

func (p *Persister) CreateAuthorizeCodeSession(ctx context.Context, signature string, requester fosite.Requester) (err error) {
return p.createSession(ctx, signature, requester, sqlTableCode)
}
Expand Down Expand Up @@ -353,8 +437,40 @@ func (p *Persister) DeletePKCERequestSession(ctx context.Context, signature stri
return p.deleteSessionBySignature(ctx, signature, sqlTablePKCE)
}

func (p *Persister) RevokeRefreshToken(ctx context.Context, id string) error {
return p.deactivateSessionByRequestID(ctx, id, sqlTableRefresh)
func (p *Persister) RevokeRefreshToken(ctx context.Context, requestId string) error {
return p.deactivateSessionByRequestID(ctx, requestId, sqlTableRefresh)
}

func (p *Persister) RevokeRefreshTokenMaybeGracePeriod(ctx context.Context, requestId string, signature string) error {
bill-robbins-ss marked this conversation as resolved.
Show resolved Hide resolved
gracePeriod := p.config.RefreshTokenRotationGracePeriod()
if gracePeriod <= 0 {
return p.RevokeRefreshToken(ctx, requestId)
}

var requester fosite.Requester
var err error
if requester, err = p.findRequesterBySignature(ctx, signature, sqlTableRefresh); err != nil {
p.l.Errorf("signature: %s not found. grace period not applied", signature)
return errors.WithStack(err)
Comment on lines +423 to +424
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add this to the error context instead? Without a request context, it will be difficult to trace the log in a noisy environment :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean here, is there code elsewhere that does this?

}

var used bool
if used,err = p.getRefreshTokenUsedStatusBySignature(ctx, signature); err != nil {
p.l.Errorf("signature: %s used status not found. grace period not applied", signature)
return errors.WithStack(err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add this to the error context instead? Without a request context, it will be difficult to trace the log in a noisy environment :)

}

if ! used {
session := requester.GetSession()
session.SetExpiresAt(fosite.RefreshToken, time.Now().UTC().Add(gracePeriod))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to understand this a bit better - when refreshing a token we do not deactivate it (active: false) immediately. Instead, if grace is enabled, we extend the expiry time by X from NOW, and we also set used: true. The next time we refresh the token, and we are still in the refresh grace period, nothing is updated.

When we refresh the token again, and NOW + x has passed, fosite will see the key as expired, implying that it is no longer active.

I think this has some serious implications. I am not 100% any more how token reuse detection works, but I think it doesn't trigger on expired tokens, only on inactive ones. Because an expired token has not been reused, it just expired! And since it's expired, you can't use it. A used (active=false) token though has been used, thus, reuse means invalidation of all other tokens.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're saying

  • this will work but the error will be wrong (expired not reused)
  • when reused and expired, since it isn't detected as reused other tokens are not revoked

I mentioned in an earlier comment that setting active=false won't work for grace periods, since calls to findSessionBySignature will not succeed. To properly detect that case (active==true, in_grace_period==true, expired) something could be modified to deal with it, I'm worried that changing findSessionBySignature to throw an expired error may have undesirable repercussions. From what I can tell token validation, which includes expiration checking, is done at a different layer and is part of a Strategy. The persistence layer does not have access to token strategies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was what I was trying to say :)

The best way to prove/disprove this hypothesis is to introduce a test which covers this. If there's no regression on revokation, this should be fine! It would theoretically also be fine to revoke the token chain on expiration also, not just on reuse.

if err = p.updateRefreshSession(ctx, requestId, session, true); err != nil {
p.l.Errorf("failed to update session with signature: %s", signature)
return errors.WithStack(err)
Comment on lines +437 to +438
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

}
} else {
p.l.Debugf("request_id: %s has already been used and is in the grace period", requestId)
bill-robbins-ss marked this conversation as resolved.
Show resolved Hide resolved
}
return nil
}

func (p *Persister) RevokeAccessToken(ctx context.Context, id string) error {
Expand Down
69 changes: 69 additions & 0 deletions scripts/create-migration.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
#!/bin/bash
bill-robbins-ss marked this conversation as resolved.
Show resolved Hide resolved

# create-migration.sh is a helper script to generate sql migration files.
# Migration file names follow the date-time naming convention established by Hydra.

set -Eeo pipefail

if [ -n "${DEBUG}" ]; then
set -x
fi

migration_dir="$( dirname "${BASH_SOURCE[0]}" )/../persistence/sql/migrations/"
supported_dialects=(cockroach mysql postgres sqlite)

if [ ! -d "$migration_dir" ]; then
echo "expected $migration_dir to exist"
exit 1
fi

function usage() {
cat << EOF

Create up and down migration files for SQLite, CockroachDB, PostgreSQL
$0 <migration-name> --all-dialects

--all-dialects create a file for each supported dialect (${supported_dialects[*]})

e.g.
$0 add_column_to_table
$0 add_column_to_table --all-dialects
EOF
}

if [ -z "${1}" ]; then
usage
exit 1
fi

if [[ ! "${1}" =~ ^[A-Za-z_]+$ ]]; then
echo "invalid migration name: ${1} must be only A-Z, a-z, or _"
usage
exit 1
fi

timestamp=$(date -u +%Y%m%d%H%M%S%5N)
echo $timestamp

file_prefix="${timestamp}_${1}"

function create_files() {
dialect=$1
filename="${file_prefix}"

if [ ! -z "${dialect}" ]; then
filename="${filename}.${dialect}."
fi

touch "${migration_dir}${filename}.up.sql"
touch "${migration_dir}${filename}.down.sql"
}

if [ "${2}" == "--all-dialects" ]; then
for dialect in "${supported_dialects[@]}"
do
create_files $dialect
done
else
create_files
fi
16 changes: 15 additions & 1 deletion spec/config.json
Original file line number Diff line number Diff line change
Expand Up @@ -867,9 +867,23 @@
]
}
}
},
"refresh_token_rotation": {
"type": "object",
"properties": {
"grace_period": {
"description": "Configures how long a Refresh Token remains valid after it has been used.",
"default": "0h",
"allOf": [
{
"$ref": "#/definitions/duration"
}
]
}
}
}
},
}
},
"secrets": {
"type": "object",
"additionalProperties": false,
Expand Down