feat: track oidc token #1518

david972 · 2021-07-10T01:26:10Z

Related issue

Proposed changes

NEW FEATURE: This patch add the possibility to retrieve the access and refresh token
It introduces a crypt/decrypt package
And a handle to get the tokens

Checklist

For the moment it's more a draft and test will be done later

I have read the contributing guidelines.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further comments

aeneasr

Awesome, thank you for working on this! I have a couple of comments which we need to improve prior to merge :)

Makefile

.gitignore

crypt/crypt_aes.go

driver/config/.schema/config.schema.json

driver/config/config.go

identity/handler.go

This feature introduce a crypt/decrypt package use for access/refresh token for now Signed-off-by: David ALEXANDRE <david.alexandre@w6d.io>

Signed-off-by: David ALEXANDRE <david.alexandre@w6d.io>

remove useless handler improve general identity handler

This feature introduce a crypt/decrypt package use for access/refresh token for now Signed-off-by: David ALEXANDRE <david.alexandre@w6d.io>

Signed-off-by: David ALEXANDRE <david.alexandre@w6d.io>

remove useless handler improve general identity handler

…ck-oidc-tokens

…opment

david972 · 2021-07-14T16:38:42Z

currently I put all code the general identity get but I planned to move it to elsewhere if the way is validated

I’ll update the tests later
For the documentation, I will need to know whether it is sufficient and where it will be better suited

aeneasr · 2021-07-23T14:51:42Z

Thank you for the changes! I will be able to review them next week! :)

aeneasr · 2021-08-03T10:52:21Z

I tried pushing some changes required for merging the PR to your fork & branch, but it appears that I am not allowed to do so 😕

% git push ...
ERROR: Permission to push denied to aeneasr.
fatal: could not read from the remote repository.

Please make sure that you have the correct access rights
and the repository exists.

But the good news is, giving access is easy! ☺️ All you need to do is enable write access for maintainers. Thank you! 😄

If the repository belongs to an organization, please add me for the project as a collaborator!

david972 · 2021-08-03T10:58:58Z

just did it

Code Review ory#1518

# Conflicts: # contrib/quickstart/kratos/email-password/kratos.yml # go.mod # go.sum

david972 · 2021-08-04T12:59:08Z

ok thank for the review

david972 · 2021-08-04T13:00:55Z

I assume that I can add some unit test now

aeneasr · 2021-08-04T13:13:41Z

I still have a few items, wait a few more minutes .)

aeneasr

Thank you! Here are some ideas. I also pushed a few changes to the naming conventions. Of course, we still need lots of tests :)

I am on holiday on friday-sunday so I can either review it tomorrow again, or next week. This is now on my priority list to get merged :)

identity/handler.go

cipher/aes.go

driver/config/.schema/config.schema.json

driver/config/config.go

identity/handler.go

identity/identity.go

aeneasr · 2021-08-04T13:12:55Z

selfservice/strategy/oidc/types.go

+	EncryptedAccessToken  []byte `json:"encrypted_access_token"`
+	EncryptedRefreshToken []byte `json:"encrypted_refresh_token"`


Make these a string as []byte is base64 encoded by encoding/json

I did but I don't know why maybe a mistake on my side but wrap and unwrap the encrypted data goes wrong.

Yes just retried this and it did not work due to non-printable character

aeneasr · 2021-08-04T13:26:04Z

Looks like my comment partially didn't make it - the tests for the schema can be added here: https://github.com/ory/kratos/tree/master/test/schema

david972 · 2021-08-04T13:31:16Z

ok I on the change try to do it before your holiday

aeneasr · 2021-08-04T13:33:52Z

awesome! :)

david972 · 2021-08-11T09:42:00Z

Sorry I've got some personal issues
I'll try to do it for monday

aeneasr

Hello @david972 - this is some great stuff here! Good job! Sorry it took so long to review - cryptography just takes a lot of time to look into.

I have made a couple of adjustments, but nothing too critical. One important thing would be to move the github-app feature into a spearate PR.

Another thing is regarding the required secret. I was thinking about the impact on people getting started with Ory Kratos. I remember when we introduced this feature to Ory Hydra, it was very confusing to people at first. They often lost the keys, mixed them up, and in general had issues with managing encryption keys.

So I wonder if it would make sense to have one cypher which is a noop, and use that per default? That noop cipher would not need any secret and would be the default.

If you want tighter security, you would then use e.g. xchacha-poly1305.

What do you think about that?

selfservice/strategy/oidc/provider_github_app.go

Makefile

cipher/aes.go

cipher/cipher_test.go

driver/config/.schema/config.schema.json

driver/config/config.go

david972 · 2021-08-30T16:32:58Z

Hello @david972 - this is some great stuff here! Good job! Sorry it took so long to review - cryptography just takes a lot of time to look into.

Thanks

I have made a couple of adjustments, but nothing too critical. One important thing would be to move the github-app feature into a spearate PR.

Another thing is regarding the required secret. I was thinking about the impact on people getting started with Ory Kratos. I remember when we introduced this feature to Ory Hydra, it was very confusing to people at first. They often lost the keys, mixed them up, and in general had issues with managing encryption keys.

So I wonder if it would make sense to have one cypher which is a noop, and use that per default? That noop cipher would not need any secret and would be the default.

If you want tighter security, you would then use e.g. xchacha-poly1305.

What do you think about that?

Yes that sound good too me to do. And actually it let people manage whether they want or not encrypting theirs data

If you give me you go I can do it very quickly

aeneasr · 2021-08-30T16:35:29Z

You have my go! :)

david972 · 2021-08-30T16:35:56Z

An other point is regarding the schema validation that require the secret how can we set it as mandatory if people choose a cypher ?

aeneasr · 2021-08-30T16:57:33Z

It is possible with if/then - we have one example here: https://github.com/ory/kratos/blob/master/driver/config/.schema/config.schema.json#L1858

aeneasr · 2021-08-31T08:19:25Z

Ah and please don't forget to add the new docs page to the sidebar :)

https://github.com/ory/kratos/blob/master/docs/sidebar.json

david972 · 2021-08-31T08:41:59Z

you right I had forgotten

david972 · 2021-08-31T08:42:44Z

right now I fight with schema 😉

update sidebar docs improve schema validation and add test remove use less statement from Makefile

aeneasr · 2021-09-11T07:07:35Z

@david972 currently on vacation, but will try to give it a review as soon as I have some stable internet :)

aeneasr

Great stuff! We’re very close :)

cipher/cipher_test.go

cipher/noop.go

docs/docs/guides/reveal-credentials.md

driver/config/.schema/config.schema.json

selfservice/strategy/oidc/types.go

aeneasr · 2021-09-11T15:04:08Z

selfservice/strategy/oidc/types.go

+	Subject               string `json:"subject"`
+	Provider              string `json:"provider"`
+	EncryptedAccessToken  string `json:"encrypted_access_token"`
+	EncryptedRefreshToken string `json:"encrypted_refresh_token"`
 }


Maybe add initial_id_token too?

What will this refer to ?

Some systems issue an ID Token alongside the access_token. It will be available as id_token if set. Some applications might want to use this to perform additional checks! In general, this refers to OpenID Connect :)

I saw that the id token is a jet that may contain audience and nonce. So that mean we should encrypt it as well

test/schema/fixtures/config.schema.test.failure/root.aliases_missing_secrets.yaml

aeneasr · 2021-09-11T15:06:59Z

identity/identity.go

+
+func (i *Identity) getOIDCToken(r *http.Request, c cipher.Provider) error {
+	for credType, credential := range i.Credentials {
+		if credType != CredentialsTypeOIDC {


This is perfect- I just think you don’t need a new struct! Use what we already have with the credentials struct :)

update docs update schema validation

chore: comment useless type feat: move credential in credentials

…ck-oidc-tokens

david972 · 2021-10-03T17:40:50Z

Hi as discussed here is my proposal
@aeneasr
currently

curl http://localhost:4434/identities/2587dd42-4530-4bc9-b04c-07070f32b88e
{
  "id": "2587dd42-4530-4bc9-b04c-07070f32b88e",
  "credentials": {
    "oidc": {
      "type": "oidc",
      "identifiers": [
        "w6d-kratos-oauth2:9482408"
      ],
      "access_token": "gho_xxxxxxxxxxxxxxxxxxxxxx",
      "created_at": "2021-09-29T19:12:48.477763Z",
      "updated_at": "2021-09-29T19:12:48.477763Z"
    },
    "password": {
      "type": "password",
      "identifiers": [
        "david.alexandre@w6d.io"
      ],
      "created_at": "2021-09-29T19:12:48.481521Z",
      "updated_at": "2021-09-29T19:12:48.481521Z"
    }
  },
...

after

{
  "id": "2587dd42-4530-4bc9-b04c-07070f32b88e",
  "credentials": {
    "oidc": {
      "type": "oidc",
      "identifiers": [
        {
          "provider": "github",
          "subject":"9482408",
          "access_token": "gho_xxxxxxxxxx"
        }
      ],
      "created_at": "2021-09-29T19:12:48.477763Z",
      "updated_at": "2021-09-29T19:12:48.477763Z"
    },
    "password": {
      "type": "password",
      "identifiers": [
        {
          "username":"david.alexandre@w6d.io"
        }
      ],
      "created_at": "2021-09-29T19:12:48.481521Z",
      "updated_at": "2021-09-29T19:12:48.481521Z"
    }
  },
...
}

aeneasr · 2021-10-08T10:30:16Z

@david972 I had some time and was able to complete the API changes :) See here: 1c722ff

It follows your proposal!

david972 · 2021-10-11T07:06:43Z

@david972 I had some time and was able to complete the API changes :) See here: 1c722ff

It follows your proposal!

great

Just one thing
If I do

curl 'http://localhost:4434/identities/2587dd42-4530-4bc9-b04c-07070f32b88e'
{
  "id": "2587dd42-4530-4bc9-b04c-07070f32b88e",
  "credentials": {
    "oidc": {
      "type": "oidc",
      "identifiers": [
        "w6d-kratos-oauth2:9482408",
        "w6d-kratos-test:9482408"
      ],
      "created_at": "2021-09-29T19:12:48.477763Z",
      "updated_at": "2021-10-08T20:29:49.946745Z"
    },
    "password": {
      "type": "password",
      "identifiers": [
        "david.alexandre@w6d.io"
      ],
      "created_at": "2021-09-29T19:12:48.481521Z",
      "updated_at": "2021-10-08T20:29:49.943148Z"
    }
  },
  "schema_id": "default",
  "schema_url": "http://127.0.0.1:4433/schemas/default",
  "state": "active",
  "traits": {
    "name": {
      "last": "ALEXANDRE",
      "first": "David"
    },
    "email": "david.alexandre@w6d.io"
  },
  "verifiable_addresses": [
    {
      "id": "1c9b6016-7325-493f-bd34-9daa1110ad9d",
      "value": "david.alexandre@w6d.io",
      "verified": false,
      "via": "email",
      "status": "sent",
      "created_at": "2021-09-29T19:12:48.472902Z",
      "updated_at": "2021-10-08T20:29:49.938865Z"
    }
  ],
  "recovery_addresses": [
    {
      "id": "cf7afd7a-d092-439d-a360-108408ccfc5c",
      "value": "david.alexandre@w6d.io",
      "via": "email",
      "created_at": "2021-09-29T19:12:48.475019Z",
      "updated_at": "2021-10-08T20:29:49.940551Z"
    }
  ],
  "created_at": "2021-09-29T19:12:48.47087Z",
  "updated_at": "2021-09-29T19:12:48.47087Z"
}

but the password part disappear if I do

curl 'http://localhost:4434/identities/2587dd42-4530-4bc9-b04c-07070f32b88e?include_credential=oidc'
{
  "id": "2587dd42-4530-4bc9-b04c-07070f32b88e",
  "credentials": {
    "oidc": {
      "type": "oidc",
      "identifiers": [
        "w6d-kratos-oauth2:9482408",
        "w6d-kratos-test:9482408"
      ],
      "config": {
        "providers": [
          {
            "initial_id_token": "",
            "subject": "9482408",
            "provider": "w6d-kratos-oauth2",
            "initial_access_token": "gho_xxxxxxxxxx",
            "initial_refresh_token": ""
          },
          {
            "initial_id_token": "",
            "subject": "9482408",
            "provider": "w6d-kratos-test",
            "initial_access_token": "ghu_xxxxxxxxxx",
            "initial_refresh_token": "ghr_xxxxxxxxxx"
          }
        ]
      },
      "created_at": "2021-09-29T19:12:48.477763Z",
      "updated_at": "2021-10-08T20:29:49.946745Z"
    }
  },
  "schema_id": "default",
  "schema_url": "http://127.0.0.1:4433/schemas/default",
  "state": "active",
  "traits": {
    "name": {
      "last": "ALEXANDRE",
      "first": "David"
    },
    "email": "david.alexandre@w6d.io"
  },
  "verifiable_addresses": [
    {
      "id": "1c9b6016-7325-493f-bd34-9daa1110ad9d",
      "value": "david.alexandre@w6d.io",
      "verified": false,
      "via": "email",
      "status": "sent",
      "created_at": "2021-09-29T19:12:48.472902Z",
      "updated_at": "2021-10-08T20:29:49.938865Z"
    }
  ],
  "recovery_addresses": [
    {
      "id": "cf7afd7a-d092-439d-a360-108408ccfc5c",
      "value": "david.alexandre@w6d.io",
      "via": "email",
      "created_at": "2021-09-29T19:12:48.475019Z",
      "updated_at": "2021-10-08T20:29:49.940551Z"
    }
  ],
  "created_at": "2021-09-29T19:12:48.47087Z",
  "updated_at": "2021-09-29T19:12:48.47087Z"
}

I understand that the password should not appear but I expected at least to see the same information

aeneasr · 2021-10-11T11:00:07Z

Yeah, that makes sense!

…1818) This patch introduces the new `include_credential` query parameter to the `GET /identities` endpoint which allows administrators to receive the initial access, refresh, and ID tokens from Social Sign In (OpenID Connect / OAuth 2.0) flows. These tokens can be stored in an encrypted format (XChaCha20Poly1305 or AES-GCM) in the database if an appropriate encryption secret is set. To get started easily these values are not encrypted per default. For more information head [over to the docs](https://kratos/docs/guides/retrieve-social-sign-in-access-refresh-id-token). Closes #1518 Closes #397

aeneasr requested changes Jul 12, 2021

View reviewed changes

david972 added 3 commits July 12, 2021 23:28

feat: track oidc token (ory#397)

e96363a

This feature introduce a crypt/decrypt package use for access/refresh token for now Signed-off-by: David ALEXANDRE <david.alexandre@w6d.io>

chore: apply format

989ac60

Signed-off-by: David ALEXANDRE <david.alexandre@w6d.io>

refact: change crypto package

7c66599

remove useless handler improve general identity handler

david972 force-pushed the track-oidc-tokens branch from f550994 to 7c66599 Compare July 14, 2021 13:13

david972 added 5 commits July 14, 2021 15:13

feat: track oidc token (ory#397)

78eff4e

This feature introduce a crypt/decrypt package use for access/refresh token for now Signed-off-by: David ALEXANDRE <david.alexandre@w6d.io>

chore: apply format

4b3e136

Signed-off-by: David ALEXANDRE <david.alexandre@w6d.io>

refact: change crypto package

ea5b2f2

remove useless handler improve general identity handler

Merge branch 'track-oidc-tokens' of github.com:w6d-io/kratos into tra…

973367b

…ck-oidc-tokens

feat: improve make quickstart-dev command to add flexibility in devel…

f91a998

…opment

fix: Implicit memory aliasing in for loop

e878027

david972 requested a review from aeneasr July 21, 2021 18:58

aeneasr added this to the v0.8.0-alpha.1 milestone Aug 1, 2021

harnash added a commit to Wikia/kratos that referenced this pull request Aug 3, 2021

Merge pull request #14 from ory/upstream_blocking_hooks

03d4f8a

Code Review ory#1518

aeneasr added 3 commits August 4, 2021 14:09

Merge remote-tracking branch 'origin/master' into track-oidc-tokens

2dfbed3

# Conflicts: # contrib/quickstart/kratos/email-password/kratos.yml # go.mod # go.sum

chore: code review

c4d58d2

chore: code review

8e8bf21

david972 requested a review from zepatrik as a code owner August 4, 2021 12:54

aeneasr requested changes Aug 4, 2021

View reviewed changes

chore: code review

572c970

aeneasr force-pushed the track-oidc-tokens branch from 5074b6f to 572c970 Compare August 30, 2021 15:57

aeneasr requested changes Aug 30, 2021

View reviewed changes

david972 added 2 commits September 1, 2021 06:11

add noop cipher

d58b3be

update sidebar docs improve schema validation and add test remove use less statement from Makefile

styles: format code

01c8cd8

david972 requested a review from aeneasr September 10, 2021 06:13

aeneasr requested changes Sep 11, 2021

View reviewed changes

david972 and others added 3 commits September 12, 2021 18:35

update comment

5fa73c5

update docs update schema validation

styles: format code

e4df80c

Merge branch 'master' into track-oidc-tokens

af1eb7f

david972 requested a review from aeneasr September 14, 2021 21:35

david972 added 2 commits September 27, 2021 22:08

feat: add id token in credential

12befa1

chore: comment useless type feat: move credential in credentials

Merge branch 'track-oidc-tokens' of github.com:w6d-io/kratos into tra…

3e736fd

…ck-oidc-tokens

aeneasr mentioned this pull request Oct 12, 2021

Track OIDC tokens #1818

Merged

aeneasr closed this in #1818 Oct 12, 2021

beezzoo mentioned this pull request Jul 31, 2022

[Snyk] Security upgrade node-fetch from 2.6.1 to 3.2.10 beezzoo/kratos#9

Open

Rutam21 mentioned this pull request Jul 31, 2022

[Snyk] Security upgrade node-fetch from 2.6.1 to 3.2.10 Rutam21/kratos#11

Open

		EncryptedAccessToken []byte `json:"encrypted_access_token"`
		EncryptedRefreshToken []byte `json:"encrypted_refresh_token"`

feat: track oidc token #1518

feat: track oidc token #1518

Conversation

david972 commented Jul 10, 2021 • edited

Related issue

Proposed changes

Checklist

Further comments

aeneasr left a comment

Choose a reason for hiding this comment

david972 commented Jul 14, 2021

aeneasr commented Jul 23, 2021

aeneasr commented Aug 3, 2021

david972 commented Aug 3, 2021

david972 commented Aug 4, 2021

david972 commented Aug 4, 2021

aeneasr commented Aug 4, 2021

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david972 Aug 14, 2021 • edited

Choose a reason for hiding this comment

aeneasr commented Aug 4, 2021

david972 commented Aug 4, 2021

aeneasr commented Aug 4, 2021

david972 commented Aug 11, 2021 • edited

aeneasr left a comment

Choose a reason for hiding this comment

david972 commented Aug 30, 2021

aeneasr commented Aug 30, 2021

david972 commented Aug 30, 2021 • edited

aeneasr commented Aug 30, 2021

aeneasr commented Aug 31, 2021

david972 commented Aug 31, 2021

david972 commented Aug 31, 2021 • edited

aeneasr commented Sep 11, 2021

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david972 commented Oct 3, 2021 • edited

aeneasr commented Oct 8, 2021

david972 commented Oct 11, 2021

aeneasr commented Oct 11, 2021

david972 commented Jul 10, 2021 •

edited

david972 Aug 14, 2021 •

edited

david972 commented Aug 11, 2021 •

edited

david972 commented Aug 30, 2021 •

edited

david972 commented Aug 31, 2021 •

edited

david972 commented Oct 3, 2021 •

edited