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

authorize: add support for logging id token #4392

Merged
merged 1 commit into from Jul 25, 2023
Merged

Conversation

calebdoxsey
Copy link
Contributor

Summary

Add support for logging the id token. When this field is included in the logs the raw id token will be logged to id-token and the decoded claims to id-token-claims.

Related issues

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@calebdoxsey calebdoxsey requested a review from a team as a code owner July 25, 2023 20:13
@calebdoxsey calebdoxsey requested a review from wasaga July 25, 2023 20:13
@coveralls
Copy link

Coverage Status

coverage: 63.644% (+0.04%) from 63.6% when pulling 2b9dc24 on cdoxsey/log-tokens into baf8918 on main.

@@ -16,6 +16,7 @@ const (
AuthorizeLogFieldEmail AuthorizeLogField = "email"
AuthorizeLogFieldHeaders = AuthorizeLogField(headersFieldName)
AuthorizeLogFieldHost AuthorizeLogField = "host"
AuthorizeLogFieldIDToken AuthorizeLogField = "id-token"
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional)

I wonder if it would make sense to allow the user to choose between just the raw ID token or just the decoded claims. Should we add an explicit AuthorizeLogFieldIDTokenClaims as well here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The requirements stated that both should be logged

We should pass the user's most recent encoded AND base64 decoded id_token as part of the request.

Though maybe I misunderstood what was being asked for.

if t, err := jwt.ParseSigned(s.GetIdToken().GetRaw()); err == nil {
var m map[string]any
_ = t.UnsafeClaimsWithoutVerification(&m)
evt = evt.Interface("id-token-claims", m)
Copy link
Contributor

Choose a reason for hiding this comment

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

(no changes required)

Do we already verify the ID token signature somewhere else, and that's why it's fine to fine to call UnsafeClaimsWithoutVerification() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know about the security ramifications of this. Verifying the signature of the ID token against the configured identity provider would be difficult to implement here (and also likely very slow)

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there's some verification related to ID tokens here:

return v.Verify(ctx, rawIDToken)

Do you know if all ID tokens come from that getIDToken() method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ID token comes from the identity provider and is saved to the session.

@calebdoxsey calebdoxsey merged commit 1aa8187 into main Jul 25, 2023
10 checks passed
@calebdoxsey calebdoxsey deleted the cdoxsey/log-tokens branch July 25, 2023 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants