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: client-side PKCE take 3 #4078

Merged
merged 4 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ jobs:
- sdk-generate
services:
postgres:
image: postgres:11.8
image: postgres:14
env:
POSTGRES_DB: postgres
POSTGRES_PASSWORD: test
Expand Down Expand Up @@ -111,15 +111,15 @@ jobs:
- sdk-generate
services:
postgres:
image: postgres:11.8
image: postgres:14
env:
POSTGRES_DB: postgres
POSTGRES_PASSWORD: test
POSTGRES_USER: test
ports:
- 5432:5432
mysql:
image: mysql:5.7
image: mysql:8.0
env:
MYSQL_ROOT_PASSWORD: test
ports:
Expand Down Expand Up @@ -222,15 +222,15 @@ jobs:
- sdk-generate
services:
postgres:
image: postgres:11.8
image: postgres:14
env:
POSTGRES_DB: postgres
POSTGRES_PASSWORD: test
POSTGRES_USER: test
ports:
- 5432:5432
mysql:
image: mysql:5.7
image: mysql:8.0
env:
MYSQL_ROOT_PASSWORD: test
ports:
Expand Down
28 changes: 21 additions & 7 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,8 @@ $(call make-lint-dependency)
echo "deprecated usage, use docs/cli instead"
go build -o .bin/clidoc ./cmd/clidoc/.

.PHONY: .bin/yq
.bin/yq:
go build -o .bin/yq github.com/mikefarah/yq/v4
.bin/yq: Makefile
GOBIN=$(PWD)/.bin go install github.com/mikefarah/yq/[email protected]

.PHONY: docs/cli
docs/cli:
Expand All @@ -58,17 +57,31 @@ docs/swagger:
curl https://raw.githubusercontent.com/ory/meta/master/install.sh | bash -s -- -b .bin ory v0.2.2
touch -a -m .bin/ory

.bin/buf: Makefile
curl -sSL \
"https://github.com/bufbuild/buf/releases/download/v1.39.0/buf-$(shell uname -s)-$(shell uname -m).tar.gz" | \
tar -xvzf - -C ".bin/" --strip-components=2 buf/bin/buf buf/bin/protoc-gen-buf-breaking buf/bin/protoc-gen-buf-lint
touch -a -m .bin/buf

.PHONY: lint
lint: .bin/golangci-lint
golangci-lint run -v --timeout 10m ./...
.bin/golangci-lint run -v --timeout 10m ./...
.bin/buf lint

.PHONY: mocks
mocks: .bin/mockgen
mockgen -mock_names Manager=MockLoginExecutorDependencies -package internal -destination internal/hook_login_executor_dependencies.go github.com/ory/kratos/selfservice loginExecutorDependencies

.PHONY: proto
proto: gen/oidc/v1/state.pb.go

gen/oidc/v1/state.pb.go: proto/oidc/v1/state.proto buf.yaml buf.gen.yaml .bin/buf .bin/goimports
.bin/buf generate
.bin/goimports -w gen/

.PHONY: install
install:
GO111MODULE=on go install -tags sqlite .
go install -tags sqlite .

.PHONY: test-resetdb
test-resetdb:
Expand Down Expand Up @@ -163,11 +176,12 @@ authors: # updates the AUTHORS file

# Formats the code
.PHONY: format
format: .bin/goimports .bin/ory node_modules
.bin/ory dev headers copyright --exclude=internal/httpclient --exclude=internal/client-go --exclude test/e2e/proxy/node_modules --exclude test/e2e/node_modules --exclude node_modules
format: .bin/goimports .bin/ory node_modules .bin/buf
.bin/ory dev headers copyright --exclude=gen --exclude=internal/httpclient --exclude=internal/client-go --exclude test/e2e/proxy/node_modules --exclude test/e2e/node_modules --exclude node_modules
goimports -w -local github.com/ory .
npm exec -- prettier --write 'test/e2e/**/*{.ts,.js}'
npm exec -- prettier --write '.github'
.bin/buf format --write

# Build local docker image
.PHONY: docker
Expand Down
12 changes: 12 additions & 0 deletions buf.gen.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
version: v2
managed:
enabled: true
override:
- file_option: go_package_prefix
value: github.com/ory/kratos
plugins:
- remote: buf.build/protocolbuffers/go
out: gen
opt: paths=source_relative
inputs:
- directory: proto
9 changes: 9 additions & 0 deletions buf.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
version: v2
modules:
- path: proto
lint:
use:
- DEFAULT
breaking:
use:
- FILE
6 changes: 6 additions & 0 deletions cipher/chacha20.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"crypto/rand"
"encoding/hex"
"io"
"math"

"github.com/pkg/errors"
"golang.org/x/crypto/chacha20poly1305"
Expand Down Expand Up @@ -43,6 +44,11 @@
return "", herodot.ErrInternalServerError.WithWrap(err).WithReason("Unable to generate key")
}

// Make sure the size calculation does not overflow.
if len(message) > math.MaxInt-aead.NonceSize()-aead.Overhead() {
Copy link
Member

Choose a reason for hiding this comment

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

This method is taken from cryptopasta, if I recall correctly. Could you point me to the documentation / issue this addresses? This method is being used in several places, and I am worried this will introduce regressions. Granted, MaxInt is pretty large.

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 AES-GCM code comes from cryptopasta. The XChaCha code was hand-written from the start.

In an earlier revision of this PR, I touched this code but reverted it later to the original version. GitHub's security scanner then started complaining, correctly, that the size calculation below could overflow. Even though the code was unchanged from master. So I added this assertion, which is the exact same as the one in Hydra.

return "", errors.WithStack(herodot.ErrInternalServerError.WithReason("plaintext too large"))

Check warning on line 49 in cipher/chacha20.go

View check run for this annotation

Codecov / codecov/patch

cipher/chacha20.go#L49

Added line #L49 was not covered by tests
}

nonce := make([]byte, aead.NonceSize(), aead.NonceSize()+len(message)+aead.Overhead())
_, err = io.ReadFull(rand.Reader, nonce)
if err != nil {
Expand Down
9 changes: 5 additions & 4 deletions cmd/identities/get_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package identities_test

import (
"context"
"encoding/hex"
"encoding/json"
"testing"

Expand Down Expand Up @@ -63,10 +62,12 @@ func TestGetCmd(t *testing.T) {
return out
}
transform := func(token string) string {
if !encrypt {
return token
if encrypt {
Copy link
Member

Choose a reason for hiding this comment

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

I assume this changed because we now, somewhere, properly check if the token can be decrypted and then this test failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. I changed most tests across Kratos to actually use encryption, and this one was the only one to fail. Good sign :)

s, err := reg.Cipher(context.Background()).Encrypt(context.Background(), []byte(token))
require.NoError(t, err)
return s
}
return hex.EncodeToString([]byte(token))
return token
}
return identity.Credentials{
Type: identity.CredentialsTypeOIDC,
Expand Down
2 changes: 1 addition & 1 deletion cmd/identities/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"github.com/ory/kratos/internal/testhelpers"
)

func setup(t *testing.T, newCmd func() *cobra.Command) (driver.Registry, *cmdx.CommandExecuter) {
func setup(t *testing.T, newCmd func() *cobra.Command) (*driver.RegistryDefault, *cmdx.CommandExecuter) {
conf, reg := internal.NewFastRegistryWithMocks(t)
_, admin := testhelpers.NewKratosServerWithCSRF(t, reg)
testhelpers.SetDefaultIdentitySchema(conf, "file://./stubs/identity.schema.json")
Expand Down
183 changes: 183 additions & 0 deletions gen/oidc/v1/state.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading